Add a merge driver #4970
Add a merge driver #4970
Conversation
chrisd8088
left a comment
chrisd8088
left a comment
There was a problem hiding this comment.
Wow, did I learn a lot reading up on merge drivers! Thank you for all this work!
I had a number of thoughts and comments, but some of them may not be germane or not fully baked; my apologies for any of that sort.
| cmd.Flags().StringVarP(&mergeDriverAncestor, "ancestor", "", "", "file with the ancestor version") | ||
| cmd.Flags().StringVarP(&mergeDriverCurrent, "current", "", "", "file with the current version") | ||
| cmd.Flags().StringVarP(&mergeDriverOther, "other", "", "", "file with the other version") | ||
| cmd.Flags().StringVarP(&mergeDriverOutput, "output", "", "", "file with the output version") |
There was a problem hiding this comment.
IIUC, this git-lfs-merge program will be the merge driver from Git's perspective, and then we'll invoke either the default git-merge-file or a user-specified program. Either way, as the "actual" or outermost merge driver, according to the docs it seems like it's our responsibility to leave the output in the same location as the current version input file:
The merge driver is expected to leave the result of the merge in the file named with
%Aby overwriting it, and exit with zero status if it managed to merge them cleanly, or non-zero if there were conflicts.
If that's the case, could we drop the --output option (or equivalent fourth argument) entirely? In the example in the manual page below, it shows:
git config merge.lfs-text.driver 'git lfs merge-driver --ancestor %O --current %A --other %B --marker-size %L --output %A
If the user is effectively always required to specify %A for both --current and --output, perhaps we can simplify things by skipping the latter and just outputting the clean()ed result back to the mergeDriverCurrent filename?
There was a problem hiding this comment.
We can, but I really don't like special-casing --current like that. In my view, it's a bug that Git has decided to specify the driver like that, since it makes it hard to reason about the behaviour. It also makes it hard to integrate with other programs which may want to use this outside of Git, which I'd like to keep open as a possibility for scripting purposes.
|
Okay, I've put in several fixup commits here that should address everything I didn't specifically respond to. Feel free to take a look at your leisure and see if there's anything I missed or if you have further comments. |
|
I think there may be a number of comments which haven't been addressed yet, is that possible? For instance, I think we'll want to update Is it possible you have some commits waiting to be pushed? |
That was indeed the case. I've now pushed them. |
chrisd8088
left a comment
chrisd8088
left a comment
There was a problem hiding this comment.
This looks great; thank you for addressing all those bits and pieces. I had one new minor note only!
In the Git merge driver code, Git uses percent sequences (à la printf(3)) to handle the file names passed to the merge driver. We'll want to do the same thing when allowing the user to perform a merge on their own using a custom program, so let's add a function that replaces percent sequences in a string with their specified replacement, shell-quoting each argument along the way. This uses the same algorithm that Git does.
Sometimes users have text files that they'd like to be able perform merges on that they also want to store in Git LFS. Let's add a custom merge driver that users can use to merge these text files, either with Git, or with a custom merge tool like the one shipped with Unity.
We ported these tests out of the tools package into the subprocess package but didn't update the strings. Let's do so now.
In commit 402e958 of PR git-lfs#4970 we implemented the new "git lfs merge-driver" command, which can be configured as a custom Git merge driver in order to help automatically merge some types of files when they are tracked as Git LFS objects. We expect this command to be invoked by Git during a merge operation, and so several of its command-line flags are required, as their arguments must specify the paths of the temporary files created by Git for use by a merge driver program. However, our git-lfs-merge-driver(1) manual page lists these required flags and their file path parameters as options, without any indication that they are actually required. As well, one of the required flags, the --output flag, is not described at all. We therefore revise this manual page to document the mandatory flags, including the --output flag. We also take the opportunity to expand the description of these flags in order to provide somewhat more context as to their meaning, and we adjust a few other minor details in the page as well.
In commit 402e958 of PR git-lfs#4970 we implemented the new "git lfs merge-driver" command, which can be configured as a custom Git merge driver in order to help automatically merge some types of files when they are tracked as Git LFS objects. We expect this command to be invoked by Git during a merge operation, and so several of its command-line flags are required, as their arguments must specify the paths of the temporary files created by Git for use by a merge driver program. Our git-lfs-merge-driver(1) manual page describes these flags but accidentally omits one of them, the --output flag, so we now add an entry for it into the list of options supported by the command. We also take the opportunity to slightly adjust the description of the "%D" placeholder token to indicate that it corresponds to the value of the --output option's argument. (If the user specifies a custom command with the --program option, the "git lfs merge-driver" command will run that command after substituting any placeholder tokens with their corresponding values.) Note that technically some of the "git lfs merge-driver" command's flags and their arguments are not actually options at all, since they are required for the "git lfs merge-driver" command to run. This includes the --output flag as well as the --ancestor, --current, and --other flags. However, since we expect to further revise the git-lfs-merge-driver(1) manual page in a future PR, we confine ourselves at present to simply adding an entry for the missing --output flag to the existing list of options and clarifying the flag's corresponding "%D" placeholder token.
3 participants
Sometimes users have text files that they'd like to be able perform merges on that they also want to store in Git LFS. Let's add a custom merge driver that users can use to merge these text files, either with Git, or with a custom merge tool like the one shipped with Unity.
There's no code to wire this up automatically. We can decide to support that in a future revision if users would find that valuable.
Fixes #4820
/cc @dr-dime as reporter