Re: git-diff passes <rev>:<path> args to GIT_EXTERNAL_DIFF incorrectly?

[Jeff King <peff@peff.net>]

On Fri, Aug 30, 2019 at 04:23:13PM +0300, Dmitry Nikulin wrote:

> 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).

> 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