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@xxxxxx 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.
