Re: [PATCH] t2017: redo physical reflog existance check

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 24, 2010, 04:03 UTC
Message-ID: <20100824040347.GA19817@burratino>
In-Reply-To: <AANLkTin76s-ONFuP+OWdxB5LJNf2D1Du+hKxB2s_WhTa@mail.gmail.com>

Hi Erick,

First, thanks for checkout --orphan. I was a skeptic but now that I have seen it being used for things similar to "going open source" (and the related "simplifying logs while working privately on a patch series") it looks to be a pretty nice tool.

Erick Mattos wrote:

> I don't see a need for so much reluctance: "test -f" is not a taboo
> inside a script in t folder and the added tests don't change anything
> about the design and implementation which IMHO is well fit.

The principle (though we do not always adhere to it) is that test scripts should pass or fail based only on advertised behavior, not implementation details. That way, _later_ any person who wants to improve the implementation will not be impeded by tests.

The behavior that "test -f .git/logs/refs/heads/eta" checks for is not part of the advertised behavior and though it does affect the observable behavior, it is not immediately obvious how. Wouldn't it be best if the test described that advertised behavior while checking for it?

e.g.:

	git config core.logallrefupdates false &&
	test_when_finished "git config core.logallrefupdates true" &&

  	git checkout master &&
  	git checkout -l --orphan eta &&
	test_must_fail git rev-parse --verify eta@{0} &&

	test_tick &&
	git commit -m "initial commit" &&
	git rev-parse --verify eta@{0}

Happily, I am not the man in charge, so feel free to take my words at whatever value you choose. :)

Regards, Jonathan