From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 24 Mar 2023 14:18:35 GMT
Subject: Re: [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id
Message-ID: <88947332-fca5-978e-4405-5c616bd91d29@dunelm.org.uk>
In-Reply-To: <ZBzGSbm7GZVK17ja@ugly>

Hi Oswald

On 23/03/2023 21:36, Oswald Buddenhagen wrote:
> On Thu, Mar 23, 2023 at 07:34:57PM +0000, Phillip Wood wrote:
>> On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>>> ... instead of as a commit, which makes the purpose clearer and will
>>> simplify things later.
>>
>> given that we want onto to be a commit I'm not sure how this makes 
>> anything clearer.
>>
> it makes it clearer that we need only the oid, not any other part of the 
> commit object. and pulling ahead the "extraction" reduces the visual 
> noise further down.
> 
>>> As a side effect, this change revealed that skip_unnecessary_picks() was
>>> butchering the commit object due to missing const-correctness. Slightly
>>> adjust its API to rectify this.
>>
>> I don't think this is correct. If you look at the original code it 
>> makes a copy of the oid and uses the copy when calling 
>> skip_unnecessary_picks()
>>
> oops, you're quite right. (facepalm)
> imo the change still makes sense, though, as it replaces the relatively 
> expensive deep copies with simple pointer updates. so just fix the 
> commit message?

I think you should just drop this patch. Copying struct object_id is 
cheap and idiomatic in the code base. If you grep for

"struct object_id \*\*[a-zA-Z0-9_]*[,)]"

you'll see that there are very few matches and all but one of those are 
passing an array.

Best Wishes

Phillip

>