Re: [PATCH v2 2/2] apply: set file mode when --reverse creates a deleted file
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 23, 2025, 23:44 UTC
- Message-ID
- <xmqqbjrivqn2.fsf@gitster.g>
- In-Reply-To
- <20250523172154.93810-3-mark@chromium.org>
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 ;-)
Show 49 quoted lines
> + test -n "$1" && test_grep "^10$1 " "$2" || test_must_be_empty "$2"
> +}
> +
> test_file_mode_staged () {
> git ls-files --stage -- "$2" >ls-files-output &&
> - test_grep "^10$1 " ls-files-output
> + test_file_mode_common "$1" ls-files-output
> }
>
> test_file_mode_HEAD () {
> git ls-tree HEAD -- "$2" >ls-tree-output &&
> - test_grep "^10$1 " ls-tree-output
> + test_file_mode_common "$1" ls-tree-output
> }
>
> test_expect_success 'git apply respects core.fileMode' '
> @@ -180,6 +184,150 @@ test_expect_success 'git apply warns about incorrect file modes' '
> test_file_mode_HEAD 0755 mode_test
> '
>
> +test_expect_success 'setup: git apply [--reverse] restores file modes (change_x_to_notx)' '
> + test_config core.fileMode false &&
> +
> + touch change_x_to_notx &&
> + git add --chmod=+x change_x_to_notx &&
> + test_file_mode_staged 0755 change_x_to_notx &&
> + test_tick && git commit -m "add change_x_to_notx as executable" &&
> + test_file_mode_HEAD 0755 change_x_to_notx &&
> +
> + git add --chmod=-x change_x_to_notx &&
> + test_file_mode_staged 0644 change_x_to_notx &&
> + test_tick && git commit -m "make change_x_to_notx not executable" &&
> + test_file_mode_HEAD 0644 change_x_to_notx &&
> +
> + git rm change_x_to_notx &&
> + test_file_mode_staged "" change_x_to_notx &&
> + test_tick && git commit -m "remove change_x_to_notx" &&
> + test_file_mode_HEAD "" change_x_to_notx &&
> +
> + git format-patch -o patches -3 &&
> + mv patches/0001-* change_x_to_notx-0001-create-0755.patch &&
> + mv patches/0002-* change_x_to_notx-0002-chmod-0644.patch &&
> + mv patches/0003-* change_x_to_notx-0003-delete.patch &&
> +
> + test_grep "^new file mode 100755$" change_x_to_notx-0001-create-0755.patch &&
> + test_grep "^old mode 100755$" change_x_to_notx-0002-chmod-0644.patch &&
> + test_grep "^new mode 100644$" change_x_to_notx-0002-chmod-0644.patch &&
> + test_grep "^deleted file mode 100644$" change_x_to_notx-0003-delete.patch
> +'OK.
> +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.
If this one failed, and if the tester is not running the test with "-i", the index and the working tree state left by failure of this step would cause the next test start in a state that it is not expecting. For example, if this one failed after applying change-x-to-notx but while applying change-x-to-notx again (perhaps it did not see the right message), the step to apply the deltion patch is skipped. The next test wants to reverse apply the deletion patch, which means it wants the path not to exist, but if this test fails in the middle, that is not guaranteed.
The same comment applies to later ones, as well.
Other than that, we seem to have a very good coverage of the combinations now. Thanks for a thorough work.