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

From: Derrick Stolee <stolee@gmail.com>
Date: Jul 20, 2018, 19:09 UTC
Message-ID: <215c6ad7-a403-9173-5965-70f68e796d7a@gmail.com>
In-Reply-To: <CACsJy8CTPtxLsSoNO8xynHGoPqQ3k6N4mSykWTQ9Ceap3yV1hw@mail.gmail.com>

On 7/20/2018 1:41 PM, Duy Nguyen wrote:

Show 11 quoted lines

> 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.

Show 6 quoted lines

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