From: Mark Mentovai <mark@chromium.org>
Date: Thu, 22 May 2025 23:48:05 GMT
Subject: Re: [PATCH] apply: set file mode when --reverse creates a deleted file
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.

>> +'
>> +
>> +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!