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

Junio C Hamano [off-list ref] writes:

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.


1:  071dfe8793 ! 1:  5c327502db MacOS: precompose_argv_prefix()
    @@ Commit message
     
         The original patch for preocomposed unicode, 76759c7dff53, placed
         precompose_argv() into parse-options.c
    -    Now add it into git.c (as well) for commands that don't use parse-options.
    -    Note that precompose() may be called twice - it is idempotent.
    +
    +    Now add it into git.c::run_builtin() as well.  Existing precompose
    +    calls in diff-files.c and others may become redundant, and if we
    +    audit the callflows that reach these places to make sure that they
    +    can never be reached without going through the new call added to
    +    run_builtin(), we might be able to remove these existing ones.
    +
    +    But in this commit, we do not bother to do so and leave these
    +    precompose callsites as they are.  Because precompose() is
    +    idempotent and can be called on an already precomposed string
    +    safely, this is safer than removing existing calls without fully
    +    vetting the callflows.
     
         There is certainly room for cleanups - this change intends to be a bug fix.
         Cleanups needs more tests in e.g. t/t3910-mac-os-precompose.sh, and should

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