Re: [PATCH 1/5] upload-pack: look up "want" lines via commit-graph

[Derrick Stolee <derrickstolee@github.com>]

On 3/1/2022 4:24 AM, Patrick Steinhardt wrote:
> On Tue, Mar 01, 2022 at 09:43:19AM +0100, Patrick Steinhardt wrote:
>> On Wed, Feb 23, 2022 at 09:13:53AM -0500, Derrick Stolee wrote:
>>> On 2/23/2022 7:35 AM, Patrick Steinhardt wrote:

>>>> -		o = parse_object(the_repository, &oid);
>>>> +		commit = lookup_commit_in_graph(the_repository, &oid);
>>>> +		if (commit)
>>>> +			o = &commit->object;
>>>> +		else
>>>> +			o = parse_object(the_repository, &oid);
>>>> +
>>>
>>> This is a neat trick. I see that we've also done this trick in
>>> revision.c:get_reference(). Perhaps it is worth creating a helper,
>>> maybe named parse_probably_commit()?
>>
>> That might be a good idea, thanks. I'll have a look at what the end
>> result would look like.
>>
>> Patrick
> 
> I had a look at existing callsites which use `lookup_commit_in_graph()`,
> but I found that it wasn't easily possible to convert them all to use a
> new helper like you propose. Most of them have some custom logic like
> skipping `parse_object()` if it's part of a promisor pack, so I really
> only found two locations where such a new helper could be used without
> also adding and supporting flags. I don't really think that's worth it
> for now.

Thanks for taking the time to look into it.

-Stolee