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

From: Jeff King <peff@peff.net>
Date: May 21, 2008, 14:36 UTC
Message-ID: <20080521143607.GA3575@sigill.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:

Show 13 quoted lines

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