Subject: Re: [PATCH v9 1/3] push: add reflog check for "--force-if-includes"

From: Srinidhi Kaushik <[email-masked]>
Date: Sat, 3 Oct 2020 01:09:06 +0530

Hi Junio,

On 10/02/2020 09:41, Junio C Hamano wrote:
> Srinidhi Kaushik <[email-masked]> writes:
> 
> >> We should not call `in_merge_bases_many()` repeatedly: there is a much
> >> better API for that: `get_reachable_subset()`.
> >
> > Perfect. I wasn't aware of this.
> 
> This is possibly a piece of misinformation.  in_merge_bases_many()
> is designed to be callable more than once.  get_reachable_subset()
> may be an overkill as we only are interested in a single "is this
> one an ancestor of any of these?", not "which ones among these are
> ancestors of the other set?".

Noted; even though "get_reachable_subset()" and "in_merge_bases_many()"
(after the commit-graph fix) return the same result, I suppose the
latter was designed for this specific use-case.

> > OK. The tests are passing with or without "GIT_TEST_COMMIT_GRAPH"
> > by switching to "get_reachable_subset()" we don't have to toggle
> > te feature during the check.
> 
> Correct.  Once the "see if this one is reachable from any of these"
> is fixed (either by correcting the broken in_merge_bases_many() or
> using get_reachable_subset()), we can get rid of this hack.

OK. Shall I update the next set by reverting the "disable commit-graph"
change, s/list/array/ and leaving the rest as is -- if we decide to go
forward with "in_merge_bases_many()", that is?

> > Again, thank you so much working on this! If you'd like, I can go ahead
> > and apply these patches and rename "reflog_commit_list" to "commit_array"
> > in the next series (v10).
> 
> I like the s/list/array/ change, but I do not think switching to
> get_reachable_subset() and having to receive a commit list only to
> free the list is warranted.
> 
> Derrick sent a fix to in_merge_bases_many() in the near-by thread.

Nice! Will take a look.

Thanks.
-- 
Srinidhi Kaushik