From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 07 Nov 2025 14:07:59 GMT
Subject: Re: [PATCH v2] fetch: fix non-conflicting tags not being committed
Message-ID: <aQ39P0mAFqDGPYxS@pks.im>
In-Reply-To: <CAOLa=ZQpTqnCQs4=wcUwJOWy5mXiG4y_eTiFtPkS2uOk4U66Tw@mail.gmail.com>

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

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