"Chandra Pratap via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Chandra Pratap <chandrapratap3519@xxxxxxxxx>
>
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.
> CC: Johannes Schindelin <johannes.schindelin@xxxxxxxxx>
> Signed-off-by: Chandra Pratap <chandrapratap3519@xxxxxxxxx>
> ---
> 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@xxxxxxxxx
> Signed-off-by: Chandra Pratap chandrapratap3519@xxxxxxxxx
The e-mail addresses somehow lost <angle brakets> around them.
> 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.
>
> 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.
> +'
> +
> test_done
>
> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
