Re: [PATCH] Fix failing test t3700-add.sh

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 29, 2016, 16:49 UTC
Message-ID: <xmqqmvl0pext.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160729163937.GD29773@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 22 quoted lines

> I was also puzzled why the test fails for you; it does not for me.
> Running the test script as root does make it fail. There are some
> earlier tests which are skipped in this case, which run "git reset
> --hard" with xfoo1 in the index, which cleans it up.
>
>> +	echo foo >xfoo &&
>> +	chmod 755 xfoo &&
>> +	git add --chmod=-x xfoo &&
>> +	case "$(git ls-files --stage xfoo)" in
>> +	100644" "*xfoo) echo pass;;
>> +	*) echo fail; git ls-files --stage xfoo; (exit 1);;
>
> Here you just pick another name, "xfoo", which does happen to work. But
> it seems like that has the same potential for flakiness if earlier tests
> get adjusted or skipped, since they also use that name.
>
> How about just:
>
>   rm -f xfoo1
>
> at the top of the test, which explicitly documents the state we are
> looking for?

That's much more sensible.

Show 5 quoted lines

> I also wondered if this test, which calls "chmod 755 xfoo1", should be
> marked with the POSIXPERM prerequisite. But I guess since its goal is to
> strip the executable bit, it "works" even on systems where that chmod is
> a noop (the "git add --chmod" doesn't do anything, but one way or the
> other we end up at the end state we expect).

We could make sure --chmod=[-+]x works both ways, which would be more robust on either type of underlying platform. Something along the lines of

	echo foo >xfoo1 &&
        git add --chmod=+x xfoo1 &&
        test_mode_in_index 100755 xfoo1 &&
        git add --chmod=-x xfoo1 &&
        test_mode_in_index 100644 xfoo1

with an obvious addition of a test_mode_in_index helper function as the same "case $(ls-files -s) in ... esac" pattern appears number of times.