Re: [PATCH tg/add-chmod+x-fix 2/2] t3700-add: protect one --chmod=+x test with POSIXPERM
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 > --- > 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
Thomas Gummerer 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
Junio C Hamano 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 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 Signed-off-by: Junio C Hamano --- 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
Am 21.09.2016 um 20:12 schrieb Junio C Hamano: Thomas Gummerer 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
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
Johannes Sixt 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 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 Signed-off-by: Junio C Hamano --- 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
Am 21.09.2016 um 23:12 schrieb Junio C Hamano: Johannes Sixt 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 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 Signed-off-by: Junio C Hamano --- 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
On 09/21, Junio C Hamano wrote: > Johannes Sixt 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 > 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 > Signed-off-by: Junio C Hamano > --- > 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
