Re: [PATCH] Honor core.precomposeUnicode in more places

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 24, 2019, 01:56 UTC
Message-ID: <xmqq5zr4414e.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CABPp-BGbDN2DVwJKh3gdQ1wDZmGuevAYRhhfe_MCyunABDsEqg@mail.gmail.com>

Elijah Newren <newren@gmail.com> writes:

> ..., and passing a config-related callback function to
> parse_options seems a little weird to me.

A little?  That's a moderate understatement.

If parse_options API were the ONLY thing that gets affected by precompose_unicode, having an "if the config has not been read yet, read only that configuration variable and nothing else" there might make sense, but that is not the case (i.e. the variable also affects how readdir() works). So the alternatives that make sense are

 (1) we stick to the current "make sure we read that variable
     sufficiently early in the main flow of the program" pattern,
     which this patch does, or

 (2) we switch to "make sure the variable is read before we need it"
     pattern, i.e. add that "read the single config variable from
     the file, if it is not yet read" call to both parse_options()
     and opendir()---if the set of operations affected by the
     precompose_unicode variable grows, we'd need to add the same
     call to the new places, too.

I think (1) is good at least for now.

Show 11 quoted lines

>> > diff --git a/upload-pack.c b/upload-pack.c
>> > index d098ef5982..159f751ea4 100644
>> > --- a/upload-pack.c
>> > +++ b/upload-pack.c
>> > @@ -1064,6 +1064,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
>> >               allow_ref_in_want = git_config_bool(var, value);
>> >       } else if (!strcmp("uploadpack.allowsidebandall", var)) {
>> >               allow_sideband_all = git_config_bool(var, value);
>> > +     } else if (!strcmp("core.precomposeunicode", var)) {
>> > +             precomposed_unicode = git_config_bool(var, value);
>> >       }

What the other hunks wanted to do was quite obvious (i.e. before calling parse_options(), make sure we know precomposed_unicode is set appropriately, so that argv[] can be tweaked correctly). But this one was a bit less clear.

It turns out that upload-pack deliberately avoids using the default config callback, but tries to limit itself to the minimally needed set, so this hunk adds the precomposed_unicode to it. By doing so, we trigger another effect of precomposed_unicode, i.e. tweaking the paths we read out of readdir(), so that the refs we have to offer to the other side are all normalzied to the precomposed form.

Makes sense.  Thanks.