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

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

Hello,

On 10/01/2020 10:12, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:
> >
> >> Changes since v8:
> >>   - Disable "commit-graph" when "in_merge_bases_many()" is called
> >>     for this check, because it returns different results depending
> >>     on whether "commit-graph" is enabled [1].
> >
> > Is that a wise move, though?  If the "different results" is
> > expected, then it is a different story, but I would think it is a
> > bug in commit-graph codepath if it produces a result different from
> > what the callers expect, and disabling from the caller's end would
> > mean that we lose one opportunity to help commit-graph folks to go
> > and fix their bugs, no?

I didn't want want to cause a delay with this patch. Since the new
option was seemingly working without it, I decided to disable it here
and added a "TODO" in the comments to remove "toggle_commit_graph()"
in the future. We can definitely put this on hold and wait until the
with and without "commit-graph" result disparities are clarified.

> > Other than that, I think the topic is in quite a good shape.  Thanks
> > for working on polishing it.

Thanks, I appreciate it!
 
> In other words, how about doing it like so.
> 
> In an ideal world, folks who know more about commit-graph than we do
> will find what's broken in in_merge_bases_many() when commit-graph
> is in use, before I finish lecturing against hiding a breakage under
> the rug.  Let's see if another call for help helps ;-)

:)
 
>  remote.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git i/remote.c w/remote.c
> index 98a578f5dc..361b8f1c0e 100644
> --- i/remote.c
> +++ w/remote.c
> @@ -2408,7 +2408,20 @@ static int is_reachable_in_reflog(const char *local, const struct ref *remote)
>  /* Toggle the "commit-graph" feature; return the previously set state. */
>  static int toggle_commit_graph(struct repository *repo, int disable) {
>  	int prev = repo->commit_graph_disabled;
> -	repo->commit_graph_disabled = disable;
> +	static int should_toggle = -1;
> +
> +	if (should_toggle < 0) {
> +		/*
> +		 * The in_merge_bases_many() seems to misbehave when
> +		 * the commit-graph feature is in use.  Disable it for
> +		 * normal users, but keep it enabled when specifically
> +		 * testing the feature.
> +		 */
> +		should_toggle = !git_env_bool("GIT_TEST_COMMIT_GRAPH", 0);
> +	}
> +
> +	if (should_toggle)
> +		repo->commit_graph_disabled = disable;
>  	return prev;
>  }
>  

Ah, this will keep a record of the behavior in the tests; nice,
will update in the next set.

I looked around a little bit trying to understand what was happening
here. As mentioned before [1], "repo_in_merge_bases_many()" returns
early because of this:

  for (i = 0; i < nr_reference; i++) {
	  if (repo_parse_commit(r, reference[i]))
		  return ret;

	  generation = commit_graph_generation(reference[i]);
	  if (generation < min_generation)
		  min_generation = generation;
	  fprintf(stderr,
		  "[%s]: (local): generation: %u, min_generation: %u\n",
		  oid_to_hex(&reference[i]->object.oid),
		  generation,
		  min_generation);
  }

  generation = commit_graph_generation(commit);
  fprintf(stderr, "[%s]: (remote) generation: %u, min_generation: %u\n",
	  oid_to_hex(&commit->object.oid), generation, min_generation);
  if (generation > min_generation) {
	  return ret;
  }


I made some changes locally to display the generation numbers of all
the commits that were collected during the "reflog" traversal.

In the beginning we get the minimum generation number of the list of
the commits in "reference[]" to see if the "commit" is reachable from
one of the items in "reference[]". Since in this case, the generation
number of "commit" is higher than minimum amongst the members of
"reference", does it mean it cannot be reached?

When "GIT_TEST_COMMIT_GRAPH" is turned off, all of the commits in
"reference" as well as "commit" have "GENERATION_NUMBER_INFINITY",
and the path moves forward to "paint_down_to_common()" and correctly
identifies the reachability.

I ran the test again, but this time with running "git-commit-graph"
right before pushing:

  (a) git commit-graph write --reachable, and the commit's generation
      number was "GENERATION_NUMBER_INFINITY".

  (b) git-show-ref -s | git commit-graph write --stdin-commits, and
      the commit's generation number was 5.

and since generation number of "commit" was always higher than the
minimum -- it returns that it is not reachable from any of "reference".

One of the failing tests in t5533 was modified (for debugging), and
the following shows the difference in behavior of
"repo_in_merge_bases_many()". The test checks if the remote tip is
reachable from the reflog entries of the local branch after
"git pull --rebase" is run.

  $ git log --graph --oneline # (before "pull --rebase")
   * be2465f J
   * 157d38b C
   * f9a2dac B
   * 112d1ac A

  $ git log --graph --oneline # (after "pull --rebase")
   * 7c16010 J
   * b995a30 G
   * 00b6961 F
   * 157d38b C
   * f9a2dac B
   * 112d1ac A

  $ git reflog master
  7c16010 master@{0}: pull --rebase origin master (finish): refs/heads/master onto b995a30227dd14b34b6f7728201aefac8cc12296
  be2465f master@{1}: commit: J
  157d38b master@{2}: commit: C
  f9a2dac master@{3}: commit: B
  112d1ac master@{4}: commit (initial): A

  - (a) is run.
    $ git push --force-if-includes --force-with-lease=master
    [7c16010bad84d8b53873875c2e242890920360f2]: (local):  generation: 4294967295, min_generation: 4294967295
    [be2465f6a155acb8f5eb9ee876bad730e0f656cf]: (local):  generation: 4, min_generation: 4
    [157d38b4112d457e6645c7c4e9a486e6189be435]: (local):  generation: 3, min_generation: 3
    [f9a2dac17e4f8cafaa9655d40cb86c53094da8d6]: (local):  generation: 2, min_generation: 2
    [112d1ac551b908f10b995d7e41456f4cd8f071c5]: (local):  generation: 1, min_generation: 1
    [b995a30227dd14b34b6f7728201aefac8cc12296]: (remote): generation: 4294967295, min_generation: 1
    [...] fail.

  - "git fetch --all" and (b) is run.
    $ git push --force-if-includes --force-with-lease=master
    [7c16010bad84d8b53873875c2e242890920360f2]: (local):  generation: 4294967295, min_generation: 4294967295
    [be2465f6a155acb8f5eb9ee876bad730e0f656cf]: (local):  generation: 4, min_generation: 4
    [157d38b4112d457e6645c7c4e9a486e6189be435]: (local):  generation: 3, min_generation: 3
    [f9a2dac17e4f8cafaa9655d40cb86c53094da8d6]: (local):  generation: 2, min_generation: 2
    [112d1ac551b908f10b995d7e41456f4cd8f071c5]: (local):  generation: 1, min_generation: 1
    [b995a30227dd14b34b6f7728201aefac8cc12296]: (remote): generation: 5, min_generation: 1
    [...] fail.

  - neither (a), nor (b) are run, and "core.commitGraph" is disabled.
    $ git push --force-if-includes --force-with-lease=master
    [7c16010bad84d8b53873875c2e242890920360f2]: (local):  generation: 4294967295, min_generation: 4294967295
    [be2465f6a155acb8f5eb9ee876bad730e0f656cf]: (local):  generation: 4294967295, min_generation: 4294967295
    [157d38b4112d457e6645c7c4e9a486e6189be435]: (local):  generation: 4294967295, min_generation: 4294967295
    [f9a2dac17e4f8cafaa9655d40cb86c53094da8d6]: (local):  generation: 4294967295, min_generation: 4294967295
    [112d1ac551b908f10b995d7e41456f4cd8f071c5]: (local):  generation: 4294967295, min_generation: 4294967295
    [b995a30227dd14b34b6f7728201aefac8cc12296]: (remote): generation: 4294967295, min_generation: 4294967295
    [...] pass.

It looks like G (b995a30) is reachable from J (7c16010), but
"repo_in_merge_bases_many()" disagrees when "GIT_TEST_COMMIT_GRAPH"
is enabled. I hope this information helps a little to understand
why this happens and whether it is a bug or not.

[1] https://public-inbox.org/git/20200928193400.GA88208@mail.clickyotomy.dev

Thanks.
-- 
Srinidhi Kaushik