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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 29, 2024, 18:37 UTC
Message-ID: <xmqqede0htp2.fsf@gitster.g>
In-Reply-To: <20240129015708.GA1762343@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 7 quoted lines

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

Show 6 quoted lines

> But I'm not sure what fallout we might have from changing that behavior
> now. So this patch takes the less-risky option, and simply teaches
> run_external_diff() to avoid passing xfrm_msg when it's NULL. That makes
> it agnostic to whether "other" and "xfrm_msg" always come as a pair. It
> fixes the segfault now, and if we want to change the --no-index "other"
> behavior on top, it will handle that, too.

Sounds sensible.

Thanks.  Will queue.