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

From: Mark Mentovai <mark@chromium.org>
Date: May 24, 2025, 03:19 UTC
Message-ID: <cdca6dcb-3a78-fa0d-b58b-8dfd92924e96@chromium.org>
In-Reply-To: <xmqqbjrivqn2.fsf@gitster.g>

Junio C Hamano wrote:

Show 11 quoted lines

> Mark Mentovai <mark@chromium.org> writes:
>
>> +test_file_mode_common() {
>
> Unlike the two callers, this lacks SP before "()".
>
> I would have expected that "no such file" would be expressed with
> mode 000000 (taken from "git diff --raw" for a removal/creation
> patch), but an empty string works just as well.  The same comment
> about requiring 0-prefix before 644 and 755 applies here, but as
> long as it is done consistently, I wouldn't complain too loudly ;-)

No, it's fine. I'll switch to the 6-digit form.

>> +	test -n "$1" && test_grep "^10$1 " "$2" || test_must_be_empty "$2"

This requires attention anyway. Reading it back, it's buggy: if there's a mode in $1, but the file $2 is erroneously empty, test_grep will evaluate false and evaluation will advance to test_must_be_empty, which will cause the whole expression to evaluate true. This construct only works properly as an if-then-else when the "then" clause can't fail. I will revise this.

Show 10 quoted lines

>> +test_expect_success 'git apply restores file modes (change_x_to_notx)' '
>> +	test_config core.fileMode false &&
>
> Wouldn't this and the subsequent tests want to begin with
>
> 	git reset --hard <commit> &&
>
> to a known good state?  We expect that after successfully running
> this test piece, for example, the path is removed after the last
> patch that removes change-x-to-notx is applied.

Yes, good point. And with just slightly more effort, I can use the same strategy to eliminate the ordering dependency between the the tests in the 1/2 patch.

> Other than that, we seem to have a very good coverage of the
> combinations now.  Thanks for a thorough work.

Thanks for your thoughtful reviews. v3 incoming shortly.