From: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
Date: Tue, 05 May 2026 19:21:11 GMT
Subject: Re: [PATCH v3 3/5] name-rev: factor code for sharing with a new command
Message-ID: <a0712ecf-5daf-4932-acde-1a4983d2d56c@app.fastmail.com>
In-Reply-To: <65e013cd-5bca-4340-8018-bcbb44371e4f@gmail.com>

On Sat, May 2, 2026, at 12:00, Phillip Wood wrote:
>>>[snip]
>>
>> 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.

They’re not shared. Both commands work the same: either the object name
is consumed and replaced or it is written out as-is, in other words when
commit lookup fails. But somehow the name-rev path prints that *failure
to look up* case before continuing here (I don’t know how):

    if (!name)
        continue;

Because the printf(3) only prints when a symbolic name was found. Either
name-only:

    <symbolic name>

Or not name-only:

    <object name> (<symbolic name>)

On the other hand format-rev uses those two print statements to output
either the name lookup case or the lookup failure case.

>>>[snip]
>>
>> 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.

Yeah, I have gone back to the sensible `continue`. Thanks.

(To be honest I tried to provoke a parsing bug here but I was unable
to. Somewhat annoying.)