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