Re: [PATCH RESEND] branch: allow deleting dangling branches with --force

From: Junio C Hamano <gitster@pobox.com>
To: Han-Wen Nienhuys <hanwen@google.com>
Cc: "René Scharfe" <l.s.r@web.de>, "Git List" <git@vger.kernel.org>,
	"Ulrich Windl" <Ulrich.Windl@rz.uni-regensburg.de>
Subject: Re: [PATCH RESEND] branch: allow deleting dangling branches with --force
Date: Thu, 26 Aug 2021 10:38:29 -0700	[thread overview]
Message-ID: <xmqq1r6gf6ne.fsf@gitster.g> (raw)
In-Reply-To: <xmqq5yvsf8p5.fsf@gitster.g> (Junio C. Hamano's message of "Thu, 26 Aug 2021 09:54:14 -0700")

Junio C Hamano <gitster@pobox.com> writes:

> Han-Wen Nienhuys <hanwen@google.com> writes:
>
>> On Wed, Aug 25, 2021 at 11:37 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> > +test_expect_success 'branch --delete --force removes dangling branch' '
>>> > +     test_when_finished "rm -f .git/refs/heads/dangling" &&
>>> > +     echo $ZERO_OID >.git/refs/heads/dangling &&
>>> > +     git branch --delete --force dangling &&
>>> > +     test_path_is_missing .git/refs/heads/dangling
>>> > +'
>>>
>>> This goes against the spirit of the series merged at c9780bb2 (Merge
>>> branch 'hn/prep-tests-for-reftable', 2021-07-13).
>>>
>>> Can we creat the dangling ref and test the lack of "dangling" ref in
>>> the end in a less transparent way?
>>
>> agreed. Try the ref-store test-helper's update-ref command?
>
> I thought the approach taken by dc474899 (t4202: mark bogus head
> hash test with REFFILES, 2021-05-31) to hide it behind a
> prerequisite was good enough, but if we can ensure the same
> behaviour under the reftable backend, that is even better.
>
> Thanks.

Having said that, there are a few observations to make about this
test script.

 * It is hopefully becoming harder and harder to check for behaviour
   in broken repositories in a "portable" way, simply because we are
   making it harder to corrupt repository.  We hopefully won't point
   a ref to point at a missing object, we hopefully won't prune an
   object away that is still pointed at by a ref, etc.

 * This script to test "branch" is full of tests that rely on direct
   manipulation of .git/refs/ filesystem hierarchy.

For these two reasons, it probably is OK to accept this patch as-is
and leave the "clean-up" to a later follow-on series, that would
cover both "what's our recommended approach to 'corrupt' the test
repository so that we can use different ref (and other) backends?"
and "make sure the tests in the script are happy with both ref
backends." issues.

Thanks.