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

["Torsten Bögershausen" <tboegi@web.de>]

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" ?

> where git sets core.precomposeUnicode to true automatically by git
> init/clone -- when a user creates a simple unicode refname (in NFC
> format) such as españa:
>
>   $ git branch españa
>
> different commands would display the branch name differently.  For
> example, git branch, git log --decorate, and git fast-export all used
>
>   65 73 70 61 c3 b1 61  (or "espa\xc3\xb1a")
>
> (NFC form) while show-ref would use
>
>   65 73 70 61 6e cc 83 61  (or "espan\xcc\x83a")
>
> (NFD form).  A stress test for git filter-repo was tripped up by this
> inconsistency, though digging in I found that the problems could
> compound; for example, if the user ran
>
>   $ git pack-refs --all
>
> and then tried to check out the branch, they would be met with:
>
>   $ git checkout españa
>   error: pathspec 'españa' did not match any file(s) known to git
>
>   $ git checkout españa --
>   fatal: invalid reference: españa
>
>   $ git branch
>     españa
>   * master
>
> Note that the user could run the `git branch` command first and copy and
> paste the `españa` portion of the output and still see the same two
> errors.  Also, if the user added --no-prune to the pack-refs command,
> then they would see three branches: master, españa, and españa (those
> last two are NFC vs. NFD forms, even if they render the same).
>
> Further, if the user had the `españa` branch checked out before
> running `git pack-refs --all`, the user would be greeted with (note
> that I'm trimming trailing output with an ellipsis):
>
>   $ git rev-parse HEAD
>   fatal: ambiguous argument 'HEAD': unknown revision or path...
>
>   $ git status
>   On branch españa
>
>   No commits yet...
>
> Or worse, if the user didn't check this stuff first, running `git
> commit` will create a new commit with all changes of all of history
> being squashed into it.
>
> In addition to pack-refs, one could also get into this state with
> upload-pack or anything that calls either pack-refs or upload-pack (e.g.
> gc or clone).
>
> 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.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  builtin/pack-refs.c | 2 ++
>  builtin/show-ref.c  | 3 +++
>  upload-pack.c       | 2 ++
>  3 files changed, 7 insertions(+)
>
> diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
> index f3353564f9..cfbd5c36c7 100644
> --- a/builtin/pack-refs.c
> +++ b/builtin/pack-refs.c
> @@ -1,4 +1,5 @@
>  #include "builtin.h"
> +#include "config.h"
>  #include "parse-options.h"
>  #include "refs.h"
>  #include "repository.h"
> @@ -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().

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
>