search

mhagger / cvs2svn

Migrate CVS repositories to Subversion or Git. This site supersedes the old tigris.org site, which has shut down.
Other
77 stars 43 forks source link

Allow svn:executable property to set filemode in git #6

Closed mrotteveel closed 6 years ago

mrotteveel commented 7 years ago

This allows the svn:executable property set using the AutoPropsPropertySetter to control the filemode for git conversions.

Instead of only checking the executable status of the cvs_file, it will also check for presence of svn:executable in the properties.

While migrating a CVS repository that didn't have the executable bit set for a lot of files, I ran into the issue that adding svn:executable through autoprops had no influence on the git migration.

mhagger commented 7 years ago

@mrotteveel: Thanks for your PR :sparkles: This is a good idea and the goal makes perfect sense.

I think that cvs2git already sets up ExecutablePropertySetter no matter how it is invoked, which should already set the svn:executable property based on whether the CVS file had its executable bit set. This tells me that we could always base this decision on the SVN property, without looking at cvs_item.cvs_file.executable at all. I think this would also accomplish your goal. I.e., instead of

if cvs_item.cvs_file.executable or 'svn:executable' in cvs_item.cvs_file.properties:

, do you get the desired result by changing the two lines to

if 'svn:executable' in cvs_item.cvs_file.properties:

?

It would be a bonus if there were a test of the new feature, though I won't insist on it since the test coverage of cvs2git is already so thin.

Thanks!

mrotteveel commented 7 years ago

@mhagger I'll take a look this weekend if that change does the job as well (I suspect so). I'll also see if I can create a test. My python is bit rusty though ;)

mrotteveel commented 7 years ago

@mhagger I have made changes to only check for presence of 'svn:executable'.

I haven't added a test, for two reasons: 1) I couldn't get the existing tests to run (I'm not sure if that is a result of using Windows Subsystem for Linux, or if I'm missing a necessary package for the test to run), and 2) the existing tests for git don't seem to check anything and I wasn't sure how to proceed from there.

Instead I have re-run the import that originally prompted me to make this change and verified that.

mhagger commented 6 years ago

@mrotteveel: That's great, thanks! I just merged your changes.

Since the source of truth for this project is still in Subversion :sad:, I had to git-svnify your patches, which results in their listing me as the author :sob:. The true authorship is recorded (as per project convention) using a "Patch by" annotation in the commit message. I have also squashed the two patches together.

Thanks again for your contribution! :sparkles: