Re: [PATCH/RFC v1 1/1] git restore -p . and precomposed unicode

From: Torsten Bögershausen <tboegi@web.de>
Date: Jan 25, 2021, 16:53 UTC
Message-ID: <20210125165307.2qkmbtixzih532ay@tb-raspi4>
In-Reply-To: <xmqqsg6qyuyi.fsf@gitster.c.googlers.com>

On Sun, Jan 24, 2021 at 11:51:33AM -0800, Junio C Hamano wrote:

Thanks for the fast review, much appreciated.

The short answer: There is more to be done, V2 is coming somewhen. For the longer story, please see inline.

Show 26 quoted lines

> tboegi@web.de writes:
>
> > The solution is to read the config variable "core.precomposeunicode" early.
>
> For a single command like "restore", we need to fail if it is run
> outside a repository anyway, so it is OK, but code in "git.c" in
> general can be called outside a repository, we may not know where
> our configuration files are, though.  So I'd prefer to avoid
> adding too many "do this thing early for every single command".
>
> Where do we normally read that variable?  I see calls to
> precompose_argv() on only a few codepaths
>
> builtin/diff-files.c:38:	precompose_argv(argc, argv);
> builtin/diff-index.c:28:	precompose_argv(argc, argv);
> builtin/diff-tree.c:129:	precompose_argv(argc, argv);
> builtin/diff.c:455:	precompose_argv(argc, argv);
> builtin/submodule--helper.c:1260:	precompose_argv(diff_args.nr, diff_args.v);
> parse-options.c:872:	precompose_argv(argc, argv);
>
> I guess the reason we can get away with it is because most of the
> newer commands use the parse_options() API, and the call to
> precompose_argv() is used by the codepaths implementing these
> commands.  And as a rule, these commands read config first before
> calling parse_options(), so by the time the control reaches there,
> the value of the variable may be already known.

Yes.

>
> The question is why "restore -p" is so special?  Or does this patch
> mean everybody, even the ones that use parse_options() is broken?

One special thing is that it uses pathspec, `git restore -p .` and $CWD is inside a decomposed directory.

There is more work to be done, as it seems. Running `git status . ` in an ASCII based repo (ignore the debug prints)

  user@mac:/tmp/210125-precomp/A.dir>  git status .
  git.c/handle_builtin:699 cmd="status" len=6
  git.c:428 prefix="A.dir/" (6)
  git.c:434 prec_pfx="(null)" (4294967295)
  builtin/commit.c:1401 prefix="A.dir/" (6)
  On branch master
  Changes not staged for commit:
    (use "git add <file>..." to update what will be committed)
      (use "git restore <file>..." to discard changes in working directory)
              modified:   O.file

-------------------------
But doing the same test within a decomosed directory:
  user@mac:/tmp/210125-decomp/Ä.dir> git status .
  git.c/handle_builtin:699 cmd="status" len=6
  git.c:428 prefix="Ä.dir/" (8)
  git.c:434 prec_pfx="Ä.dir/" (7)
  builtin/commit.c:1401 prefix="Ä.dir/" (7)
  On branch master
  nothing to commit, working tree clean
---------
But using ".." as the pathspec works:

  user@mac:/tmp/210125-decomp/Ä.dir> git status ..
  git.c/handle_builtin:699 cmd="status" len=6
  git.c:428 prefix="Ä.dir/" (8)
  git.c:434 prec_pfx="Ä.dir/" (7)
  builtin/commit.c:1401 prefix="Ä.dir/" (7)
  On branch master
  Changes not staged for commit:
    (use "git add <file>..." to update what will be committed)
      (use "git restore <file>..." to discard changes in working directory)
              modified:   "../A\314\210.dir/\303\226.file"

  no changes added to commit (use "git add" and/or "git commit -a")
--------------

So in that sense, it seems as if `git restore` is special because the patch helps.

More patches are needed asseen above.

And the rest of the suggestions makes all sense.