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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 18, 2023, 18:04 UTC
Message-ID: <xmqqle9rfkvd.fsf@gitster.g>
In-Reply-To: <pull.1620.git.1702908568890.gitgitgadget@gmail.com>

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

> From: Chandra Pratap <chandrapratap3519@gmail.com>
>

This part goes in the final commit when the patch gets applied. Everything between the three-dash line and the patch (i.e., the first "diff --get" line) are discarded. Move what you wrote below here to make it the proposed log message for this patch.

Assuming that gets done, let's review what will become the proposed log message.

Show 14 quoted lines

> CC: Johannes Schindelin <johannes.schindelin@gmail.com>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
>     apply: make git apply respect core.fileMode settings
>     
>     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().

"lstat()" you mean.  Add "," between "Windows" and " which".

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

Perfect.

>     
>     Reviewed-by: Johannes Schindelin johannes.schindelin@gmail.com
>     Signed-off-by: Chandra Pratap chandrapratap3519@gmail.com

The e-mail addresses somehow lost <angle brakets> around them.

Show 15 quoted lines

> diff --git a/apply.c b/apply.c
> index 3d69fec836d..56790f515e0 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3778,8 +3778,11 @@ static int check_preimage(struct apply_state *state,
>  		return error_errno("%s", old_name);
>  	}
>  
> -	if (!state->cached && !previous)
> -		st_mode = ce_mode_from_stat(*ce, st->st_mode);
> +	if (!state->cached && !previous) {
> +		if (!trust_executable_bit && patch->old_mode)
> +			st_mode = patch->old_mode;
> +		else st_mode = ce_mode_from_stat(*ce, st->st_mode);
> +	}

Write the body of the "else" clause on a separate line.

More importantly, even though we know we cannot trust st->st_mode on such a filesystem (that is what !trust_executable_bit is about), once we have a cache entry in the in-core index, shouldn't we trust ce->ce_mode more than what the incoming patch says? Or is the executable bit of a cache-entry totally hosed on a platform with !trust_executable_bit?

I thought the way things should work was

 (1) "--chmod=+x", which you used in the test, should mark the added
     path executable in the index.  Writing that to a tree (by
     making a commit) should record script.sh as an executable
     (i.e., "git ls-tree -r" should show 100755 not 100644).

 (2) if you read such a tree, then the index will have the "correct"
     executable bit in the cache entry (i.e., "git ls-files -s"
     should show 100755 not 100644).

IOW, I am wondering if the above should look more like

	if (!state->cached && !previous) {
		if (!trust_executable_bit) {
			if (*ce)
				st_mode = (*ce)->ce_mode;
			else
				st_mode = patch->old_mode;
		} else {
			st_mode = ce_mode_from_stat(*ce, st->st_mode);
		}
	}

As setting patch->old_mode to st_mode is equivalent to saying "we blindly trust the data on the patch much more than what we know about the current repository state", which goes directly against what "check_preimage()" wants to achieve.

Show 13 quoted lines

>  
> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
> index e7a7295f1b6..95917fee128 100755
> --- a/t/t4129-apply-samemode.sh
> +++ b/t/t4129-apply-samemode.sh
> @@ -101,4 +101,19 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
>  	)
>  '
>  
> +test_expect_success FILEMODE 'ensure git apply respects core.fileMode' '
> +	test_config core.fileMode false &&
> +	echo true >script.sh &&
> +	git add --chmod=+x script.sh &&

Perhaps we would want to check with "git ls-files -s script.sh" what its mode bits are (hopefully it would be executable).

> +	test_tick && git commit -m "Add script" &&

Similarly, check with "git ls-tree -r HEAD script.sh" what its mode bits are?

> +	echo true >>script.sh &&
> +	test_tick && git commit -m "Modify script" script.sh &&

Ditto.

> +	git format-patch -1 --stdout >patch &&

Check that the patch expects script.sh to have its executable bit here, too?

> +	git switch -c branch HEAD^ &&
> +	git apply patch 2>err &&

We may also want to check "git apply --cached" and "git apply --index" here, not just the "poor-man's GNU patch emulation" mode.

> +	! test_grep "has type 100644, expected 100755" err

If you use test_grep, the correct negation is not like that, but

	test_grep ! "has type 100644, expected 100755" err

Giving a better diagnosis when the expectation is violated is the whole point of using "test_grep" not a vanilla "grep", so we need to tell it that we are reversing our expectations.

Thanks.

Show 5 quoted lines

> +'
> +
>  test_done
>
> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996