Re: [PATCH 1/3] diff: use filespec path to set up tempfiles for ext-diff

From: Jeff King <peff@peff.net>
Date: Jan 6, 2023, 13:10 UTC
Message-ID: <Y7gdsxNT2TM/5M17@coredump.intra.peff.net>
In-Reply-To: <xmqqbknbsu9y.fsf@gitster.g>

On Fri, Jan 06, 2023 at 09:48:57PM +0900, Junio C Hamano wrote:

Show 6 quoted lines

> > The breakage seems to go all the way back to cd676a5136 (diff
> > --relative: output paths as relative to the current subdirectory,
> > 2008-02-12).
> 
> Not surprising.  When I wrote all the rest of "diff", I didn't
> plan to do "--relative" ;-)

:) Commit cd676a5136 mentions that "diff --relative" does not interact well with "--no-index", and that was one of the things I tested while poking (to make sure I did not make anything worse). And indeed, it seems that --relative is mostly ignored there. We could follow through on the plan from the end of the commit message to forbid combining the two, but it may not be that important given how long it has been an issue (and that I think people may set diff.relative in their config these days).

Show 5 quoted lines

> Thanks for a clear description.  The fix looks trivially obvious and
> correct.
> [...]
> > -	struct diff_tempfile *temp = prepare_temp_file(r, name, df);
> > +	struct diff_tempfile *temp = prepare_temp_file(r, df->path, df);

One nagging concern I had is whether "df->path" might ever point to something unexpected (or even be NULL). The fact that textconv unconditionally passes it made me feel a lot better. But I also ended up walking back to the source of the "name" and "other" fields, which is this code in run_diff():

	name  = one->path;
	other = (strcmp(name, two->path) ? two->path : NULL);
	attr_path = name;
	if (o->prefix_length)
		strip_prefix(o->prefix_length, &name, &other);

So those values really are just aliases for one->path and two->path, modulo the prefix stripping (which as you might guess, came from cd676a5136 itself). And using them directly instead of the stripped versions is definitely the right thing to do.

-Peff