Re: [PATCH] diff: handle NULL meta-info when spawning external diff

From: Jeff King <peff@peff.net>
Date: Jan 30, 2024, 06:06 UTC
Message-ID: <20240130060658.GD166761@coredump.intra.peff.net>
In-Reply-To: <xmqqede0htp2.fsf@gitster.g>

On Mon, Jan 29, 2024 at 10:37:29AM -0800, Junio C Hamano wrote:

Show 20 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> >> $ git diff --no-index foo bar
> >> zsh: segmentation fault (core dumped)  git diff --no-index foo bar
> >
> > Thanks for providing a simple reproduction recipe. There's a pretty
> > straight-forward fix below, though it leaves open some question of
> > whether there's another bug lurking with --no-index (but either way, I
> > think we'd want this simple fix as a first step).
> 
> Yup, I agree with you that the "--no-index" mode violates the basic
> design that "the other path" and "xfrm_msg" go hand-in-hand.  In its
> two tree comparison mode "git diff --no-index A/ B/", it should be
> able to behave sensibly, but in its two files comparison mode to
> compare plain regular files 'foo' and 'bar', there is nothing it can
> do reasonably, I am afraid.  You could say that the change is
> renaming 'foo' to create 'bar', and feed consistent data that is
> aligned with that rename to external diff, which might be slightly
> more logical than showing a change to 'foo' that has no rename
> involved (i.e. omitting "other name"), but neither is satisfying.

Yeah, I think the two-tree mode does behave correctly, and this is really just about the two-blob mode. I agree that one could think of it as a rename or not, depending on how much you want to read into the importance of the names (after all, you could compare a/foo and b/foo, which is sort of a moral equivalent of the usual two-tree case).

The current behavior is somewhere in between, though. You get an "other" name passed to the external diff, but the metainfo argument makes no mention of a rename (it's either blank for an exact rename, or may contain an "index" line if there was a content change).

I'm not sure anybody really cares that much either way, though. It's external diff, which I suspect hardly anybody uses, and those extra fields aren't even documented in the first place.

-Peff