Re: [PATCH v2 00/18] Consolidate reachability logic

[Derrick Stolee <stolee@gmail.com>]

On 7/20/2018 1:41 PM, Duy Nguyen wrote:
> On Fri, Jul 20, 2018 at 6:35 PM Derrick Stolee <dstolee@microsoft.com> wrote:
>> There are many places in Git that use a commit walk to determine
>> reachability between commits and/or refs. A lot of this logic is
>> duplicated.
>>
>> I wanted to achieve the following:
>>
>> Consolidate several different commit walks into one file
> I'm surprised get_shallow_commits() in shallow.c didn't make the cut.
> It's no problem though if you already considered it and decided it was
> better left alone.

Thanks for pointing this out. I didn't know about it. It would make an 
excellent follow-up series.

>> Reduce duplicate reachability logic
>> Increase testability (correctness and performance)
>> Improve performance of reachability queries
> What's your recommendation on adding new commit reachability code? I
> might have to add one to fix prune_shallow() if I don't find anything
> fit. I guess the code should go to commit-reach.c too?

In my opinion, new commit walks should go into commit-reach.c. Then, you 
can justify why you are using a "new" walk instead of using an existing 
walk. Further, you can probably think of the walk in more generic terms 
than the specific application you need. Finally, you can use the 
'test-tool reach <method>' pattern to test the specific walk you create 
outside of the logic for which you needed it.

I understand that while this patch is under review, you will probably 
want to continue adding your walk where it is, then we can consolidate 
the code after both have settled.

Thanks,

-Stolee