Re: [PATCH v8 0/3] push: add "--[no-]force-if-includes"

[Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>]

Hello,

On 09/28/2020 19:46, SZEDER Gábor wrote:
> On Mon, Sep 28, 2020 at 10:31:34AM -0700, Junio C Hamano wrote:
> > Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:
> > 
> > > Add a new option: "--force-if-includes" to "git-push" where forced
> > > updates are allowed only if the tip of the remote-tracking ref has
> > > been integrated locally, by verifying if the tip of the remote-tracking
> > > ref -- on which a local branch has based on -- is reachable from at
> > > least one of the "reflog" entries of the branch about to be updated
> > > by force on the remote.
> > 
> > https://travis-ci.org/github/git/git/jobs/730962458 is a build of
> > 'seen' with this topic, and the same 'seen' without this topic is
> > https://travis-ci.org/github/git/git/builds/730857608 that passes
> > all the jobs.  It is curious why one particular job fails while
> > others in the same build is OK.
> 
> That build runs the test suite with a bunch of GIT_TEST_* knobs
> enabled, and the last two tests added in this series fail when run as:
> 
>   GIT_TEST_COMMIT_GRAPH=1 ./t5533-push-cas.sh
 
Thanks for the heads-up. It turns out that "in_merge_bases_many()"
returns different results depending on "GIT_TEST_COMMIT_GRAPH".
Initially I thought that it might be related to batching the entries,
but that is not the case.

One of the tests that is failing is:
  cd src &&
  git switch branch &&
  test_commit I &&
  git switch master &&
  test_commit J &&
  git pull --rebase origin master &&
  git push --force-if-includes --force-with-lease="master"

Here, we are testing to check if forced updates are allowed after
the remote changes have been incorporated locally, which is true
in this case and should pass.

"in_merge_bases_many()" used in the check as follows:

  for (chunk = list.items; chunk < list.items + list.nr; chunk += size) {
	  size = list.items + list.nr - chunk;
	  if (MERGE_BASES_BATCH_SIZE < size)
	        size = MERGE_BASES_BATCH_SIZE;

	  if ((ret = in_merge_bases_many(commit, size, chunk)))
	        break;
  }

In "repo_in_merge_bases_many()" [1], the following condition evaluates
to true when "GIT_TEST_COMMIT_GRAPH" is 1.

	generation = commit_graph_generation(commit);
	if (generation > min_generation)
		return ret;

Unfortunately, I am unfamiliar with the code, and not sure why this
happens; I remember Junio mention [2] something about generation
numbers could it be related to that?

A possible "workaround" is to use "in_merge_bases()" for each of the
commits we collect in the list, and the tests seem to pass with
"GIT_TEST_COMMIT_GRAPH" being set; but I wonder if that's the right
way to fix this.

[1] https://git.kernel.org/pub/scm/git/git.git/tree/commit-reach.c#n319
[2] https://public-inbox.org/git/xmqqft7djzz0.fsf@gitster.c.googlers.com/

Thanks.
-- 
Srinidhi Kaushik