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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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

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

> [...]
> +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/