Add --append-old-sha option to migrate import command, appends old-SH…#5948
Add --append-old-sha option to migrate import command, appends old-SH…#5948dparrini wants to merge 1 commit intogit-lfs:mainfrom
Conversation
…A=<SHA> to rewritten commit messages
chrisd8088
left a comment
There was a problem hiding this comment.
Thank you so much for this PR! I think it's a good idea to be able to offer this additional functionality when using the git lfs migrate command, and our users will appreciate it.
There are a couple of things we'll need for this PR to be complete, in addition to the things I've mentioned in review comments regarding terminology and handling commits with pre-existing trailers.
First, I think we should offer this functionality when using both the export and import sub-commands of the git lfs migrate command, not just with the import sub-command.
An entry describing the behaviour of this feature and its new command-line option will need to be added to both the IMPORT section and the EXPORT section of the git-lfs-migrate(1) manual page.
Last but not least, we'll want to add a number of tests to the t/t-migrate-import.sh test script and the t/t-migrate-export.sh test script. (I'd probably add them near the tests of the --object-map flag, myself.)
These don't need to be especially complicated tests, but should exercise the handling of commits with pre-existing trailers (i.e., no extra whitespace line between the existing trailers and the LFS-* one we generate) and commits without such trailers (i.e., one extra whitespace line only). Other things to check include:
- Handling commits with and without trailing line feed characters. Git normally ensures there is one trailing
\n, per thegit-whitespace(1)manual page, but we should be prepared to handle unusual cases created by, for instance, using thegit commit --cleanup=verbatimoption. If there is no trailing LF character, we'll need to add two before our trailer; if there's just one, we'll need to add one (unless the final line is itself a trailer); and if there's more than one we can just append our trailer line. - Handling commits where no changes are made and the SHA stays the same, so we want to verify our trailer is not appended in this case.
- Using the new flag with the
--no-rewriteoption should result in an error message and no changes.
Thank you again very much for getting this work underway, we really appreciate these kinds of contributions from the community!
| copy(newSha, oid) | ||
| } else { | ||
| if opt.AppendOldShaToMessages { | ||
| rewrittenCommit.Message = rewrittenCommit.Message + "\n\nold-SHA=" + fmt.Sprintf("%x", oid) + "\n" |
There was a problem hiding this comment.
To my knowledge, this is the first time the Git LFS project has introduced a commit message trailer of its own, so we have an opportunity to choose the format of this trailer in a way that allows for as much consistency as possible in the future.
My principal request is that we start the trailer name with LFS-, as I think we should use that key prefix on every trailer we generate.
Next, I think we want to try to align the wording of our trailer's key with those used by several other major project, especially as defined for the Linux kernel and recommended for Git itself. These typically take the form of a verb in the past tense, followed by a preposition, e.g., Acked-by and Reviewed-by.
I would therefore suggest we use LFS-imported-from and LFS-exported-from as the keys for these trailers, but I'm open to other ideas, so long as we keep the LFS- prefix.
| importCmd.Flags().BoolVar(&migrateNoRewrite, "no-rewrite", false, "Add new history without rewriting previous") | ||
| importCmd.Flags().StringVarP(&migrateCommitMessage, "message", "m", "", "With --no-rewrite, an optional commit message") | ||
| importCmd.Flags().BoolVar(&migrateFixup, "fixup", false, "Infer filepaths based on .gitattributes") | ||
| importCmd.Flags().BoolVar(&appendOldShaToMessages, "append-old-sha", false, "Append old commit SHA to rewritten commit messages") |
There was a problem hiding this comment.
The Git project refers to RFC 822-style key/value pairs added at the end of a commit message as "trailers"; see, for example, the git-interpret-trailers(1) manual page and the --trailer option to the git commit command.
Following that example, I think we should perhaps call this new option to our command something like --object-map-trailer, as that would align with the existing --object-map option we already provide.
| copy(newSha, oid) | ||
| } else { | ||
| if opt.AppendOldShaToMessages { | ||
| rewrittenCommit.Message = rewrittenCommit.Message + "\n\nold-SHA=" + fmt.Sprintf("%x", oid) + "\n" |
There was a problem hiding this comment.
One concern here is that at the moment this line always appends two line feed characters, followed by the trailer line, which assumes that the commit message does not already contain a trailer like Co-authored-by, which many projects and services (like GitLab and GitHub) support. We would not want to end up with a commit message with these trailers separated by a blank line, e.g.:
Add large file tests to validate new trailer behaviour!
Co-authored-by: Other Person <person@example.com>
LFS-imported-from: 0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33
So I think, unfortunately, that we'll want this code to do a bit of parsing to look for pre-existing trailers, and only if there are none add an extra line of whitespace before the new trailer.
| @@ -147,8 +147,9 @@ func migrateImportCommand(cmd *cobra.Command, args []string) { | |||
| } | |||
|
|
|||
| migrate(args, rewriter, l, &githistory.RewriteOptions{ | |||
There was a problem hiding this comment.
Because it won't make sense for users to supply this option along with the --no-rewrite option, we should probably add a check above this point (and also in the commands/command_migrate_export.go file in a similar place) to return an error message to the user if they provide both --no-rewrite and --object-map-trailer.
This could occur in roughly the same place as the existing check which disallows the use of --no-rewrite together with --fixup, for example.
Hi, this PR is a candidate solution to the issue that I opened today:
#5947
The original tests passes (tested on WSL) but I'm not sure how to add tests for this feature. It also lacks mention on documentation. Any help is appreciated if this PR is reviewed as useful.