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

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

Mark Mentovai <mark@chromium.org> writes:

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

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