mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <>
To: Elijah Newren <>
Cc: "Torsten Bögershausen" <>,
	"Git Mailing List" <>
Subject: Re: [PATCH] Honor core.precomposeUnicode in more places
Date: Wed, 24 Apr 2019 10:56:33 +0900	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <> (Elijah Newren's message of "Tue, 23 Apr 2019 12:06:45 -0700")

Elijah Newren <> 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.

  reply	other threads:[~2019-04-24  1:56 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-23 17:30 [PATCH] Honor core.precomposeUnicode in more places Elijah Newren
2019-04-23 18:29 ` Torsten Bögershausen
2019-04-23 19:06   ` Elijah Newren
2019-04-24  1:56     ` Junio C Hamano [this message]
2019-04-25 14:58 ` [PATCH v2] " Elijah Newren
2019-07-26 19:47 ` [PATCH] " Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).