[PATCH tg/add-chmod+x-fix 1/2] t3700-add: create subdirectory gently

[PATCH tg/add-chmod+x-fix 1/2] t3700-add: create subdirectory gently

[Johannes Sixt]

The subdirectory 'sub' is created early in the test file. Later, a test
case removes it during its clean-up actions. However, this test case is
protected by POSIXPERM. Consequently, 'sub' remains when the POSIXPERM
prerequisite is not satisfied. Later, a recently introduced test case
creates 'sub' again. Use -p with mkdir so that it does not fail if 'sub'
already exists.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t3700-add.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 0a962a6..16ab2da 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -380,7 +380,7 @@ test_expect_success 'no file status change if no pathspec is given' '
 '
 
 test_expect_success 'no file status change if no pathspec is given in subdir' '
-	mkdir sub &&
+	mkdir -p sub &&
 	(
 		cd sub &&
 		>sub-foo1 &&
-- 
2.10.0.85.gea34e30

[PATCH tg/add-chmod+x-fix 2/2] t3700-add: protect one --chmod=+x test with POSIXPERM

[Johannes Sixt]

A recently introduced test checks the result of 'git status' after
setting the executable bit on a file. This check does not yield the
expected result when the filesystem does not support the executable bit
(and core.filemode is false). Skip the test case.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 I am surprised that add --chmod=+x changes only the index, but not
 the file on disk!?!

 t/t3700-add.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 16ab2da..13e0dd2 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -361,7 +361,7 @@ test_expect_success 'git add --chmod=[+-]x changes index with already added file
 	test_mode_in_index 100644 xfoo3
 '
 
-test_expect_success 'file status is changed after git add --chmod=+x' '
+test_expect_success POSIXPERM 'file status is changed after git add --chmod=+x' '
 	echo "AM foo4" >expected &&
 	echo foo >foo4 &&
 	git add foo4 &&
-- 
2.10.0.85.gea34e30

Re: [PATCH tg/add-chmod+x-fix 2/2] t3700-add: protect one --chmod=+x test with POSIXPERM

[Thomas Gummerer]

Hi Johannes,

On 09/20, Johannes Sixt wrote:
> A recently introduced test checks the result of 'git status' after
> setting the executable bit on a file. This check does not yield the
> expected result when the filesystem does not support the executable bit
> (and core.filemode is false). Skip the test case.

Thanks for cleaning up my mess.  The patch looks correct to me.

> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  I am surprised that add --chmod=+x changes only the index, but not
>  the file on disk!?!

I *think* --chmod is mainly thought of as a convenience for git users
on a filesystem that doesn't have an executable flag.  So it was
introduced this way as the permissions on the file system don't matter
in that case.  A change of that behaviour may make sense for this
though.

>  t/t3700-add.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index 16ab2da..13e0dd2 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -361,7 +361,7 @@ test_expect_success 'git add --chmod=[+-]x changes index with already added file
>  	test_mode_in_index 100644 xfoo3
>  '
>  
> -test_expect_success 'file status is changed after git add --chmod=+x' '
> +test_expect_success POSIXPERM 'file status is changed after git add --chmod=+x' '
>  	echo "AM foo4" >expected &&
>  	echo foo >foo4 &&
>  	git add foo4 &&
> -- 
> 2.10.0.85.gea34e30
> 

-- 
Thomas

Re: [PATCH tg/add-chmod+x-fix 2/2] t3700-add: protect one --chmod=+x test with POSIXPERM

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

>>  I am surprised that add --chmod=+x changes only the index, but not
>>  the file on disk!?!
>
> I *think* --chmod is mainly thought of as a convenience for git users
> on a filesystem that doesn't have an executable flag.  So it was
> introduced this way as the permissions on the file system don't matter
> in that case.  A change of that behaviour may make sense for this
> though.

Perhaps we shouldn't even test this, then?  I can see future people
wanting to change this behaviour, while I can see argument for not
touching the working tree file, too.  It is essential for the main
purpose of the command (i.e. "I want to flip the executable bit for
the path in the index") to make sure that the bit in the index is
changed.  Comparing the index with the working tree using "status"
is probably not how you would want to do so.  A future breakage may
cause the indexed blob name to change by mistake, and status would
happily report difference but you would not notice its output is
saying "Hey, they are different between the index and the working
tree", while you are expecting ONLY the change in the executable bit.

How about doing

	git add foo4 &&
        git add --chmod=+x foo4 &&
	test_mode_in_index 100755 foo4

instead?

>
>>  t/t3700-add.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
>> index 16ab2da..13e0dd2 100755
>> --- a/t/t3700-add.sh
>> +++ b/t/t3700-add.sh
>> @@ -361,7 +361,7 @@ test_expect_success 'git add --chmod=[+-]x changes index with already added file
>>  	test_mode_in_index 100644 xfoo3
>>  '
>>  
>> -test_expect_success 'file status is changed after git add --chmod=+x' '
>> +test_expect_success POSIXPERM 'file status is changed after git add --chmod=+x' '
>>  	echo "AM foo4" >expected &&
>>  	echo foo >foo4 &&
>>  	git add foo4 &&
>> -- 
>> 2.10.0.85.gea34e30
>> 

Re: [PATCH tg/add-chmod+x-fix 2/2] t3700-add: protect one --chmod=+x test with POSIXPERM

[Johannes Sixt]

Am 21.09.2016 um 20:12 schrieb Junio C Hamano:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
>>>  I am surprised that add --chmod=+x changes only the index, but not
>>>  the file on disk!?!
>>
>> I *think* --chmod is mainly thought of as a convenience for git users
>> on a filesystem that doesn't have an executable flag.  So it was
>> introduced this way as the permissions on the file system don't matter
>> in that case.

OK.

>>  A change of that behaviour may make sense for this
>> though.

Hm, git-add is for moving content from the worktree to the index. I 
don't think it should change the worktree. I should not have been surprised.

It should probably not even introduce a change in the index that it does 
not see in the worktree, but, hey, git-add is a reasonable porcelain 
substitute for the --chmod task that otherwise git-update-index would 
have to do.

> Perhaps we shouldn't even test this, then?

If I am right that git-add should not change the worktree, it should be 
tested. But 'git status -s' is probably the wrong tool for the reasons 
you gave (it could accidentally detect a change due to content 
difference instead of a file mode difference). At any rate, such a test 
must be protected with POSIXPERM.

-- Hannes

Re: [PATCH tg/add-chmod+x-fix 2/2] t3700-add: protect one --chmod=+x test with POSIXPERM

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ...  Comparing the index with the working tree using "status"
> is probably not how you would want to do so.  A future breakage may
> cause the indexed blob name to change by mistake, and status would
> happily report difference but you would not notice its output is
> saying "Hey, they are different between the index and the working
> tree", while you are expecting ONLY the change in the executable bit.

In other words, how about doing it this way?

-- >8 --
From: Johannes Sixt <j6t@kdbg.org>
Date: Tue, 20 Sep 2016 08:18:25 +0200
Subject: [PATCH] t3700-add: do not rely on executable bit of the working tree file

A recently introduced test checks the result of 'git status' after
setting the executable bit on a file. This check does not yield the
expected result when the filesystem does not support the executable
bit.

The primary thing we care about is that a file added with --chmod=+x
has executable bit in the index, not that it is registered in the
index somehow differently from what is in the working tree, which
is what the "status" output tries to catch.  Let's check what we
care about, i.e. the path is marked as an executable in the index.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3700-add.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 16ab2da..1a733bb 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -361,13 +361,11 @@ test_expect_success 'git add --chmod=[+-]x changes index with already added file
 	test_mode_in_index 100644 xfoo3
 '
 
-test_expect_success 'file status is changed after git add --chmod=+x' '
-	echo "AM foo4" >expected &&
+test_expect_success 'git add --chmod=[+-]x changes index with newly added file' '
 	echo foo >foo4 &&
 	git add foo4 &&
 	git add --chmod=+x foo4 &&
-	git status -s foo4 >actual &&
-	test_cmp expected actual
+	test_mode_in_index 100755 foo4
 '
 
 test_expect_success 'no file status change if no pathspec is given' '
-- 
2.10.0-515-g9036219

Re: [PATCH tg/add-chmod+x-fix 2/2] t3700-add: protect one --chmod=+x test with POSIXPERM

[Johannes Sixt]

Am 21.09.2016 um 22:47 schrieb Junio C Hamano:
> -test_expect_success 'file status is changed after git add --chmod=+x' '
> -	echo "AM foo4" >expected &&
> +test_expect_success 'git add --chmod=[+-]x changes index with newly added file' '
>  	echo foo >foo4 &&
>  	git add foo4 &&
>  	git add --chmod=+x foo4 &&
> -	git status -s foo4 >actual &&
> -	test_cmp expected actual
> +	test_mode_in_index 100755 foo4
>  '

No, that's redundant. There are plenty of other test cases that check 
for this. You could just remove the case.

But I came to a different conclusion as I said in a message that crossed 
yours. I hope Thomas can pick up the baton again.

-- Hannes

Re: [PATCH tg/add-chmod+x-fix 2/2] t3700-add: protect one --chmod=+x test with POSIXPERM

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> But I came to a different conclusion as I said in a message that
> crossed yours. I hope Thomas can pick up the baton again.

Yeah, our mails crossed, apparently, and I do agree with your
reasoning.  How about this, then?

-- >8 -- 
From: Johannes Sixt <j6t@kdbg.org>
Date: Tue, 20 Sep 2016 08:18:25 +0200
Subject: [PATCH] t3700-add: do not check working tree file mode without POSIXPERM

A recently introduced test checks the result of 'git status' after
setting the executable bit on a file. This check does not yield the
expected result when the filesystem does not support the executable
bit.

What we care about is that a file added with "--chmod=+x" has
executable bit in the index and that "--chmod=+x" (or any other
options for that matter) does not muck with working tree files.
The former is tested by other existing tests, so let's check the
latter more explicitly and only under POSIXPERM prerequisite.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3700-add.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 16ab2da..924a266 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -361,13 +361,11 @@ test_expect_success 'git add --chmod=[+-]x changes index with already added file
 	test_mode_in_index 100644 xfoo3
 '
 
-test_expect_success 'file status is changed after git add --chmod=+x' '
-	echo "AM foo4" >expected &&
+test_expect_success POSIXPERM 'git add --chmod=[+-]x does not change the working tree' '
 	echo foo >foo4 &&
 	git add foo4 &&
 	git add --chmod=+x foo4 &&
-	git status -s foo4 >actual &&
-	test_cmp expected actual
+	! test -x foo4
 '
 
 test_expect_success 'no file status change if no pathspec is given' '
-- 
2.10.0-515-g9036219

Re: [PATCH tg/add-chmod+x-fix 2/2] t3700-add: protect one --chmod=+x test with POSIXPERM

[Johannes Sixt]

Am 21.09.2016 um 23:12 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> But I came to a different conclusion as I said in a message that
>> crossed yours. I hope Thomas can pick up the baton again.
>
> Yeah, our mails crossed, apparently, and I do agree with your
> reasoning.  How about this, then?
>
> -- >8 --
> From: Johannes Sixt <j6t@kdbg.org>
> Date: Tue, 20 Sep 2016 08:18:25 +0200
> Subject: [PATCH] t3700-add: do not check working tree file mode without POSIXPERM
>
> A recently introduced test checks the result of 'git status' after
> setting the executable bit on a file. This check does not yield the
> expected result when the filesystem does not support the executable
> bit.
>
> What we care about is that a file added with "--chmod=+x" has
> executable bit in the index and that "--chmod=+x" (or any other
> options for that matter) does not muck with working tree files.
> The former is tested by other existing tests, so let's check the
> latter more explicitly and only under POSIXPERM prerequisite.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t3700-add.sh | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index 16ab2da..924a266 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -361,13 +361,11 @@ test_expect_success 'git add --chmod=[+-]x changes index with already added file
>  	test_mode_in_index 100644 xfoo3
>  '
>
> -test_expect_success 'file status is changed after git add --chmod=+x' '
> -	echo "AM foo4" >expected &&
> +test_expect_success POSIXPERM 'git add --chmod=[+-]x does not change the working tree' '
>  	echo foo >foo4 &&
>  	git add foo4 &&
>  	git add --chmod=+x foo4 &&
> -	git status -s foo4 >actual &&
> -	test_cmp expected actual
> +	! test -x foo4
>  '
>
>  test_expect_success 'no file status change if no pathspec is given' '
>

That makes a lot of sense. Thank you so much!

-- Hannes

Re: [PATCH tg/add-chmod+x-fix 2/2] t3700-add: protect one --chmod=+x test with POSIXPERM

[Thomas Gummerer]

On 09/21, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > But I came to a different conclusion as I said in a message that
> > crossed yours. I hope Thomas can pick up the baton again.

Sorry for not getting back earlier, my git time is quite limited
unfortunately.

> Yeah, our mails crossed, apparently, and I do agree with your
> reasoning.  How about this, then?

Thanks, both the reasoning and the patch below make sense to me.

> -- >8 -- 
> From: Johannes Sixt <j6t@kdbg.org>
> Date: Tue, 20 Sep 2016 08:18:25 +0200
> Subject: [PATCH] t3700-add: do not check working tree file mode without POSIXPERM
> 
> A recently introduced test checks the result of 'git status' after
> setting the executable bit on a file. This check does not yield the
> expected result when the filesystem does not support the executable
> bit.
> 
> What we care about is that a file added with "--chmod=+x" has
> executable bit in the index and that "--chmod=+x" (or any other
> options for that matter) does not muck with working tree files.
> The former is tested by other existing tests, so let's check the
> latter more explicitly and only under POSIXPERM prerequisite.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t3700-add.sh | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index 16ab2da..924a266 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -361,13 +361,11 @@ test_expect_success 'git add --chmod=[+-]x changes index with already added file
>  	test_mode_in_index 100644 xfoo3
>  '
>  
> -test_expect_success 'file status is changed after git add --chmod=+x' '
> -	echo "AM foo4" >expected &&
> +test_expect_success POSIXPERM 'git add --chmod=[+-]x does not change the working tree' '
>  	echo foo >foo4 &&
>  	git add foo4 &&
>  	git add --chmod=+x foo4 &&
> -	git status -s foo4 >actual &&
> -	test_cmp expected actual
> +	! test -x foo4
>  '
>  
>  test_expect_success 'no file status change if no pathspec is given' '
> -- 
> 2.10.0-515-g9036219
> 

-- 
Thomas

Re: [PATCH tg/add-chmod+x-fix 1/2] t3700-add: create subdirectory gently

[Thomas Gummerer]

Hi Johannes,

On 09/20, Johannes Sixt wrote:
> The subdirectory 'sub' is created early in the test file. Later, a test
> case removes it during its clean-up actions. However, this test case is
> protected by POSIXPERM. Consequently, 'sub' remains when the POSIXPERM
> prerequisite is not satisfied. Later, a recently introduced test case
> creates 'sub' again. Use -p with mkdir so that it does not fail if 'sub'
> already exists.

Thanks for catching and fixing this.  Having a look at the tests shows
that this happens through a git reset --hard at the start of the tests
that require POSIXPERM, which makes me wonder whether we should do
such a cleanup somewhere unconditionally, or move the tests that do
require POSIXPERM to the end of the test file to possibly prevent
similar breakages in the future?

> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  t/t3700-add.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index 0a962a6..16ab2da 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -380,7 +380,7 @@ test_expect_success 'no file status change if no pathspec is given' '
>  '
>  
>  test_expect_success 'no file status change if no pathspec is given in subdir' '
> -	mkdir sub &&
> +	mkdir -p sub &&
>  	(
>  		cd sub &&
>  		>sub-foo1 &&
> -- 
> 2.10.0.85.gea34e30
> 

-- 
Thomas