apply: make git apply respect core.fileMode settings #1620
Conversation
Welcome to GitGitGadgetHi @Chand-ra, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that your Pull Request has a good description, as it will be used as cover letter. You can CC potential reviewers by adding a footer to the PR description with the following syntax: Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
/allow |
|
User Chand-ra is now allowed to use GitGitGadget. WARNING: Chand-ra has no public email address set on GitHub; |
There was a problem hiding this comment.
@Chand-ra thank you for working on this!
It would probably make sense to refer to #1555 in the PR description (something like "This closes #1555").
Also, could you integrate the script I provided in #1555 (comment) into the patch so that a new test case runs every time the test suite runs and thereby avoids regressions? t/t4129-apply-samemode.sh strikes me as a natural home for this new test case.
I should note that the part setting up the repository won't be necessary in the t4129, as it is done automatically by the test suite framework. Further, you will want to turn |
|
@dscho thanks a lot for the informative review! I have made the changes you suggested, let me know if there is anything else that needs to be done before I submit this patch. |
Chand-ra
force-pushed
the
devel
branch
2 times, most recently
from
25ded4a
to
d896351
Compare
Chand-ra
changed the title
There was a problem hiding this comment.
Could you do a last touch-up of the commit message? I'd like you to turn the CC: into a Reviewed-by: and fill the body of the commit message with some background along the lines of https://github.blog/2022-06-30-write-better-commits-build-better-projects/: Explain intent (I guess the commit title does a good job of that already) and context (what does core.fileMode do and in what scenarios is that relevant?), then implementation (anything that's not obvious from the diff is a good thing to add here) and justification ("the warning is annoying and not helpful", maybe?).
|
@Chand-ra excellent, thank you for addressing my feedback! I left a few ideas how to make the commit message more compelling (which always helps patches on the Git mailing list to go through more smoothly, seeing as reviewers pay a lot of attention to the care put into commit messages), but apart from that, this patch is good to go from my side! |
|
/preview |
|
Preview email sent as pull.1620.git.1702906703811.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1620.git.1702908568890.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "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.
> 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.
> 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 |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Junio C Hamano <gitster@pobox.com> writes:
>> +test_expect_success FILEMODE 'ensure git apply respects core.fileMode' '
Forgot to point out the most important thing.
The code change in this patch is primarily about making the code
work better for folks without trustworthy filemode support.
Emulating what happens by setting core.fileMode to false on a
platform with capable filesystems may be a way to test the code, but
we should have a test specific to folks without FILEMODE
prerequisites and make sure it works well, no?
IOW, shouldn't we drom FILEMODE prerequisite from this test? How
does it break on say Windows if this test is added without FILEMODE
prerequisite?
|
|
On the Git mailing list, Chandra Pratap wrote (reply to this): Thanks for the review, Junio. I have considered your feedback and
will adjust the patch as such. The mail formatting issues seem to
have arisen from my invigilant use of GitGitGadget.
> Junio C Hamano <gitster@pobox.com> writes:
>
> 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);
> }
> }
You're right, we should prioritise the file mode info from the
existing cache entry (if one exists) instead of blindly assigning
the one from the incoming patch. It's more robust that way.
> Perhaps we would want to check with "git ls-files -s script.sh" what
> its mode bits are (hopefully it would be executable).
>
> Similarly, check with "git ls-tree -r HEAD script.sh" what its mode>
> bits are?
>
> Check that the patch expects script.sh to have its executable bit
> here, too?
I assume we're doing all this filemode checking to ensure that the
executable bit doesn't get lost due to any other git command?
> The code change in this patch is primarily about making the code
> work better for folks without trustworthy filemode support.
> Emulating what happens by setting core.fileMode to false on a
> platform with capable filesystems may be a way to test the code, but
> we should have a test specific to folks without FILEMODE
> prerequisites and make sure it works well, no?
>
> IOW, shouldn't we drom FILEMODE prerequisite from this test?
I will keep that in mind. |
|
User |
|
/preview |
|
Preview email sent as pull.1620.v2.git.1703009578723.gitgitgadget@gmail.com |
|
/preview |
|
Preview email sent as pull.1620.v2.git.1703009963932.gitgitgadget@gmail.com |
|
/preview |
|
This patch series was integrated into seen via git@b96ffa4. |
|
This patch series was integrated into seen via git@63fc4ad. |
|
There was a status update in the "Cooking" section about the branch "git apply" on a filesystem without filemode support have learned to take a hint from what is in the index for the path, even when not working with the "--index" or "--cached" option, when checking the executable bit match what is required by the preimage in the patch. Will cook in 'next'. cf. <xmqqzfwb53a9.fsf@gitster.g> source: <20231226233218.472054-1-gitster@pobox.com> |
|
This patch series was integrated into seen via git@3719a69. |
|
This patch series was integrated into seen via git@ac5ea45. |
|
This patch series was integrated into seen via git@3449aac. |
|
This patch series was integrated into seen via git@f4c06b9. |
|
There was a status update in the "Cooking" section about the branch "git apply" on a filesystem without filemode support have learned to take a hint from what is in the index for the path, even when not working with the "--index" or "--cached" option, when checking the executable bit match what is required by the preimage in the patch. Will cook in 'next'. cf. <xmqqzfwb53a9.fsf@gitster.g> source: <20231226233218.472054-1-gitster@pobox.com> |
|
This patch series was integrated into seen via git@044aaa6. |
|
This patch series was integrated into seen via git@0b5eb50. |
|
This patch series was integrated into seen via git@9a521d4. |
|
There was a status update in the "Cooking" section about the branch "git apply" on a filesystem without filemode support have learned to take a hint from what is in the index for the path, even when not working with the "--index" or "--cached" option, when checking the executable bit match what is required by the preimage in the patch. Will cook in 'next'. cf. <xmqqzfwb53a9.fsf@gitster.g> source: <20231226233218.472054-1-gitster@pobox.com> |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Junio,
On Wed, 7 Feb 2024, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Chandra Pratap noticed that "git apply" on a filesystem without
> > executable bit support gives a warning when applying a patch that
> > expects the preimage file to have executable bit on. Dscho noticed
> > that the initial fix by Chandra did not work well when applying a
> > patch in reverse. It turns out that apply.c:reverse_patches()
> > invalidates the "a patch that does not change mode bits have the
> > mode bits in .old_mode member and not in .new_mode member" invariant
> > we rely on.
> >
> > Here is the result of concerted effort.
> >
> > Chandra Pratap (1):
> > apply: ignore working tree filemode when !core.filemode
> >
> > Junio C Hamano (2):
> > apply: correctly reverse patch's pre- and post-image mode bits
> > apply: code simplification
> >
> > apply.c | 16 +++++++++++++---
> > t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++
> > 2 files changed, 40 insertions(+), 3 deletions(-)
>
> Anybody wants to offer a review on this? I actually am fairly
> confortable with these without any additional review, but since I am
> sweeping the "Needs review" topics in the What's cooking report, I
> thought I would ask for this one, too.
I just had a look over all three of the patches, and to me, they look good
to go.
Ciao,
Johannes |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > Chandra Pratap (1):
>> > apply: ignore working tree filemode when !core.filemode
>> >
>> > Junio C Hamano (2):
>> > apply: correctly reverse patch's pre- and post-image mode bits
>> > apply: code simplification
>> >
>> > apply.c | 16 +++++++++++++---
>> > t/t4129-apply-samemode.sh | 27 +++++++++++++++++++++++++++
>> > 2 files changed, 40 insertions(+), 3 deletions(-)
>>
>> Anybody wants to offer a review on this? I actually am fairly
>> confortable with these without any additional review, but since I am
>> sweeping the "Needs review" topics in the What's cooking report, I
>> thought I would ask for this one, too.
>
> I just had a look over all three of the patches, and to me, they look good
> to go.
Thanks. |
|
This patch series was integrated into seen via git@13b7d25. |
|
This patch series was integrated into seen via git@183b4bf. |
|
This patch series was integrated into seen via git@796ecfe. |
|
There was a status update in the "Cooking" section about the branch "git apply" on a filesystem without filemode support have learned to take a hint from what is in the index for the path, even when not working with the "--index" or "--cached" option, when checking the executable bit match what is required by the preimage in the patch. Will cook in 'next'. cf. <xmqqzfwb53a9.fsf@gitster.g> source: <20231226233218.472054-1-gitster@pobox.com> |
|
This patch series was integrated into seen via git@e2b6517. |
|
This patch series was integrated into seen via git@79f036a. |
|
There was a status update in the "Cooking" section about the branch "git apply" on a filesystem without filemode support have learned to take a hint from what is in the index for the path, even when not working with the "--index" or "--cached" option, when checking the executable bit match what is required by the preimage in the patch. Will cook in 'next'. cf. <xmqqzfwb53a9.fsf@gitster.g> source: <20231226233218.472054-1-gitster@pobox.com> |
|
This patch series was integrated into seen via git@0fcab17. |
|
This patch series was integrated into seen via git@f9e6dd3. |
|
This patch series was integrated into seen via git@cf47fb7. |
|
This patch series was integrated into master via git@cf47fb7. |
|
This patch series was integrated into next via git@cf47fb7. |
|
Closed via cf47fb7. |
Regarding this:
I have modified the patch so that the output of git ls-files and
git ls-tree are stored in a temporary file instead of being directly
piped to grep but also noticed similar working in other test cases
in the same test file. For example,
test_expect_success FILEMODE 'same mode (index only)' '
....
....
....
git ls-files -s file | grep "^100755"
and
test_expect_success FILEMODE 'mode update (index only)' '
...
...
...
git ls-files -s file | grep "^100755"
Would we want to modify these scripts as well so they follow the
same convention as above or is it okay to let them be as is?
cc: Torsten Bögershausen tboegi@web.de
cc: Johannes Schindelin Johannes.Schindelin@gmx.de