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

[Jeff King <peff@peff.net>]

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

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

> 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