From: Karthik Nayak <karthik.188@gmail.com>
Date: Fri, 07 Nov 2025 15:13:54 GMT
Subject: Re: [PATCH v2] fetch: fix non-conflicting tags not being committed
Message-ID: <CAOLa=ZS3oFiopf0ys2ZS5z0MdE8s6jqapPyaR86gj5CcJ9jaYQ@mail.gmail.com>
In-Reply-To: <aQ39P0mAFqDGPYxS@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Nov 07, 2025 at 05:15:32AM -0800, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> > On Thu, Nov 06, 2025 at 09:39:25AM +0100, Karthik Nayak wrote:
>> > The following test demonstrates this behaviour:
>> >
>> > 	test_expect_success "fetch single branch without explicit tag option" '
>> > 		git init source &&
>> > 		git -C source commit --allow-empty --message common &&
>> > 		git clone file://"$(pwd)"/source target &&
>> > 		(
>> > 			cd source &&
>> > 			git commit --allow-empty --message discard-me &&
>> > 			git tag discard-me &&
>> > 			git commit --amend --allow-empty --message fetch-me &&
>> > 			git tag fetch-me
>> > 		) &&
>> >
>> > 		# The "discard-me" tag does not point into the history that we are
>> > 		# about to fetch, so it should not have been created.
>> > 		git -C target fetch origin &&
>> > 		git -C target tag -l >actual &&
>> > 		echo "fetch-me" >expect &&
>> >
>> > 		# But with "--tags" we instruct git-fetch(1) to fetch all tags, so we
>> > 		# should now see it.
>> > 		git -C target fetch origin --tags &&
>>
>> Here, we don't really backfill, but rather we request all tags from the
>> remote, hence we end up with the 'discard-me' tag. Not because of the
>> diverged history. I also confirmed this by adding a breakpoint into the
>> `backfill_tags()` function, while running this test.
>
> Oh, exactly. But there's two fetches here: the first one only fetches
> "fetch-me" because we don't pass "--tags". The second one was simply as
> a demonstration that we would also fetch the other tag that doesn't
> point into our fetched history with "--tags".
>

Yup, even the first 'fetch' doesn't hit the backfill flow. Since it
points to the reference being fetched.

> I notice though that the first fetch forgot to `test_cmp`.
>
>> > 		git -C target tag -l >actual &&
>> > 		cat >expect <<-\EOF &&
>> > 		discard-me
>> > 		fetch-me
>> > 		EOF
>> > 		test_cmp expect actual
>> > 	'
>>
>> But I was able to slightly modify the test to get the required affect:
>>
>>   test_expect_success "backfill tags when providing a refspec" '
>>   	git init source &&
>>   	git -C source commit --allow-empty --message common &&
>>   	git clone file://"$(pwd)"/source target &&
>>   	(
>>   	    cd source &&
>>   	    git commit --allow-empty --message history &&
>>   	    git tag history &&
>>   	    git commit --allow-empty --message fetch-me &&
>>   	    git tag fetch-me
>>   	) &&
>>
>>   	# The "history" tag is backfilled eventhough we requested
>>   	# to only fetch the master
>>   	git -C target fetch origin master:branch &&
>>   	git -C target tag -l >actual &&
>>   	cat >expect <<-\EOF &&
>>   	fetch-me
>>   	history
>>   	EOF
>>   	test_cmp expect actual
>>   '
>>
>> I will add this in. Thanks for the explanation, it really helped
>> consolidate my understanding here.
>
> Yup, that should work, as well.
>
>> >> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> >> index c7ff3480fb..d5aee5af10 100644
>> >> --- a/builtin/fetch.c
>> >> +++ b/builtin/fetch.c
>> >> @@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,
> [snip]
>> >> +	if (*transaction && !is_atomic) {
>> >> +		struct ref_rejection_data data = {
>> >> +			.conflict_msg_shown = 0,
>> >> +			.remote_name = remote_name,
>> >> +			.retcode = &retcode,
>> >> +		};
>> >> +
>> >> +		ref_transaction_for_each_rejected_update(*transaction,
>> >> +							 ref_transaction_rejection_handler,
>> >> +							 &data);
>> >> +
>> >> +		ref_transaction_free(*transaction);
>> >> +		*transaction = NULL;
>> >> +	}
>> >
>> > Okay. Do we need to discern cases where this is called and we haven't
>> > managed to even queue a single reference update?
>> >
>>
>> I don't see a reason. This is anyways a post-commit action, if there are
>> no updates, there will be no rejections. So this will be a no-op.
>
> I guess the question was rather whether we fear a negative consequence
> by trying to commit an empty transaction. The commit doesn't know to
> short-circuit empty transactions, so we'd still end up locking data even
> though we eventually end up doing nothing.
>
> Thanks!
>
> Patrick

That's correct, but that's also an internal detail of the reference
backend.

  - In the files backend, since we lock individual files, no updates
    means no locks.

  - The packed backend and reftable backend would lock the entire
    backend.

So I guess this is something we should fix on the backends themselves.