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

From: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
Date: May 22, 2025, 22:24 UTC
Message-ID: <91c86145-1a78-465e-a08c-2fb727dc5a02@app.fastmail.com>
In-Reply-To: <20250522220235.8650-1-mark@chromium.org>

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/

Show 10 quoted lines

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

-- 
Kristoffer