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

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2025, 14:49 UTC
Message-ID: <xmqq1psf1ixp.fsf@gitster.g>
In-Reply-To: <76ce493d-d5bd-fc63-8942-76d0b3cebbf9@chromium.org>

Mark Mentovai <mark@chromium.org> writes:

Show 8 quoted lines

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

Ah, OK, you are simulating a filesystem that is unaware of executable bit, so the bit on the file is ignored as long as the file is registered in the index, and the executable bit in the index is used instead. There is no need to sync, and it would truely be a no-op on a filesystem without executable hit, but when emulating with forced core.filemode on a filesystem with executable bit, doing so may make test writer feel better for whatever reason, since they can look at the "ls -l" output insteadof "ls-files -s" output to see if it is executable.

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

Yeah, I understand the intention. It is more like debugging printf left over from development of the test. It is misleading in the final product, though---makes it look as if the executable bit on the filesystem must match that of the index entry when running with core.filemode=off on executable-capable filesystems (to be honest, I am not sure how burdensome it is to the code base to support this mode of operation, where core.filemode lies to the system---but it is handy so let's keep the promise, "filesystem executable bits do not matter when core.filemode is off, even if you are on a filesystem that does store execuable bits", which the current code makes).

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

Yes, if we are trying to see how "removal of an executable" when applied in reverse turns into "addition of an executable", we need a sample patch that removes an executable.

But you can certainly flip the order in the first test to commit an unexecutable, turn it to executable, and then remove ;-)

In any case, for a better coverage, you'd probably want to do both, i.e. reverse apply a patch that removes an executable, another patch that removes a non-executable, yet another patch that adds an executable, and the fourth patch that adds a non-executable.

And the two tests that applies additions in reverse would probably need three states to start from (i.e. the file does not exist, the file does exist but with a wrong mode, the file exists with the right mode).

Show 5 quoted lines

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

Understood.

> On the balance, I chose to keep the tests more isolated,
> but I'm happy to revise if that's what you prefer.

I would prefer more smaller tests than fewer larger tests, so that a breakage can be identified more easily. &&- chaining 20 related tests that checks 20 different conditions is rather cumbersome--you need to find which step failed first.

Thanks.