Re: [PATCH v4 1/1] MacOS: precompose_argv_prefix()

On Wed, Feb 03, 2021 at 02:13:53PM -0800, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

Just as the prefix-less variant was idempotent and that was the
reason why cmd_diff_files() had its own precompose() even if the
incoming argv[] is supposed to be already precomposed because it
was processed in another call to precompose() in run_builtin(),
this patch keeps these seemingly redundant calls, because it is not
meant as a clean-up but as a bugfix for the prefix part.

OK. ... Ah, no, the call in run_builtin() is a new thing.  We didn't
have it, and the redundant call is what this patch introduced, so
we need to be a bit more careful about the analysis here.  It is one
thing to say "we leave the existing iffy code and address only a
single bug" and do so.  It is entirely different to say so and then
do "we introduce an iffy code and address only a single bug".  We
need to admit that what we added _is_ iffy but supposed to be safe.
Just saying "it is supposed to be safe" without saying why it is
iffy is dishonest and does not help future developers who may want
to jump in and clean the code.

Perhaps

	Now add it into git.c::run_builtin() as well.  Existing
	precompose calls in diff-files.c and others would become
	redundant but because we do not want to make sure that there
	is no way for the control to reach them without passing
	run_builtin(), we'll keep them in place just in case.  The
	calls to precompose() are idempotent so it should not hurt.

or something?

FYI, I've tweaked the proposed log message like the following before
queuing.  I think that would be explicit enough to remind us that we
may be able to improve the situation fairly easily.

Thanks for helping me out.
That gives me some time to look deeper and prepare more test cases
ana a then cleanup on top of this patch (later).