# t3701 fails if core.filemode disabled 15 messages from 2008-05-18 to 2008-05-22. Participants: Alex Riesen, Johannes Schindelin, Jeff King, Mark Levedahl, Junio C Hamano. Thread: https://gitlist.dev/t/13567 ## Alex Riesen, 2008-05-18 15:23 Subject: t3701 fails if core.filemode disabled Message-ID: <20080518152337.GB3058@steel.home> URL: https://gitlist.dev/e/20080518152337.GB3058%40steel.home ``` Signed-off-by: Alex Riesen --- This is on Cygwin, yes. I have the core.filemode disabled in ~/.gitconfig. How about stopping the test before the failing portion (only the last two fail, below)? $ ./t3701-add-interactive.sh -d -v ... * expecting success: git reset --hard && echo content >>file && chmod +x file && printf "n\\ny\\n" | git add -p && git show :file | grep content && git diff file | grep "new mode" HEAD is now at ddca8ca commit ddca8caf8c92841fca6311cf3ee839dd304f353d -- diff --git a/file b/file index 180b47c..b6f2c08 100644 --- a/file +++ b/file @@ -1 +1,2 @@ baseline +content Stage this hunk [y/n/a/d/?]? * FAIL 9: patch does not affect mode git reset --hard && echo content >>file && chmod +x file && printf "n\\ny\\n" | git add -p && git show :file | grep content && git diff file | grep "new mode" * expecting success: git reset --hard && echo content >>file && chmod +x file && printf "y\\nn\\n" | git add -p && git diff --cached file | grep "new mode" && git diff file | grep "+content" HEAD is now at ddca8ca commit ddca8caf8c92841fca6311cf3ee839dd304f353d -- diff --git a/file b/file index 180b47c..b6f2c08 100644 --- a/file +++ b/file @@ -1 +1,2 @@ baseline +content Stage this hunk [y/n/a/d/?]? * FAIL 10: stage mode but not hunk git reset --hard && echo content >>file && chmod +x file && printf "y\\nn\\n" | git add -p && git diff --cached file | grep "new mode" && git diff file | grep "+content" * failed 2 among 10 test(s) $ t/t3701-add-interactive.sh | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index f15be93..976ef72 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -66,6 +66,8 @@ test_expect_success 'revert works (commit)' ' grep "unchanged *+3/-0 file" output ' +test "$(git config core.filemode)" = false && test_done + test_expect_success 'patch does not affect mode' ' git reset --hard && echo content >>file && -- 1.5.5.1.354.g902c ``` ## Johannes Schindelin, 2008-05-18 15:35 Subject: Re: t3701 fails if core.filemode disabled Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0805181634100.30431%40racer In-Reply-To: <20080518152337.GB3058@steel.home> ``` Hi, On Sun, 18 May 2008, Alex Riesen wrote: > Signed-off-by: Alex Riesen Are you sure that you want the commit message to read "t3701 fails if core.filemode disabled"? Not rather "skip t3701 if core.filemode is disabled"? > --- > > This is on Cygwin, yes. I have the core.filemode disabled in > ~/.gitconfig. How about stopping the test before the failing portion > (only the last two fail, below)? > > $ ./t3701-add-interactive.sh -d -v > ... > * expecting success: > git reset --hard && > echo content >>file && > chmod +x file && > printf "n\\ny\\n" | git add -p && > git show :file | grep content && > git diff file | grep "new mode" > > HEAD is now at ddca8ca commit > ddca8caf8c92841fca6311cf3ee839dd304f353d > -- > diff --git a/file b/file > index 180b47c..b6f2c08 100644 > --- a/file > +++ b/file > @@ -1 +1,2 @@ Are you really, really, really sure you want to have that in the commit? After all, it is _after_ the "---", and the line _does_ begin with "diff --git". Ciao, Dscho ``` ## Alex Riesen, 2008-05-18 17:04 Subject: Re: t3701 fails if core.filemode disabled Message-ID: <81b0412b0805181004s45de5ba1v328a8753ecaca42@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0805181004s45de5ba1v328a8753ecaca42%40mail.gmail.com In-Reply-To: ``` 2008/5/18 Johannes Schindelin : > On Sun, 18 May 2008, Alex Riesen wrote: >> Signed-off-by: Alex Riesen > > Are you sure that you want the commit message to read "t3701 fails if > core.filemode disabled"? Not rather "skip t3701 if core.filemode is > disabled"? It is not meant for inclusion. More an RFC. I'll resubmit, of course, if just skipping the two last tests is ok (maybe git-add--interactive can be changed so that it handles this filemode problem internally). ``` ## Jeff King, 2008-05-18 19:08 Subject: Re: t3701 fails if core.filemode disabled Message-ID: <20080518190839.GC15506@sigill.intra.peff.net> URL: https://gitlist.dev/e/20080518190839.GC15506%40sigill.intra.peff.net In-Reply-To: <20080518152337.GB3058@steel.home> ``` On Sun, May 18, 2008 at 05:23:37PM +0200, Alex Riesen wrote: > This is on Cygwin, yes. I have the core.filemode disabled in > ~/.gitconfig. How about stopping the test before the failing portion > (only the last two fail, below)? What's in your ~/.gitconfig shouldn't have any effect (the test scripts take care to avoid looking at anything outside of your git directory). But presumably this test is broken on Cygwin, anyway? I don't mind disabling these tests if they don't make sense on certain platforms, but regarding your specific proposal: - can you confirm that the test doesn't make sense, and not that it is simply broken on cygwin? Does changing your ~/.gitconfig's core.filemode make a difference? It shouldn't, but that could be a bug in test-lib. What happens if you run the test manually? Does git-add just not prompt for the mode change? - if the tests are to be disabled, I think it is better to if tests_make_sense; do tests fi rather than exiting the script. It is less error prone if tests get added later. - What is the right tests_make_sense? You are checking core.filemode, but that should not be leaking in from your .gitconfig. Does cygwin have a different defaults for that value? Is it actually a matter of being on a filesystem which doesn't properly handle the executable bit? -Peff ``` ## Alex Riesen, 2008-05-18 20:01 Subject: Re: t3701 fails if core.filemode disabled Message-ID: <20080518200121.GA5789@steel.home> URL: https://gitlist.dev/e/20080518200121.GA5789%40steel.home In-Reply-To: <20080518190839.GC15506@sigill.intra.peff.net> ``` Jeff King, Sun, May 18, 2008 21:08:39 +0200: > On Sun, May 18, 2008 at 05:23:37PM +0200, Alex Riesen wrote: > > > This is on Cygwin, yes. I have the core.filemode disabled in > > ~/.gitconfig. How about stopping the test before the failing portion > > (only the last two fail, below)? > > What's in your ~/.gitconfig shouldn't have any effect (the test scripts > take care to avoid looking at anything outside of your git directory). > But presumably this test is broken on Cygwin, anyway? Correct. > I don't mind disabling these tests if they don't make sense on certain > platforms, but regarding your specific proposal: > > - can you confirm that the test doesn't make sense, and not that it is > simply broken on cygwin? Does changing your ~/.gitconfig's > core.filemode make a difference? It shouldn't, but that could be > a bug in test-lib. What happens if you run the test manually? Does > git-add just not prompt for the mode change? I setting core.filemode _inside_ the test breaks it in exactly the same way (on Linux, I'm at home). I'll retest tomorrow > - if the tests are to be disabled, I think it is better to > if tests_make_sense; do > tests > fi > rather than exiting the script. It is less error prone if tests get > added later. I agree > - What is the right tests_make_sense? You are checking core.filemode, > but that should not be leaking in from your .gitconfig. Does cygwin > have a different defaults for that value? Could be. > Is it actually a matter of being on a filesystem which doesn't > properly handle the executable bit? That - too. I shall see tomorrow ``` ## Mark Levedahl, 2008-05-19 00:17 Subject: Re: t3701 fails if core.filemode disabled Message-ID: <4830C71A.60308@verizon.net> URL: https://gitlist.dev/e/4830C71A.60308%40verizon.net In-Reply-To: <20080518190839.GC15506@sigill.intra.peff.net> ``` Jeff King wrote: > On Sun, May 18, 2008 at 05:23:37PM +0200, Alex Riesen wrote: > >> This is on Cygwin, yes. I have the core.filemode disabled in >> ~/.gitconfig. How about stopping the test before the failing portion >> (only the last two fail, below)? > > What's in your ~/.gitconfig shouldn't have any effect (the test scripts > take care to avoid looking at anything outside of your git directory). > But presumably this test is broken on Cygwin, anyway? > On Cygwin, support for correctly interpreting the execute bit is governed by the operating system (WindowsNT/XP vs Windows9x), the file system (NTFS vs FAT), and the (NO)NTEA and (NO)NTSEC options via the CYGWIN environment variable. So, there is no simple answer... By default on WindowsNT/XP and running on NTFS, execute modes are supported and the git testsuite currently works. Some other configurations will not support core.filemode=true, but Cygwin's stated goal is to emulate Linux and it does so by default using current common configurations. Thus, disabling the tests unconditionally on Cygwin would be wrong. Perhaps the better path is to skip the tests if "$(uname -o)" = "Cygwin" and core.filemode=false, but enable them otherwise, allowing users who know their configuration will not support the execute bit to avoid the test failure. Mark ``` ## Alex Riesen, 2008-05-19 06:01 Subject: Re: t3701 fails if core.filemode disabled Message-ID: <20080519060103.GB3179@steel.home> URL: https://gitlist.dev/e/20080519060103.GB3179%40steel.home In-Reply-To: <4830C71A.60308@verizon.net> ``` Mark Levedahl, Mon, May 19, 2008 02:17:30 +0200: > Jeff King wrote: >> On Sun, May 18, 2008 at 05:23:37PM +0200, Alex Riesen wrote: >> >>> This is on Cygwin, yes. I have the core.filemode disabled in >>> ~/.gitconfig. How about stopping the test before the failing portion >>> (only the last two fail, below)? >> >> What's in your ~/.gitconfig shouldn't have any effect (the test scripts >> take care to avoid looking at anything outside of your git directory). >> But presumably this test is broken on Cygwin, anyway? >> > On Cygwin, support for correctly interpreting the execute bit is governed > by the operating system (WindowsNT/XP vs Windows9x), the file system > (NTFS vs FAT), and the (NO)NTEA and (NO)NTSEC options via the CYGWIN *and* the contents of the file. Look at the code. ``` ## Alex Riesen, 2008-05-19 20:23 Subject: Re: t3701 fails if core.filemode disabled Message-ID: <20080519202342.GA9694@steel.home> URL: https://gitlist.dev/e/20080519202342.GA9694%40steel.home In-Reply-To: <20080518200121.GA5789@steel.home> ``` Alex Riesen, Sun, May 18, 2008 22:01:21 +0200: > Jeff King, Sun, May 18, 2008 21:08:39 +0200: > > I don't mind disabling these tests if they don't make sense on certain > > platforms, but regarding your specific proposal: > > > > - can you confirm that the test doesn't make sense, and not that it is > > simply broken on cygwin? Does changing your ~/.gitconfig's > > core.filemode make a difference? It shouldn't, but that could be > > a bug in test-lib. What happens if you run the test manually? Does > > git-add just not prompt for the mode change? > > I setting core.filemode _inside_ the test breaks it in exactly the > same way (on Linux, I'm at home). I'll retest tomorrow It is "git init" which sets core.filemode false (of course!) ``` ## Jeff King, 2008-05-19 20:55 Subject: Re: t3701 fails if core.filemode disabled Message-ID: <20080519205550.GA24246@sigill.intra.peff.net> URL: https://gitlist.dev/e/20080519205550.GA24246%40sigill.intra.peff.net In-Reply-To: <20080519202342.GA9694@steel.home> ``` On Mon, May 19, 2008 at 10:23:42PM +0200, Alex Riesen wrote: > > I setting core.filemode _inside_ the test breaks it in exactly the > > same way (on Linux, I'm at home). I'll retest tomorrow > > It is "git init" which sets core.filemode false (of course!) Ah, of course. In that case, then your change makes sense; by definition, if core.filemode isn't set, those tests are meaningless. Though I think a final version should, as we discussed, omit those tests rather than ending the test script. -Peff ``` ## Alex Riesen, 2008-05-20 21:59 Subject: [PATCH] Fix t3701 if core.filemode disabled Message-ID: <20080520215932.GB10437@steel.home> URL: https://gitlist.dev/e/20080520215932.GB10437%40steel.home In-Reply-To: <20080519205550.GA24246@sigill.intra.peff.net> ``` Signed-off-by: Alex Riesen --- Jeff King, Mon, May 19, 2008 22:55:50 +0200: > On Mon, May 19, 2008 at 10:23:42PM +0200, Alex Riesen wrote: > > > > I setting core.filemode _inside_ the test breaks it in exactly the > > > same way (on Linux, I'm at home). I'll retest tomorrow > > > > It is "git init" which sets core.filemode false (of course!) > > Ah, of course. In that case, then your change makes sense; by definition, > if core.filemode isn't set, those tests are meaningless. Though I think > a final version should, as we discussed, omit those tests rather than > ending the test script. > Sure. I have explicitely test for core.filemode=false, because some older setups (where git init did not set it) don't have the setting and git config core.filemode reports nothing. t/t3701-add-interactive.sh | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index f15be93..bd94ac6 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -65,6 +65,7 @@ test_expect_success 'revert works (commit)' ' git add -i output && grep "unchanged *+3/-0 file" output ' +if test "$(git config core.filemode)" != false ; then test_expect_success 'patch does not affect mode' ' git reset --hard && @@ -84,5 +85,6 @@ test_expect_success 'stage mode but not hunk' ' git diff file | grep "+content" ' +fi test_done -- 1.5.5.1.354.g902c ``` ## Jeff King, 2008-05-21 14:36 Subject: Re: [PATCH] Fix t3701 if core.filemode disabled Message-ID: <20080521143607.GA3575@sigill.intra.peff.net> URL: https://gitlist.dev/e/20080521143607.GA3575%40sigill.intra.peff.net In-Reply-To: <20080520215932.GB10437@steel.home> ``` On Tue, May 20, 2008 at 11:59:32PM +0200, Alex Riesen wrote: > Sure. I have explicitely test for core.filemode=false, because > some older setups (where git init did not set it) don't have > the setting and git config core.filemode reports nothing. I don't think that is relevant here; this is a test script, and as such is testing the results of the current version of git-init (from when we git-init the trash directory). That being said, I think the test you have is perfectly fine, and the patch is correct. And I am OK with the patch as-is, though I have a few style nits: > @@ -65,6 +65,7 @@ test_expect_success 'revert works (commit)' ' > git add -i output && > grep "unchanged *+3/-0 file" output > ' > +if test "$(git config core.filemode)" != false ; then > > test_expect_success 'patch does not affect mode' ' > git reset --hard && > @@ -84,5 +85,6 @@ test_expect_success 'stage mode but not hunk' ' > git diff file | grep "+content" > ' > > +fi 1. It should be $(git config --bool core.filemode). As it happens, git-init always uses the word "false" so this works OK, but it is probably better to model good behavior and to be more robust. 2. It's a little hard to see which tests are affected. I would have done something more like: if test "$(git config --bool core.filemode)" = true test_filemode= else test_filemode=: fi $test_filemode test_expect_success ... But maybe that is just overengineering. 3. Usually when we skip tests we do something like say 'skipping filemode tests (filesystem does not properly support modes') -Peff ``` ## Alex Riesen, 2008-05-22 13:20 Subject: Re: [PATCH] Fix t3701 if core.filemode disabled Message-ID: <20080522132042.GF3206@steel.home> URL: https://gitlist.dev/e/20080522132042.GF3206%40steel.home In-Reply-To: <20080521143607.GA3575@sigill.intra.peff.net> ``` Jeff King, Wed, May 21, 2008 16:36:07 +0200: > 2. It's a little hard to see which tests are affected. I would have done > something more like: > > if test "$(git config --bool core.filemode)" = true > test_filemode= > else > test_filemode=: > fi > > $test_filemode test_expect_success ... > > But maybe that is just overengineering. But a nice one. I like the idea but Junio already did your other suggestions in master, so I just keep it in mind for the next one ``` ## Junio C Hamano, 2008-05-22 17:49 Subject: Re: [PATCH] Fix t3701 if core.filemode disabled Message-ID: <7vzlqiut4a.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzlqiut4a.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080522132042.GF3206@steel.home> ``` Alex Riesen writes: > Jeff King, Wed, May 21, 2008 16:36:07 +0200: >> 2. It's a little hard to see which tests are affected. I would have done >> something more like: >> >> if test "$(git config --bool core.filemode)" = true >> test_filemode= >> else >> test_filemode=: >> fi >> >> $test_filemode test_expect_success ... >> >> But maybe that is just overengineering. > > But a nice one. I like the idea but Junio already did your other > suggestions in master, so I just keep it in mind for the next one If you like that, I think you would like the way t0050 does even better ;-). It is Steffen Prohaska's invention, IIRC. ``` ## Jeff King, 2008-05-22 17:55 Subject: Re: [PATCH] Fix t3701 if core.filemode disabled Message-ID: <20080522175552.GA14391@sigill.intra.peff.net> URL: https://gitlist.dev/e/20080522175552.GA14391%40sigill.intra.peff.net In-Reply-To: <7vzlqiut4a.fsf@gitster.siamese.dyndns.org> ``` On Thu, May 22, 2008 at 10:49:09AM -0700, Junio C Hamano wrote: > > But a nice one. I like the idea but Junio already did your other > > suggestions in master, so I just keep it in mind for the next one > > If you like that, I think you would like the way t0050 does even better > ;-). It is Steffen Prohaska's invention, IIRC. Well, you can't test_expect_failure with it. :) Though I think it is actually nice to mention which tests are being skipped (something I asked for in point 3 of my other message, but which contradicts the example I gave in point 2 of the same message :) ). So something like: have_foo= test_foo() { case "$have_foo" in t) test_expect_success "$@" *) say "skipping test $1 (don't have foo)" esac } test_expect_success 'see if we have foo' ' if magic_foo_test; then have_foo=t fi || true ' test_foo 'use foo' '...' -Peff ``` ## Alex Riesen, 2008-05-22 21:22 Subject: Re: [PATCH] Fix t3701 if core.filemode disabled Message-ID: <81b0412b0805221422m734f019emc7a60bb5d44d7369@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0805221422m734f019emc7a60bb5d44d7369%40mail.gmail.com In-Reply-To: <7vzlqiut4a.fsf@gitster.siamese.dyndns.org> ``` 2008/5/22 Junio C Hamano : > Alex Riesen writes: >> Jeff King, Wed, May 21, 2008 16:36:07 +0200: >>> >>> $test_filemode test_expect_success ... >>> >>> But maybe that is just overengineering. >> >> But a nice one. I like the idea but Junio already did your other >> suggestions in master, so I just keep it in mind for the next one > > If you like that, I think you would like the way t0050 does even better > ;-). It is Steffen Prohaska's invention, IIRC. > I do :) ```