Re: [PATCH v3 3/5] name-rev: factor code for sharing with a new command
- From
- kristofferhaugsbakk@fastmail.com <kristofferhaugsbakk@fastmail.com>
- Date
- May 1, 2026, 17:24 UTC
- Message-ID
- <20260501172450.25037-2-kristofferhaugsbakk@fastmail.com>
- In-Reply-To
- <8016697f-9eb7-4c75-be87-d9479186919c@gmail.com>
Hi Phillip. Thanks for taking a look.
On Thu, Apr 30, 2026, at 15:54, Phillip Wood wrote:
Show 18 quoted lines
>>[snip]
>> @@ -524,25 +543,32 @@ static void name_rev_line(char *p, struct name_ref_data *data)
>> const char *name = NULL;
>> char c = *(p + 1);
>> int p_len = p - p_start + 1;
>> + struct object *o = NULL;
>> + int oid_ret = 1;
>>
>> counter = 0;
>>
>> *(p + 1) = 0;
>> - if (!repo_get_oid(the_repository, p - (hexsz - 1), &oid)) {
>> - struct object *o =
>> - lookup_object(the_repository, &oid);
>> + oid_ret = repo_get_oid(the_repository, p - (hexsz - 1), &oid);
>
> It would be safer to restore *(p + 1) here rather that relying on each
> case block to do it.Yeah, I didn’t want to repeat that bookkeeping but in some iteration it looked necessary. But it’s good that it isn’t.
Show 15 quoted lines
>
> *(p + 1) = c;
>> +
>> + switch (cmd->type) {
>> + case NAME_REV:
>> + if (!oid_ret)
>> + o = lookup_object(the_repository, &oid);
>> if (o)
>> name = get_rev_name(o, &buf);
>> + *(p + 1) = c;
>> + if (!name)
>> + goto start;
>
> The pre-image uses "continue" which will increment p - why the change in
> behavior?They looked the same to me. So I will need to think about this some more. Just a lack of C experience on my part.
Replacing the `continue` with a goto at the start of the loop was also unnecessary. Of course the `continue` breaks out of the loop and not the switch-block (unlike `break`).
But I didn’t break `t6120-describe.sh`. So I’ll also take a look to see if there are any holes.
Thanks again.
>[snip]
-- Happy May Day