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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 2, 2020, 16:41 UTC
Message-ID: <xmqqft6wa8dc.fsf@gitster.c.googlers.com>
In-Reply-To: <20201002150710.GA54370@mail.clickyotomy.dev>

Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> 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?".

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

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

Thanks.