Patrick Steinhardt <ps@xxxxxx> writes:
> 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.
>
> 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?
> 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.
>> @@ -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
Attachment:
signature.asc
Description: PGP signature
