From: kristofferhaugsbakk@fastmail.com
Date: Fri, 01 May 2026 17:24:49 GMT
Subject: Re: [PATCH v3 3/5] name-rev: factor code for sharing with a new command
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:
>>[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.

>
> 			*(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