Re: [PATCH 3/3] commit: use prio_queue_replace() in pop_most_recent_commit()

["René Scharfe" <l.s.r@web.de>]

On 7/15/25 10:43 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>> diff --git a/commit.c b/commit.c
>> index 0200759aaa..8244221b30 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -742,17 +742,24 @@ void commit_list_sort_by_date(struct commit_list **list)
>>  struct commit *pop_most_recent_commit(struct prio_queue *queue,
>>  				      unsigned int mark)
>>  {
>> -	struct commit *ret = prio_queue_get(queue);
>> +	struct commit *ret = prio_queue_peek(queue);
>> +	int delete_pending = 1;
> 
> Briefly I was puzzled by the name (I would have called first-parent
> since the logic was "we treat first parent specially by using
> replace instead of get/put"), but the variable signals "instead of
> get to remove the item from the queue, we just peeked, so we need to
> remove it later" with its name, which is understandable.

Indeed, we're just interested in the removal part of prio_queue_get()
here, as we have done the cheap half (the peeking) already.  We don't
have a prio_queue_delete().  Adding one would perhaps add some clarity
here, but also widen the interface and probably not bring much of a
performance gain.

So perhaps calling the variable get_pending like the prio_queue_get()
that we end up invoking would reduce the initial puzzlement?

> 
>>  	struct commit_list *parents = ret->parents;
>>  
>>  	while (parents) {
>>  		struct commit *commit = parents->item;
>>  		if (!repo_parse_commit(the_repository, commit) && !(commit->object.flags & mark)) {
>>  			commit->object.flags |= mark;
>> -			prio_queue_put(queue, commit);
>> +			if (delete_pending)
>> +				prio_queue_replace(queue, commit);
>> +			else
>> +				prio_queue_put(queue, commit);
>> +			delete_pending = 0;
>>  		}
>>  		parents = parents->next;
>>  	}
>> +	if (delete_pending)
>> +		prio_queue_get(queue);
>>  	return ret;
>>  }