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

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

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

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