Re: [PATCH] Honor core.precomposeUnicode in more places
- From
Elijah Newren <newren@gmail.com>
- Date
- Apr 23, 2019, 19:06 UTC
- Message-ID
- <CABPp-BGbDN2DVwJKh3gdQ1wDZmGuevAYRhhfe_MCyunABDsEqg@mail.gmail.com>
- In-Reply-To
- <20190423182924.r6mkwrl2o7pcwjoa@tb-raspi4>
On Tue, Apr 23, 2019 at 11:29 AM Torsten Bögershausen <tboegi@web.de> wrote:
Show 5 quoted lines
>
> 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.
...
Show 11 quoted lines
> > @@ -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.
Show 39 quoted lines
> 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
> >