Re: [PATCH v3 3/5] name-rev: factor code for sharing with a new command
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- May 2, 2026, 10:00 UTC
- Message-ID
- <65e013cd-5bca-4340-8018-bcbb44371e4f@gmail.com>
- In-Reply-To
- <20260501172450.25037-2-kristofferhaugsbakk@fastmail.com>
On 01/05/2026 18:24, kristofferhaugsbakk@fastmail.com wrote:
Show 24 quoted lines
> Hi Phillip. Thanks for taking a look.
>
> On Thu, Apr 30, 2026, at 15:54, Phillip Wood wrote:
>>> [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.It looks like the printing code is shared between the two case blocks in patch 5 as well so we should move that outside them as well and just set "name" inside the switch statement.
Show 24 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.I think the difference only matters in pathological cases as "goto start" means we end up looking at the same character twice but the loop carries on as normal after that. We should just keep using "continue", I'm not sure we need a new test case.
Thanks
Phillip
Show 5 quoted lines
> > Thanks again. > >> [snip] >