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

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.

tboegi@web.de writes:

quoted

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.