Re: [PATCH v2 7/7] update-ref: add --allow-partial flag for stdin mode

From: Karthik Nayak <karthik.188@gmail.com>
Date: Mar 3, 2025, 20:34 UTC
Message-ID: <CAOLa=ZTeDH+yWpK6KBK=yDFymyUFuDg7Qb3Ww7EnX8wgHC0_ag@mail.gmail.com>
In-Reply-To: <ceda422e-8c8e-4a1d-aaab-9a7a2fc009dd@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 19 quoted lines

> Hi Karthik
>
> On 25/02/2025 09:29, Karthik Nayak wrote:
>> When updating multiple references through stdin, Git's update-ref
>> command normally aborts the entire transaction if any single update
>> fails. While this atomic behavior prevents partial updates by default,
>> there are cases where applying successful updates while reporting
>> failures is desirable.
>>
>> Add a new `--allow-partial` flag that allows the transaction to continue
>> even when individual reference updates fail. This flag can only be used
>> in `--stdin` mode and builds upon the partial transaction support added
>> to the refs subsystem.
>
> As '--stdin' allows a single instance of "git update-ref" to create more
> than one transaction perhaps we should instead allow the caller to
> specify which transactions they want to allow to fail by passing an
> argument to "start", similar to how we support "no-deref" with "update"
>

I was considering adding a 'allow-rejection' flag similar to "no-deref" with "update", but decided against it since that was an update level config and we want a transaction level configuration.

But I like your idea better. I'm not bullish on either the current implementation or your suggestion, since we can extend one from the other. My main motivation to add this flag to update-ref(1) is to show viability. The goal would be to follow up with changes to `git-fetch(1)` and `git-receive-pack(1)` to use partial transactions.

Show 13 quoted lines

>> following format:
>>
>>    rejected SP (<old-oid> | <old-target>) SP (<new-oid> | <new-target>) SP <rejection-reason> LF
>>
>> or with `-z`:
>>
>>    rejected NUL (<old-oid> | <old-target>) NUL (<new-oid> | <new-target>) NUL <rejection-reason> NUL
>
> What's the reason for the different output with '-z'? In the list of
> options '-z' is documented as only applying to the input stream. Looking
> at the code the existing messages generated by report_ok() are all
> printed to stdout with a LF terminator.
>

That's a great point, there was no real reason and I think we can drop this.

Show 55 quoted lines

>> +static void print_rejected_refs(const char *refname,
>> +				const struct object_id *old_oid,
>> +				const struct object_id *new_oid,
>> +				const char *old_target,
>> +				const char *new_target,
>> +				enum transaction_error err,
>> +				void *cb_data UNUSED)
>> +{
>> +	struct strbuf sb = STRBUF_INIT;
>> +	char space = ' ';
>> +	const char *reason = "";
>> +
>> +	switch (err) {
>> +	case TRANSACTION_NAME_CONFLICT:
>> +		reason = _("refname conflict");
>> +		break;
>> +	case TRANSACTION_CREATE_EXISTS:
>> +		reason = _("reference already exists");
>> +		break;
>> +	case TRANSACTION_NONEXISTENT_REF:
>> +		reason = _("reference does not exist");
>> +		break;
>> +	case TRANSACTION_INCORRECT_OLD_VALUE:
>> +		reason = _("incorrect old value provided");
>> +		break;
>> +	case TRANSACTION_INVALID_NEW_VALUE:
>> +		reason = _("invalid new value provided");
>> +		break;
>> +	case TRANSACTION_EXPECTED_SYMREF:
>> +		reason = _("expected symref but found regular ref");
>> +		break;
>> +	default:
>> +		reason = _("unkown failure");
>> +	}
>
> I agree with Patrick that these messages should not be translated.
>
>> +	if (!line_termination)
>> +		space = line_termination;
>> +
>> +	strbuf_addf(&sb, "rejected%c%s%c%s%c%c%s%c%s%c", space,
>> +		    refname, space, new_oid ? oid_to_hex(new_oid) : new_target,
>> +		    space, space, old_oid ? oid_to_hex(old_oid) : old_target,
>> +		    space, reason, line_termination);
>> +
>> +	fwrite(sb.buf, sb.len, 1, stdout);
>> +	strbuf_release(&sb);
>> +	fflush(stdout);
>
> There is no need to flush after each line, we'll flush all the error
> messages when we call report_ok() in parse_cmd_commit() or when the
> program exits. The caller has no way to know how many error messages
> there are to read so flushing each one individually does not help the
> reader avoid deadlocks.
>

That does make sense, I'll remove the flush here!

Show 21 quoted lines

>> +}
>> +
>>   static void parse_cmd_commit(struct ref_transaction *transaction,
>>   			     const char *next, const char *end UNUSED)
>>   {
>> @@ -573,6 +622,10 @@ static void parse_cmd_commit(struct ref_transaction *transaction,
>>   		die("commit: extra input: %s", next);
>>   	if (ref_transaction_commit(transaction, &error))
>>   		die("commit: %s", error.buf);
>> +
>> +	ref_transaction_for_each_rejected_update(transaction,
>> +						 print_rejected_refs, NULL);
>> +
>>   	report_ok("commit");
>
> This is good, the caller knows to stop reading when they see "commit: ok"
>
>
> Best Wishes
>
> Phillip

Thanks for the review on the series.