Re: [PATCH v2] Teach git apply to respect core.fileMode settings

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 19, 2023, 22:21 UTC
Message-ID: <xmqqv88tal53.fsf@gitster.g>
In-Reply-To: <pull.1620.v2.git.1703010646036.gitgitgadget@gmail.com>

"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 15 quoted lines

> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> When applying a patch that adds an executable file, git apply
> ignores the core.fileMode setting (core.fileMode in git config
> specifies whether the executable bit on files in the working tree
> should be honored or not) resulting in warnings like:
>
> warning: script.sh has type 100644, expected 100755
>
> even when core.fileMode is set to false, which is undesired. This
> is extra true for systems like Windows, which don't rely on "lsat()".
>
> Fix this by inferring the correct file mode from the existing
> index entry when core.filemode is set to false. The added
> test case helps verify the change and prevents future regression.

Thanks. Has _this_ particular iteration of the patch been reviewed by Dscho? Otherwise ...

> Reviewed-by: Johannes Schindelin <johannes.schindelin@gmail.com>

... this line is a bit problematic.  Just double-checking.

> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---

Show 15 quoted lines

> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
> index e7a7295f1b6..73fc472b246 100755
> --- a/t/t4129-apply-samemode.sh
> +++ b/t/t4129-apply-samemode.sh
> @@ -101,4 +101,29 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
>  	)
>  '
>  
> +test_expect_success 'ensure git apply respects core.fileMode' '
> +	test_config core.fileMode false &&
> +	echo true >script.sh &&
> +	git add --chmod=+x script.sh &&
> +	git ls-files -s script.sh | grep "^100755" &&
> +	test_tick && git commit -m "Add script" &&
> +	git ls-tree -r HEAD script.sh | grep "^100755" &&

I am wondering if we want to be more strict about hiding error return code from "git ls-files" and "git ls-tree" behind pipes like these. Usually we encourage using a temporary file, e.g.,

	...
	git ls-files -s script.sh >ls-files-output &&
	test_grep "^100755" ls-files-output &&
	...

> +	echo true >>script.sh &&
> +	test_tick && git commit -m "Modify script" script.sh &&
> +	git format-patch -1 --stdout >patch &&
> +	grep "index.*100755" patch &&

Anchor the pattern when you know where it has to appear and what the surrounding letters must be, e.g.,

	test_grep "^index .* 100755$" patch &&

A test that expects a match with "grep" is silent when it fails, but if you use test_grep, the output from "sh t4129-*.sh -i -v" gets easier to view when the test fails, as it is more verbose and say "we wanted to see X in the output but your output does not have it".

> +	git switch -c branch HEAD^ &&
> +	git apply --index patch 2>err &&
> +	! grep "has type 100644, expected 100755" err &&

If you wanted to use test_grep here, the way to negate it is a bit peculiar, i.e.

	test_grep ! "has type ..." err &&

It does not have as much value as the positive case, as "! grep" that expects to fail would show the unexpected match. In any case, making sure this particular error message does not appear in the output is a good way to test it, instead of insisting that the output is empty, since we may add output to the standard error stream for unrelated reasons to issue warnings, etc.

> +	git restore -S script.sh && git restore script.sh &&

Why not "git reset --hard" here? Just being curious why we want to waste two invocations of "git restore".

Show 10 quoted lines

> +	git apply patch 2>err &&
> +	! grep "has type 100644, expected 100755" err &&
> +
> +	git apply --cached patch 2>err &&
> +	! grep "has type 100644, expected 100755" err
> +'
> +
>  test_done
>
> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996