frej / fast-export

A mercurial to git converter using git-fast-import
http://repo.or.cz/w/fast-export.git
808 stars 255 forks source link

Add fix_path plugin #241

Closed bueli closed 3 years ago

bueli commented 4 years ago

This plugin sanitizes pathnames containing // (double slash) which will trigger Empty path component found in input issue

bueli commented 4 years ago

Reduced code and fixed commit message as you requested

bueli commented 4 years ago

Hi Frej

Will squash soon.

I think your quality goals are very appropriate for production code. But here we are in a deserted corner of an auxiliary tool ... Why do you spend and demand so much effort?

You almost lost me, but now i want to understand where the energy is coming from ...

Cheers, bue

Am 09.09.2020 um 14:41 schrieb Frej Drejhammar notifications@github.com:

 @frej commented on this pull request.

In plugins/fix_path/init.py:

@@ -0,0 +1,12 @@ +def build_filter(args):

  • return Filter(args)
  • +class Filter:

  • def init(self, args):
  • i = 1
  • file_data = {'filename':filename,'file_ctx':file_ctx,'d':d}

  • def file_data_filter(self, file_data):
  • filename = file_data['filename']
  • if filename and '//' in filename: Forget the comment about compiling the regexp, I read the code hastily, sorry.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.

frej commented 4 years ago

But here we are in a deserted corner of an auxiliary tool ... Why do you spend and demand so much effort? You almost lost me, but now i want to understand where the energy is coming from

It's to minimize the maintenance effort, the clearer the patches, the simpler it is to maintain as I can figure out what a change is supposed to be doing. This is a real time saver, as with the declining popularity of Mercurial, I expect the frequency of maintenance actions to become more and more infrequent. The less time I have to spend to remember how something works, the happier I am.

As a maintainer, merging code is always a risk, if I merge a sloppy looking patch, will I have to maintain it in the future, are there hidden bugs? A sloppy looking patch does not really inspire confidence in the quality of the code, especially if the effort to make it "right" in the first place would have been minimal and only would have required a read-through on part of the contributor. Also the willingness to respond to review suggestions and questions is, I have found, a pretty good indication if the contributor will help me if unforeseen problems show up after the merge.

bueli commented 4 years ago

Please squash your commits, there is no reason to preserve the development history of this small change for eternity.

Also make sure that the body of the commit message is wrapped at, at most 80, chars. The previous linked recommendations suggests around 72 characters.

Request a re-review when done.

It's squashed

frej commented 4 years ago

For the third time, why do you check if // occurs in the string before running filename.replace("//", "/"), instead of running replace() directly? Checking the Python implementation there is no performance benefit from it, you'll end up scanning the string twice when the pattern // is in the string.

frej commented 3 years ago

No response to review comments in more than 90 days, closing without prejudice for inactivity.