t3701 fails if core.filemode disabled

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

Hi,

On Sun, 18 May 2008, Alex Riesen wrote:

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"?

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

2008/5/18 Johannes Schindelin <Johannes.Schindelin@gmx.de>:

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

On Sun, May 18, 2008 at 05:23:37PM +0200, Alex Riesen wrote:

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

Jeff King, Sun, May 18, 2008 21:08:39 +0200:

Correct.

I setting core.filemode _inside_ the test breaks it in exactly the same way (on Linux, I'm at home). I'll retest tomorrow

I agree

Could be.

That - too. I shall see tomorrow

Alex Riesen, Sun, May 18, 2008 22:01:21 +0200:

It is "git init" which sets core.filemode false (of course!)

On Mon, May 19, 2008 at 10:23:42PM +0200, Alex Riesen wrote:

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

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Jeff King, Mon, May 19, 2008 22:55:50 +0200:

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

On Tue, May 20, 2008 at 11:59:32PM +0200, Alex Riesen wrote:

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:

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

Jeff King, Wed, May 21, 2008 16:36:07 +0200:

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

Alex Riesen <raa.lkml@gmail.com> writes:

If you like that, I think you would like the way t0050 does even better ;-). It is Steffen Prohaska's invention, IIRC.

On Thu, May 22, 2008 at 10:49:09AM -0700, Junio C Hamano wrote:

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

2008/5/22 Junio C Hamano <gitster@pobox.com>:

I do :)

Jeff King wrote:

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

Mark Levedahl, Mon, May 19, 2008 02:17:30 +0200:

*and* the contents of the file. Look at the code.