Re: [PATCH v4 2/3] replay: make atomic ref updates the default behavior

From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Oct 28, 2025, 19:03 UTC
Message-ID: <6a41eae1-8d44-401b-85e2-4e52187da525@gmail.com>
In-Reply-To: <xmqq7bwmy6r6.fsf@gitster.g>

On 23/10/25 02:49, Junio C Hamano wrote:

Show 15 quoted lines

> Siddharth Asthana <siddharthasthana31@gmail.com> writes:
>
>> diff --git a/builtin/replay.c b/builtin/replay.c
>> index b64fc72063..1246add636 100644
>> --- a/builtin/replay.c
>> +++ b/builtin/replay.c
>> @@ -20,6 +20,11 @@
>>   #include <oidset.h>
>>   #include <tree.h>
>>   
>> +enum ref_action_mode {
>> +	REF_ACTION_UPDATE,
>> +	REF_ACTION_PRINT
>> +};
>> +

Hi Junio, Thank you for the detailed review! Both are straightforward fixes:

Show 7 quoted lines

> We allow and encourage the last item in enum definition to have
> trailing comma, i.e.
>
>          enum ref_action_mode {
>                  REF_ACTION_UPDATE,
>                  REF_ACTION_PRINT,
>          };

Will add the trailing comma in v5 - makes future additions much cleaner.

Show 11 quoted lines

>
> unless the last one is somehow special and we are not supposed to
> add any new item after that (e.g., a sentinel REF_ACTION_MAX that is
> supposed to give the upper limit of the values).  That way, future
> developers can add new items with minimum patch noise.
>
>> @@ -434,10 +491,15 @@ int cmd_replay(int argc,
>> ...
>> +					ret = error(_("failed to update ref %s: %s"),
>> +						    decoration->name, transaction_err.buf);
> Hmph, don't we want to use '%s' when reporting the ->name thing?

You are absolutely right about the codingGuidelines. I will fix both error messages to properly quote the ref names:

         error(_("failed to update ref '%s': %s"), decoration->name, 
transaction_err.buf);

These will be in v5 along with Christian and Philip's feedback.

Thanks, Siddharth

Show 29 quoted lines

> Documentation/CodingGuidelines has this:
>
>     Error Messages
>
>      - Do not end a single-sentence error message with a full stop.
>
>      - Do not capitalize the first word, only because it is the first word
>        in the message ("unable to open '%s'", not "Unable to open '%s'").  But
>        "SHA-3 not supported" is fine, because the reason the first word is
>        capitalized is not because it is at the beginning of the sentence,
>        but because the word would be spelled in capital letters even when
>        it appeared in the middle of the sentence.
>
>      - Say what the error is first ("cannot open '%s'", not "%s: cannot open").
>
>      - Enclose the subject of an error inside a pair of single quotes,
>        e.g. `die(_("unable to open '%s'"), path)`.
>
>      - Unless there is a compelling reason not to, error messages from
>        porcelain commands should be marked for translation, e.g.
>        `die(_("bad revision %s"), revision)`.
>
>      - Error messages from the plumbing commands are sometimes meant for
>        machine consumption and should not be marked for translation,
>        e.g., `die("bad revision %s", revision)`.
>
>      - BUG("message") are for communicating the specific error to developers,
>        thus should not be translated.
>