Re: [PATCH] commit-reach: fix in_merge_bases_many bug

From: Derrick Stolee <stolee@gmail.com>
Date: Oct 2, 2020, 20:08 UTC
Message-ID: <ac3ac80d-7f8e-6b16-03fa-1cd38b5d860b@gmail.com>
In-Reply-To: <xmqqr1qg8kfa.fsf@gitster.c.googlers.com>

On 10/2/2020 4:03 PM, Junio C Hamano wrote:

Show 17 quoted lines

> Derrick Stolee <stolee@gmail.com> writes:
> 
>> Thanks for double-checking. I also think that dropping the
>> "hide the error" patch is prudent.
> 
> Thanks again for a quick and straight-forward fix.  As I mentioned
> elsewhere in the thread, it appears that we invented duplicate API
> with parallel implementation in get_reachable_subset(), which seems
> to be a strict superset of in_merge_bases_many(), and that may be
> what led to an initial and incorrect "get_reachable_subset() is not
> broken the same way as in_merge_bases_many() so use it instead"
> response.  I haven't paid attention to the quality of implementation
> of get_reachable_subset() as much as in_merge_bases_many() (e.g. I
> know of an obvious way to optimize the latter) so far, but it would
> be wonderful if we can eventually rewrite in_merge_bases_many() as a
> very thin wrapper() around get_reachable_subset() without any
> downside.

I have self-assigned myself https://github.com/gitgitgadget/git/issues/740 which will investigate these duplicates and see what can be done.

I know that the initial refactoring focused on keeping the "public" interface the same and just creating shims between different method prototypes. In this case, it might be worth doing a full replacement since in_merge_bases_many() has so few callers.

I'll be proactive to look for other cases that might be tricky for new contributors to find the "right" way to do it.

Thanks, -Stolee