Elijah Newren <newren@xxxxxxxxx> 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.
>> > 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.
