Re: [PATCH v2 2/4] fetch: use batched reference updates

From: Karthik Nayak <karthik.188@gmail.com>
Date: May 16, 2025, 09:53 UTC
Message-ID: <CAOLa=ZR+3RPDHucjEVx8s64nrVGjzNTu9gX6Nw5vwQGg8PtpUw@mail.gmail.com>
In-Reply-To: <aCbP1SxncSVw2fCa@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 46 quoted lines

> On Thu, May 15, 2025 at 04:07:26PM +0200, Karthik Nayak wrote:
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 5279997c96..15eac2b1c2 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1688,6 +1644,37 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)
>>  	return result;
>>  }
>>
>> +struct ref_rejection_data {
>> +	int *retcode;
>> +	int conflict_msg_shown;
>> +	const char *remote_name;
>> +};
>> +
>> +static void ref_transaction_rejection_handler(const char *refname,
>> +					      const struct object_id *old_oid UNUSED,
>> +					      const struct object_id *new_oid UNUSED,
>> +					      const char *old_target UNUSED,
>> +					      const char *new_target UNUSED,
>> +					      enum ref_transaction_error err,
>> +					      void *cb_data)
>> +{
>> +	struct ref_rejection_data *data = (struct ref_rejection_data *)cb_data;
>
> Nit: unnecessary cast.
>
>> +	if (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {
>> +		error(_("some local refs could not be updated; try running\n"
>> +			" 'git remote prune %s' to remove any old, conflicting "
>> +			"branches"), data->remote_name);
>> +		data->conflict_msg_shown = 1;
>> +	} else {
>> +		char *reason = ref_transaction_error_msg(err);
>> +
>> +		error(_("fetching ref %s failed: %s"), refname, reason);
>> +		free(reason);
>> +	}
>> +
>> +	*data->retcode = 1;
>> +}
>
> Okay, we stopped ignoring generic errors now and will print them. What
> I'm still unclear about: which exact errors do we accept now that
> `REF_TRANSACTION_ALLOW_FAILURE` is specified? Most of the error codes we
> probably want to accept, but what about `REF_TRANSACTION_ERROR_GENERIC`?

The current mechanism in `ref_transaction_maybe_set_rejected()` doesn't handle `REF_TRANSACTION_ERROR_GENERIC` errors. This was a design choice (more of a requirement of what this error represents), where `REF_TRANSACTION_ERROR_GENERIC` errors cannot be resolved on an individual reference level. It includes:

  - System errors such as I/O errors
  - Duplicates present

Both of these represent issues which are bigger than a single ref update, so we have to propagate these errors up.

Show 9 quoted lines

>
> This makes me wonder a bit about the current layout of how we handle
> these errors. If the rejection handler was invoked while preparing the
> transaction for each reference as we go instead of afterwards we could
> decide on-the-fly whether a specific error should be ignored or not.
> That might lead to a design that is both more flexible and more obvious
> at the same time because error handling is now handled explicitly by the
> callsite that wants to ignore some errors.
>

I did ponder on this while I was building the batched transaction mechanism. I decided to take it iteratively. We can, for instance, modify `ref_transaction_maybe_set_rejected()` to work with a callback function which would allow the users to accept/reject errors.

However, even if we go down that route, `REF_TRANSACTION_ERROR_GENERIC` errors still cannot be overlooked, these errors will abort the entire transaction.

That said, I'm not trying to avoid going down that route. I do agree with the flexibility it does provide. Once we hit such a usecase, we should make that change.

For 'git-fetch(1)' and 'git-recieve-pack(1)', do you see a usecase?

Show 8 quoted lines

> Last but not least, I think that it would also allow us to decide ahead
> of time whether we want to commit. Right now we basically say "just
> commit it, whatever happens". But if I'm not mistaken, all the errors
> that we care about and that callers may want to ignore are already
> detected at prepare time. So if we already bubbled up relevant info
> while calling `ref_transaction_prepare()` the caller may then decide to
> not commit at all based on some criteria.
>

Indeed, that is correct. I can confirm that even now all the calls to `ref_transaction_maybe_set_rejected()` are made in the prepare phase, so we could already do this, since `transaction->rejections` is already populated at this stage.

> Sorry, I should've probably proposed this when you introducued this
> mechanism. But sometimes you only see things like that as we gain more
> users.
>

You don't have to apologize. Such discussions are very important and you shouldn't hesitate to bring up such points.

Show 15 quoted lines

>> @@ -1808,6 +1795,24 @@ static int do_fetch(struct transport *transport,
>>  			retcode = 1;
>>  	}
>>
>> +	/*
>> +	 * If not atomic, we can still use batched updates, which would be much
>> +	 * more performant. We don't initiate the transaction before pruning,
>> +	 * since pruning must be an independent step, to avoid F/D conflicts.
>> +	 *
>> +	 * TODO: if reference transactions gain logical conflict resolution, we
>> +	 * can delete and create refs (with F/D conflicts) in the same transaction
>> +	 * and this can be moved about the 'prune_refs()' block.
>
> s/about/above/?
>

Indeed!

> Patrick