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

[Elijah Newren <newren@gmail.com>]

On Tue, Apr 23, 2019 at 11:29 AM Torsten Bögershausen <tboegi@web.de> wrote:
>
> On Tue, Apr 23, 2019 at 10:30:56AM -0700, Elijah Newren wrote:
> > On Mac's HFS ("Hilarious FileSystem"?  "Halfwitted FileSystem"?) --
>
> How about "Hierarchical File System" ?

Sorry, I should have removed my draft commentary before submitting.
Yes, you are of course right.

...
> > Add code in a few places (pack-refs, show-ref, upload-pack) to check and
> > honor the setting of core.precomposeUnicode to avoid these bugs.
>
> That's all correct, one minor question below.
...

> > @@ -16,6 +17,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
> >               OPT_BIT(0, "prune", &flags, N_("prune loose refs (default)"), PACK_REFS_PRUNE),
> >               OPT_END(),
> >       };
> > +     git_config(git_default_config, NULL);
> >       if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
> >               usage_with_options(pack_refs_usage, opts);
>
> I wonder if we could move the call to git_config() into parse_options(),
> (or another common place) but I haven't checked the details yet.
> Same below for show_ref().

Interesting idea.  However, moving it into parse_options() presumes
that either `git_default_config` will always be passed to
git_config(), or else the arguments needed by git_config() will be
explicitly passed to parse_options.  A quick grep through the source
code suggests that only about 1/3 of callsites pass git_default_config
to git_config(), and passing a config-related callback function to
parse_options seems a little weird to me.  Plus, the fact that 2/3 of
the callsites use a special config callback function suggests that
we'd still not catch all cases with such a change, much like I had to
update the upload_pack_config special callback below.

> And thankks for picking this up.
>
> >       return refs_pack_refs(get_main_ref_store(the_repository), flags);
> > diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> > index 6a706c02a6..6456da70cc 100644
> > --- a/builtin/show-ref.c
> > +++ b/builtin/show-ref.c
> > @@ -1,5 +1,6 @@
> >  #include "builtin.h"
> >  #include "cache.h"
> > +#include "config.h"
> >  #include "refs.h"
> >  #include "object-store.h"
> >  #include "object.h"
> > @@ -182,6 +183,8 @@ static const struct option show_ref_options[] = {
> >
> >  int cmd_show_ref(int argc, const char **argv, const char *prefix)
> >  {
> > +     git_config(git_default_config, NULL);
> > +
> >       argc = parse_options(argc, argv, prefix, show_ref_options,
> >                            show_ref_usage, 0);
> >
> > 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);
> >       }
> >
> >       if (current_config_scope() != CONFIG_SCOPE_REPO) {
> > --
> > 2.21.0.420.g4906d192b3
> >