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

[Junio C Hamano <gitster@pobox.com>]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

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

Thanks for pointing these out.  

The phrasing convention was chosen not because it is superiour than
others (e.g., as long as we do so consistently, there is no logical
reason why describing the state of the codebase without the patch in
the past tense and the state after in the present tense is inferior
to how we describe the state without the patch in the present
tense), but to have _some_ usable standard way to express what the
status quo looks like, what the perceived problems are, and what is
done to improve the situation.  What you described is what we chose
to use, and it is good for all of us to stick to it.