Re: [PATCH v2 1/7] fetch: increase test coverage of fetches

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 18, 2022, 16:59 UTC
Message-ID: <xmqqy2282jrq.fsf@gitster.g>
In-Reply-To: <Yg9Bkyp1EDvOzzOp@ncase>

Patrick Steinhardt <ps@pks.im> writes:

Show 22 quoted lines

> On Thu, Feb 17, 2022 at 12:41:33PM -0800, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
> [snip]
>> > +test_expect_success 'atomic fetch with failing backfill' '
>> > +	git init clone3 &&
>> > +
>> > +	# We want to test whether a failure when backfilling tags correctly
>> > +	# aborts the complete transaction when `--atomic` is passed: we should
>> > +	# neither create the branch nor should we create the tag when either
>> > +	# one of both fails to update correctly.
>> > +	#
>> > +	# To trigger failure we simply abort when backfilling a tag.
>> 
>> What does this paragraph want the phrase `backfilling tags` to mean?
>> Just from end-user's perspective, there is only one (i.e. if an
>> object that is tagged gets fetched and that tag is not here, fetch
>> it too), but at the mechanism level, there are two distinct code
>> paths (i.e. if OPT_FOLLOWTAGS gets the job done, there is no need to
>> call backfill_tags()).  Which failure does this talk about, or it
>> does not matter which code path gave us the tag?
>
> It refers to `backfill_tags()`. Should I update this comment to clarify?

After reading the patch, the issue is only about the case where we perform the second fetch and the case where OPT_FOLLOWTAGS does everything necessary is not relevant. So hinting that we are interested to see what a failure in the follow-on fetch does to the atomicity would be a good idea, and mentioning backfill_tags() would have been a good way to do so. Perhaps like "whether a failure in backfill_tags() correctly aborts..." or something?

Thanks.