Re: [PATCH v1 1/2] t0050: ls-files path fails if path of workdir is NFD

From: Junio C Hamano <gitster@pobox.com>
Date: May 7, 2024, 17:30 UTC
Message-ID: <xmqq1q6dpm1j.fsf@gitster.g>
In-Reply-To: <20240507084429.19781-1-tboegi@web.de>

tboegi@web.de writes:

Show 25 quoted lines

> From: Torsten Bögershausen <tboegi@web.de>
>
> Add a test case for this bug report, slightly edited and shortened:
>
> ls-files path' fails if absolute path of workdir contains NFD (macOS)
> On macOS, 'git ls-files path' does not work (gives an error)
> if the absolute 'path' contains characters in NFD (decomposed).
> I guess this is a (minor) bug of git.
>
> $ cd /somewhere         # some safe place, /tmp or ~/tmp etc.
> $ mkdir $'u\xcc\x88'    # ü in NFD
> $ cd ü                  # or cd $'u\xcc\x88' or cd $'\xc3\xbc'
> $ git init
> $ git ls-files $'/somewhere/u\xcc\x88'   # NFD
>   fatal: /somewhere/ü: '/somewhere/ü' is outside repository at '/somewhere/ü'
> $ git ls-files $'/somewhere/\xc3\xbc'    # NFC
> (the same error as above)
>
> In the 'fatal:' error message, there are three ü;
> the 1st and 2nd are in NFC, the 3rd is in NFD.
>
> The added test case here follows the error description,
> with the exception that the 'ü' is replaced by an 'ä',
> which we already have as NFD and NFC in t0050.
> A fix will be done in the next commit.

That will break bisection. I think combining the two commits into one would make sense for a small change like this, consisting a focused and straight-forward fix plus a clean and concise test.

Show 18 quoted lines

> Reported-by: Jun T <takimoto-j@kba.biglobe.ne.jp>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  t/t0050-filesystem.sh | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
> index 325eb1c3cd..bb85ec38cb 100755
> --- a/t/t0050-filesystem.sh
> +++ b/t/t0050-filesystem.sh
> @@ -156,4 +156,16 @@ test_expect_success CASE_INSENSITIVE_FS 'checkout with no pathspec and a case in
>  	)
>  '
>
> +test_expect_success 'git ls-files under NFD' '
> +	(
> +		mkdir somewhere &&
> +		mkdir somewhere/$aumlcdiar &&

Would a single "mkdir -p" suffice?

		mkdir -p "somewhere/$aumlcdiar" &&

> +		mypwd=$PWD &&
> +		cd somewhere/$aumlcdiar &&
> +		git init &&
> +		git ls-files "$mypwd/somewhere/$aumlcdiar"  2>err &&

We do not control what is in "$mypwd". Can it have funny characters that can confuse Git? Quoting the path with a pair of double quotes protects the shell from getting confused with $IFS whitespaces, but we may want to protect the pathspec handing in Git with something like

	git --literal-pathspecs ls-files "..."

here.

Show 7 quoted lines

> +		>expected &&
> +		test_cmp expected err
> +	)
> +'
>  test_done
> --
> 2.41.0.394.ge43f4fd0bd