Re: [PATCH] apply: set file mode when --reverse creates a deleted file

> On Fri, May 23, 2025, at 00:02, Mark Mentovai wrote:
>> Commit 01aff0a (apply: correctly reverse patch's pre- and post-image
>> mode bits; 2023-12-26) revised reverse_patches() to maintain the desired
>
> The way the commit is referred to is almost like the usual
> and recommended
>
>     git show -s --pretty=reference
>
> But with a semicolon instead of a comma.
>
>> property that when only one of patch::old_mode and patch::new_mode is
>> set, the mode will be carried in old_mode. That property is generally
>> correct, with one notable notable exception: when creating a file, only
>
> s/notable notable/notable/
>
>> new_mode will be set. Since reversing a deletion results in a creation,
>> new_mode must be set in that case.
>>
>> Omitting handling for this case meant that reversing a patch that
>> removed an executable file would not result in the executable permission
>> being set on the re-created file.
>>
>> When git apply --reverse is used, reverse_patches() will now additionaly
>> swap old_mode and new_mode for what's represented in the patch as a file
>> deletion, as it is transformed into a file creation under reversal.
>
> The usual way to refer to code behavior is to talk about the code
> without this patch/commit in the present tense.  I think this is talking
> about how the code behaves with this patch applied/with this commit.
>
> In my opinion it helps the narrative flow since something right-now is
> problematic.  Therefore (see next point) do this and that to fix the
> situation.
>
>> Tests are added that ensure that git apply sets file modes correctly on
>> file creation, both in the normal (forward) and reverse direction.
>> Existing test coverage for file modes focused only on mode changes of
>> existing files, and only in the forward direction.
>
> It’s recommended to describe changes as “do this and that” to the code.
> “Add tests”, not “added tests” or “tests are added” (and the latter here
> seems to use a passive construct that doesn’t feel in line with the
> preceding paragraphs).