git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: tboegi@web.de
Cc: git@vger.kernel.org, random_n0body@icloud.com,
	levraiphilippeblain@gmail.com
Subject: Re: [PATCH v4 1/1] MacOS: precompose_argv_prefix()
Date: Wed, 03 Feb 2021 11:33:00 -0800
Message-ID: <xmqq35ydc5g3.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20210203162823.15756-1-tboegi@web.de> (tboegi@web.de's message of "Wed, 3 Feb 2021 17:28:23 +0100")

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> The following sequence leads to a "BUG" assertion running under MacOS:
>
>   DIR=git-test-restore-p
>   Adiarnfd=$(printf 'A\314\210')
>   DIRNAME=xx${Adiarnfd}yy
>   mkdir $DIR &&
>   cd $DIR &&
>   git init &&
>   mkdir $DIRNAME &&
>   cd $DIRNAME &&
>   echo "Initial" >file &&
>   git add file &&
>   echo "One more line" >>file &&
>   echo y | git restore -p .
>
>  Initialized empty Git repository in /tmp/git-test-restore-p/.git/
>  BUG: pathspec.c:495: error initializing pathspec_item
>  Cannot close git diff-index --cached --numstat
>  [snip]
>
> The command `git restore` is run from a directory inside a Git repo.
> Git needs to split the $CWD into 2 parts:
> The path to the repo and "the rest", if any.
> "The rest" becomes a "prefix" later used inside the pathspec code.
>
> As an example, "/path/to/repo/dir-inside-repå" would determine
> "/path/to/repo" as the root of the repo, the place where the
> configuration file .git/config is found.
>
> The rest becomes the prefix ("dir-inside-repå"), from where the
> pathspec machinery expands the ".", more about this later.
> If there is a decomposed form, (making the decomposing visible like this),
> "dir-inside-rep°a" doesn't match "dir-inside-repå".
>
> Git commands need to:
>
>  (a) read the configuration variable "core.precomposeunicode"
>  (b) precocompose argv[]
>  (c) precompose the prefix, if there was any
>
> The first commit,
> 76759c7dff53 "git on Mac OS and precomposed unicode"
> addressed (a) and (b).
>
> The call to precompose_argv() was added into parse-options.c,
> because that seemed to be a good place when the patch was written.
>
> Commands that don't use parse-options need to do (a) and (b) themselfs.
>
> The commands `diff-files`, `diff-index`, `diff-tree` and `diff`
> learned (a) and (b) in
> commit 90a78b83e0b8 "diff: run arguments through precompose_argv"
>
> Branch names (or refs in general) using decomposed code points
> resulting in decomposed file names had been fixed in
> commit 8e712ef6fc97 "Honor core.precomposeUnicode in more places"
>
> The bug report from above shows 2 things:
> - more commands need to handle precomposed unicode
> - (c) should be implemented for all commands using pathspecs
>
> Solution:
> precompose_argv() now handles the prefix (if needed), and is renamed into
> precompose_argv_prefix().
>
> Inside this function the config variable core.precomposeunicode is read
> into the global variable precomposed_unicode, as before.
> This reading is skipped if precomposed_unicode had been read before.
>
> The original patch for preocomposed unicode, 76759c7dff53, placed
> precompose_argv() into parse-options.c
> Now add it into git.c (as well) for commands that don't use parse-options.
> Note that precompose() may be called twice - it is idempotent.

Just as the prefix-less variant was idempotent and that was the
reason why cmd_diff_files() had its own precompose() even if the
incoming argv[] is supposed to be already precomposed because it
was processed in another call to precompose() in run_builtin(),
this patch keeps these seemingly redundant calls, because it is not
meant as a clean-up but as a bugfix for the prefix part.

OK. ... Ah, no, the call in run_builtin() is a new thing.  We didn't
have it, and the redundant call is what this patch introduced, so
we need to be a bit more careful about the analysis here.  It is one
thing to say "we leave the existing iffy code and address only a
single bug" and do so.  It is entirely different to say so and then
do "we introduce an iffy code and address only a single bug".  We
need to admit that what we added _is_ iffy but supposed to be safe.
Just saying "it is supposed to be safe" without saying why it is
iffy is dishonest and does not help future developers who may want
to jump in and clean the code.

Perhaps

	Now add it into git.c::run_builtin() as well.  Existing
	precompose calls in diff-files.c and others would become
	redundant but because we do not want to make sure that there
	is no way for the control to reach them without passing
	run_builtin(), we'll keep them in place just in case.  The
	calls to precompose() are idempotent so it should not hurt.

or something?

>  - And yes, there may be more work to be done. For the moment I would like to
>    get this merged into git.git, and address the rest later.

Sure.

Thanks.

  reply	other threads:[~2021-02-03 19:35 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-06 11:35 git-bugreport-2021-01-06-1209.txt (git can't deal with special characters) Daniel Troger
2021-01-06 14:21 ` Torsten Bögershausen
2021-01-06 16:49   ` Daniel Troger
2021-01-06 21:47     ` Torsten Bögershausen
2021-01-06 22:21       ` Daniel Troger
2021-01-06 23:07         ` Randall S. Becker
2021-01-07 14:34           ` Philippe Blain
2021-01-07 15:49             ` Torsten Bögershausen
2021-01-07 16:21               ` Philippe Blain
2021-01-08 19:07                 ` Torsten Bögershausen
2021-01-24 15:13 ` [PATCH/RFC v1 1/1] git restore -p . and precomposed unicode tboegi
2021-01-24 19:51   ` Junio C Hamano
2021-01-25 16:53     ` Torsten Bögershausen
2021-01-29 17:15 ` [PATCH v2 1/1] MacOS: precompose_argv_prefix() tboegi
2021-01-29 23:19   ` Junio C Hamano
2021-01-31  0:43   ` Junio C Hamano
2021-01-31  9:50     ` Torsten =?unknown-8bit?Q?B=C3=B6gershausen?=
2021-02-02 15:11 ` [PATCH v3 " tboegi
2021-02-02 17:43   ` Junio C Hamano
2021-02-03 16:28 ` [PATCH v4 " tboegi
2021-02-03 19:33   ` Junio C Hamano [this message]
2021-02-03 22:13     ` Junio C Hamano
2021-02-05 17:31       ` Torsten Bögershausen

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=xmqq35ydc5g3.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=levraiphilippeblain@gmail.com \
    --cc=random_n0body@icloud.com \
    --cc=tboegi@web.de \
    --subject='Re: [PATCH v4 1/1] MacOS: precompose_argv_prefix()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git