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

From: Chandra Pratap <chandrapratap3519@gmail.com>
Date: Dec 19, 2023, 17:07 UTC
Message-ID: <CA+J6zkQVcSqpi8yRirEJn6Vbe9WkvhtWE5_YRZVzq7bNrFoFPA@mail.gmail.com>
In-Reply-To: <xmqqbkanfkjy.fsf@gitster.g>

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.

Show 14 quoted lines

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

Show 8 quoted lines

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

Show 8 quoted lines

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