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

From: Mark Mentovai <mark@chromium.org>
Date: May 22, 2025, 23:48 UTC
Message-ID: <76ce493d-d5bd-fc63-8942-76d0b3cebbf9@chromium.org>
In-Reply-To: <xmqqtt5c1ccs.fsf@gitster.g>

Junio C Hamano wrote:

> Mark Mentovai <mark@chromium.org> writes:

>> +     git checkout -- data.txt &&
>
> This should be a no-op, right?  What are we testing here?

This syncs the executable bit to the working tree.

I found it useful when developing the test, but it's probably not strictly necessary as the test is intentionally independent of the executable bit in the local filesystem. I can drop this if you think it's unnecessary.

Show 14 quoted lines

>> +'
>> +
>> +test_expect_success 'git apply --reverse restores file modes' '
>> +	test_config core.fileMode false &&
>> +	echo true >tool.sh &&
>
> I we took the above approach to prepare patches in separate files,
> we do not have to set up a different scenario completely anew.
> Instead, we can start from a state where data.txt is missing, and
> then reverse-apply the remove patch we used in the previous test
> first (and make sure the mode is without executable bit), then
> reverse-apply the chmod-x patch (and make sure the file is now
> executable), and then reverse-apply the creatoin patch (to ensure it
> is gone).

The file that I deleted at the end of the previous (apply "forward") test was not executable. It's important that this (apply --reverse) test begin by reversing a delete of an executable file. That was the reason I built up a second, separate set of patches to apply: I wanted to give better coverage to git-apply setting the executable bit when it creates a file, whether it's in the forward direction or reversing a deletion, because that's the harder case (and the one which wasn't working correctly, and which prompted this patch).

If you like that better, I could achieve this with yet another mode transition, but that would produce a stack of 4 patches that would need to be applied in each direction, or 12 operations in total (4 to create, 4 to apply forward, and 4 to apply in reverse). Or I can keep the existing structure, which is still 12 operations in total (3 to create, 3 to apply forward, 3 more to create, and finally 3 to apply in reverse). On the balance, I chose to keep the tests more isolated, but I'm happy to revise if that's what you prefer.

I'll also integrate the rest of your feedback, and Kristoffer's. Thanks!