Re: git-diff passes <rev>:<path> args to GIT_EXTERNAL_DIFF incorrectly?
On Fri, Aug 30, 2019 at 04:23:13PM +0300, Dmitry Nikulin wrote:
Show 21 quoted lines
> On Fri, 30 Aug 2019 at 13:16, Phillip Wood <phillip.wood123@gmail.com> wrote:
> > I'm not sure why the last argument is being split in
> > your example. It is not split in the example below
>
> I have replicated the splitting issue on my small demo repo [1]:
>
> $ env GIT_EXTERNAL_DIFF=./print_argv.py git diff -M origin/branch1
> origin/branch1-mv -- file1.txt file1-mv.txt
> ['./print_argv.py',
> 'file1.txt',
> '/tmp/EWaCSc_file1.txt',
> '2bef330804cb3f6962e45a72a12a3071ee9b5888',
> '100644',
> '/tmp/mtEiSc_file1-mv.txt',
> 'f8fd6737fbe5a45c97ba9c9de495dc46ff11eccd',
> '100644',
> 'file1-mv.txt',
> 'similarity index 90%\n'
> 'rename from file1.txt\n'
> 'rename to file1-mv.txt\n'
> 'index 2bef330..f8fd673 100644\n']
Interesting. I _don't_ see that splitting when I run the same command in your demo repo (nor, looking at Git's code, do I see how it could happen; we always add the metainfo as a single argument).
Show 27 quoted lines
> This is, however, tangential to the original problem: documenting the
> external diff CLI interface for diffing two blobs. Here is what I am
> seeing:
>
> $ env GIT_EXTERNAL_DIFF=./print_argv.py git diff
> origin/branch1:file1.txt origin/branch1-mv:file1-mv.txt
> ['./print_argv.py',
> 'file1.txt',
> '/tmp/n9USvy_file1.txt',
> '2bef330804cb3f6962e45a72a12a3071ee9b5888',
> '100644',
> '/tmp/Zst0uy_file1-mv.txt',
> 'f8fd6737fbe5a45c97ba9c9de495dc46ff11eccd',
> '100644',
> 'file1-mv.txt',
> 'index 2bef330..f8fd673 100644\n']
>
> The meaning and origin of the last arg remains mysterious, and the
> other args do not conform to the published documentation[2], which
> states that the args should be:
>
> path old-file old-hex old-mode new-file new-hex new-mode
>
> Instead the args that are passed are:
>
> path old-filename old-file old-hex old-mode new-file new-hex
> new-mode new-filename something
I think you have an extra "old-filename" in the second list. Without that, the first 7 arguments are as documented.
The last two are:
- when the destination path differs from the source path, we append
it. This is generally a sign of a rename/copy, though it triggers
in the blob case because Git has been manually given a pair of files
with non-matching names. - the final one is metainfo that Git typically prints between the
"diff" header and the diff itself. This is added only when we add
the filename, and would generally carry information about the rename
(but of course since there isn't one, it has only the index line).These were both added by 427dcb4bca ([PATCH] Diff overhaul, adding half of copy detection., 2005-05-21). I think the intent was that existing diff commands would/could ignore the extra arguments.
Certainly the world would be a better place if those were described in the external-diff documentation (specifically in relation to renames, which is their intended use).
As for the behavior in the blob-diff case, I think it's _pretty_ reasonable. It's certainly useful to give the new-filename. The metainfo is mostly useless in this case, and potentially could be suppressed if the pair is not an actual rename/copy. But I also think it's not hurting much (and a script can tell that's what's going on by the lack of rename lines in the metainfo).
-Peff
