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

[Derrick Stolee <stolee@gmail.com>]

On 10/2/2020 4:03 PM, Junio C Hamano wrote:
> 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