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

[Junio C Hamano <gitster@pobox.com>]

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

> 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>
> ---

> 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".

> +	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