t3701 fails if core.filemode disabled

t3701 fails if core.filemode disabled

[Alex Riesen]

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

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

Re: t3701 fails if core.filemode disabled

[Johannes Schindelin]

Hi,

On Sun, 18 May 2008, Alex Riesen wrote:

> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>

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

Re: t3701 fails if core.filemode disabled

[Alex Riesen]

2008/5/18 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> On Sun, 18 May 2008, Alex Riesen wrote:
>> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
>
> 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).

Re: t3701 fails if core.filemode disabled

[Jeff King]

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

Re: t3701 fails if core.filemode disabled

[Alex Riesen]

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

Re: t3701 fails if core.filemode disabled

[Mark Levedahl]

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

Re: t3701 fails if core.filemode disabled

[Alex Riesen]

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.

Re: t3701 fails if core.filemode disabled

[Alex Riesen]

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

Re: t3701 fails if core.filemode disabled

[Jeff King]

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

[PATCH] Fix t3701 if core.filemode disabled

[Alex Riesen]

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
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 </dev/null >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

Re: [PATCH] Fix t3701 if core.filemode disabled

[Jeff King]

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 </dev/null >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

Re: [PATCH] Fix t3701 if core.filemode disabled

[Alex Riesen]

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

Re: [PATCH] Fix t3701 if core.filemode disabled

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> 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.

Re: [PATCH] Fix t3701 if core.filemode disabled

[Jeff King]

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

Re: [PATCH] Fix t3701 if core.filemode disabled

[Alex Riesen]

2008/5/22 Junio C Hamano <gitster@pobox.com>:
> Alex Riesen <raa.lkml@gmail.com> 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 :)