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