From: Mark Mentovai Date: Sat, 24 May 2025 03:19:25 GMT Subject: Re: [PATCH v2 2/2] apply: set file mode when --reverse creates a deleted file Message-ID: In-Reply-To: Junio C Hamano wrote: > Mark Mentovai 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. >> +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 && > > 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.