Re: [PATCH] checkout: avoid BUG() when hitting a broken repository

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 22, 2022, 11:55 UTC
Message-ID: <220122.86y238ht7r.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqbl04d1s9.fsf_-_@gitster.g>

On Fri, Jan 21 2022, Junio C Hamano wrote:

Show 5 quoted lines

> So, taking the two earlier comments from me together...
>
> I _think_ I was the one who spotted the funny skip_prefix() whose
> result was not used, and suggested this unrelated check, during the
> review.  Sorry about that.

Where was that? I don't see a comment like that in the original thread[1], or do you mean the recent one in [2]?

Doesn't matter much now, but whatever it was I can't find it nor recall it, or I've misread this.

Show 17 quoted lines

> [...]
> +test_expect_success 'checkout a branch without refs/heads/* prefix' '
> +	git clone --no-tags . repo-odd-prefix &&
> +	(
> +		cd repo-odd-prefix &&
> +
> +		origin=$(git symbolic-ref refs/remotes/origin/HEAD) &&
> +		git symbolic-ref refs/heads/a-branch "$origin" &&
> +
> +		git checkout -f a-branch &&
> +		git checkout -f a-branch
> +	)
> +'
> +
>  test_expect_success 'checkout -b to a new branch, set to HEAD' '
>  	test_when_finished "
>  		git checkout branch1 &&

It's shortly before the release, so whatever fixes the bug is good with me, and this patch works.

I don't think dropping the parts of the tests that actually check the resulting repository state is a good change in this re-imagining of my initial fix[3].

I see that per your [4] you disagree with the current behavior being "cast in stone". I also think we should change it, I just think testing exactly what state we're in before and after will make that easier.

Here we're just testing that we don't die, and not even that it's not a noop. If and when we change the behavior it'll be extra work to check that we didn't change something we didn't expect (and basically requiring digging up [3] again).

In this case we didn't have any test coverage (hence missing the regression), and with this test we still don't have meaningful coverage.

If you're looking to clearly mark things that are desired v.s. expected behavior wouldn't that be better done in general via something like a new "test_expect_oddity"?

Again, for the upcoming release I think this is fine. I'd just like to clarify the above, since this isn't the first time we've had a back and forth where you wanted a less specific test that (in the "make coverage" etc. sense) would lose coverage v.s. a more specific check.

1. https://lore.kernel.org/git/patch-1.1-9b17170b794-20211014T000949Z-avarab@gmail.com/
2. https://lore.kernel.org/git/xmqqr190d2xg.fsf@gitster.g/
3. https://lore.kernel.org/git/patch-1.1-21ddf7c628d-20220120T212233Z-avarab@gmail.com/
4. https://lore.kernel.org/git/xmqqlez8d2e6.fsf@gitster.g/