git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "SZEDER Gábor" <szeder.dev@gmail.com>,
	git@vger.kernel.org, "Taylor Blau" <me@ttaylorr.com>,
	"Garima Singh" <garima.singh@microsoft.com>,
	"Derrick Stolee" <dstolee@microsoft.com>
Subject: Re: [PATCH v9 0/3] push: add "--[no-]force-if-includes"
Date: Thu, 1 Oct 2020 23:24:43 +0530	[thread overview]
Message-ID: <20201001175443.GA28444@mail.clickyotomy.dev> (raw)
In-Reply-To: <xmqq4knddg5v.fsf@gitster.c.googlers.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

  reply	other threads:[~2020-10-01 17:54 UTC|newest]

Thread overview: 120+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-04 18:51 [PATCH] push: make `--force-with-lease[=<ref>]` safer Srinidhi Kaushik
2020-09-07 15:23 ` Phillip Wood
2020-09-08 15:48   ` Srinidhi Kaushik
2020-09-07 16:14 ` Junio C Hamano
2020-09-08 16:00   ` Srinidhi Kaushik
2020-09-08 21:00     ` Junio C Hamano
2020-09-07 19:45 ` Johannes Schindelin
2020-09-08 15:58   ` Junio C Hamano
2020-09-09  3:40     ` Johannes Schindelin
2020-09-08 16:59   ` Srinidhi Kaushik
2020-09-16 11:55     ` Johannes Schindelin
2020-09-08 19:34   ` Junio C Hamano
2020-09-09  3:44     ` Johannes Schindelin
2020-09-10 10:22       ` Johannes Schindelin
2020-09-10 14:44         ` Srinidhi Kaushik
2020-09-11 22:16           ` Johannes Schindelin
2020-09-14 11:06             ` Srinidhi Kaushik
2020-09-14 20:08             ` Junio C Hamano
2020-09-16  5:31               ` Srinidhi Kaushik
2020-09-16 10:20                 ` Johannes Schindelin
2020-09-19 17:48                   ` Junio C Hamano
2020-09-10 14:46         ` Junio C Hamano
2020-09-11 22:17           ` Johannes Schindelin
2020-09-14 20:07             ` Junio C Hamano
2020-09-12 15:04 ` [PATCH v2 0/2] push: make "--force-with-lease" safer Srinidhi Kaushik
2020-09-12 15:04   ` [PATCH v2 1/2] push: add "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-12 18:20     ` Junio C Hamano
2020-09-12 21:25       ` Srinidhi Kaushik
2020-09-12 15:04   ` [PATCH v2 2/2] push: enable "forceIfIncludesWithLease" by default Srinidhi Kaushik
2020-09-12 18:22     ` Junio C Hamano
2020-09-12 18:15   ` [PATCH v2 0/2] push: make "--force-with-lease" safer Junio C Hamano
2020-09-12 21:03     ` Srinidhi Kaushik
2020-09-13 14:54   ` [PATCH v3 0/7] push: add "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-13 14:54     ` [PATCH v3 1/7] remote: add reflog check for "force-if-includes" Srinidhi Kaushik
2020-09-14 20:17       ` Junio C Hamano
2020-09-16 10:51         ` Srinidhi Kaushik
2020-09-14 20:31       ` Junio C Hamano
2020-09-14 21:13       ` Junio C Hamano
2020-09-16 12:35       ` Johannes Schindelin
2020-09-19 17:01         ` Srinidhi Kaushik
2020-09-13 14:54     ` [PATCH v3 2/7] transport: add flag for "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-13 14:54     ` [PATCH v3 3/7] send-pack: check ref status for "force-if-includes" Srinidhi Kaushik
2020-09-13 14:54     ` [PATCH v3 4/7] transport-helper: update " Srinidhi Kaushik
2020-09-13 14:54     ` [PATCH v3 5/7] builtin/push: add option "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-16 12:36       ` Johannes Schindelin
2020-09-13 14:54     ` [PATCH v3 6/7] doc: add reference for "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-14 21:01       ` Junio C Hamano
2020-09-16  5:35         ` Srinidhi Kaushik
2020-09-13 14:54     ` [PATCH v3 7/7] t: add tests for "force-if-includes" Srinidhi Kaushik
2020-09-16 12:47     ` [PATCH v3 0/7] push: add "--[no-]force-if-includes" Johannes Schindelin
2020-09-19 17:03   ` [PATCH v4 0/3] " Srinidhi Kaushik
2020-09-19 17:03     ` [PATCH v4 1/3] push: add reflog check for "--force-if-includes" Srinidhi Kaushik
2020-09-19 20:03       ` Junio C Hamano
2020-09-21  8:42         ` Srinidhi Kaushik
2020-09-21 18:48           ` Junio C Hamano
2020-09-23 10:22             ` Srinidhi Kaushik
2020-09-23 16:47               ` Junio C Hamano
2020-09-21 13:19         ` Phillip Wood
2020-09-21 16:12           ` Junio C Hamano
2020-09-21 18:11             ` Junio C Hamano
2020-09-23 10:27           ` Srinidhi Kaushik
2020-09-19 17:03     ` [PATCH v4 2/3] push: parse and set flag " Srinidhi Kaushik
2020-09-19 20:26       ` Junio C Hamano
2020-09-19 17:03     ` [PATCH v4 3/3] t, doc: update tests, reference " Srinidhi Kaushik
2020-09-19 20:42       ` Junio C Hamano
2020-09-23  7:30     ` [PATCH v5 0/3] push: add "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-23  7:30       ` [PATCH v5 1/3] push: add reflog check for "--force-if-includes" Srinidhi Kaushik
2020-09-23 10:18         ` Phillip Wood
2020-09-23 11:26           ` Srinidhi Kaushik
2020-09-23 16:24           ` Junio C Hamano
2020-09-23 16:29         ` Junio C Hamano
2020-09-23  7:30       ` [PATCH v5 2/3] push: parse and set flag " Srinidhi Kaushik
2020-09-23  7:30       ` [PATCH v5 3/3] t, doc: update tests, reference " Srinidhi Kaushik
2020-09-23 10:24         ` Phillip Wood
2020-09-26 10:13       ` [PATCH v6 0/3] push: add "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-26 10:13         ` [PATCH v6 1/3] push: add reflog check for "--force-if-includes" Srinidhi Kaushik
2020-09-26 10:13         ` [PATCH v6 2/3] push: parse and set flag " Srinidhi Kaushik
2020-09-26 10:13         ` [PATCH v6 3/3] t, doc: update tests, reference " Srinidhi Kaushik
2020-09-26 10:21         ` [PATCH v6 0/3] push: add "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-26 11:46         ` [PATCH v7 " Srinidhi Kaushik
2020-09-26 11:46           ` [PATCH v7 1/3] push: add reflog check for "--force-if-includes" Srinidhi Kaushik
2020-09-26 23:42             ` Junio C Hamano
2020-09-27 12:27               ` Srinidhi Kaushik
2020-09-26 11:46           ` [PATCH v7 2/3] push: parse and set flag " Srinidhi Kaushik
2020-09-26 11:46           ` [PATCH v7 3/3] t, doc: update tests, reference " Srinidhi Kaushik
2020-09-27 14:17           ` [PATCH v8 0/3] push: add "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-27 14:17             ` [PATCH v8 1/3] push: add reflog check for "--force-if-includes" Srinidhi Kaushik
2020-09-27 14:17             ` [PATCH v8 2/3] push: parse and set flag " Srinidhi Kaushik
2020-09-27 14:17             ` [PATCH v8 3/3] t, doc: update tests, reference " Srinidhi Kaushik
2020-09-30 12:54               ` Philip Oakley
2020-09-30 14:27                 ` Srinidhi Kaushik
2020-09-28 17:31             ` [PATCH v8 0/3] push: add "--[no-]force-if-includes" Junio C Hamano
2020-09-28 17:46               ` SZEDER Gábor
2020-09-28 19:34                 ` Srinidhi Kaushik
2020-09-28 19:51                   ` Junio C Hamano
2020-09-28 20:00                 ` Junio C Hamano
2020-10-01  8:21             ` [PATCH v9 " Srinidhi Kaushik
2020-10-01  8:21               ` [PATCH v9 1/3] push: add reflog check for "--force-if-includes" Srinidhi Kaushik
2020-10-02 13:52                 ` Johannes Schindelin
2020-10-02 14:50                   ` Johannes Schindelin
2020-10-02 16:22                     ` Junio C Hamano
2020-10-02 15:07                   ` Srinidhi Kaushik
2020-10-02 16:41                     ` Junio C Hamano
2020-10-02 19:39                       ` Srinidhi Kaushik
2020-10-02 20:14                         ` Junio C Hamano
2020-10-02 20:58                           ` Srinidhi Kaushik
2020-10-02 21:36                             ` Junio C Hamano
2020-10-02 16:26                   ` Junio C Hamano
2020-10-01  8:21               ` [PATCH v9 2/3] push: parse and set flag " Srinidhi Kaushik
2020-10-01  8:21               ` [PATCH v9 3/3] t, doc: update tests, reference " Srinidhi Kaushik
2020-10-01 15:46               ` [PATCH v9 0/3] push: add "--[no-]force-if-includes" Junio C Hamano
2020-10-01 17:12                 ` Junio C Hamano
2020-10-01 17:54                   ` Srinidhi Kaushik [this message]
2020-10-01 18:32                     ` Junio C Hamano
2020-10-02 16:50                     ` Junio C Hamano
2020-10-02 19:42                       ` Srinidhi Kaushik
2020-10-03 12:10               ` [PATCH v10 " Srinidhi Kaushik
2020-10-03 12:10                 ` [PATCH v10 1/3] push: add reflog check for "--force-if-includes" Srinidhi Kaushik
2020-10-03 12:10                 ` [PATCH v10 2/3] push: parse and set flag " Srinidhi Kaushik
2020-10-03 12:10                 ` [PATCH v10 3/3] t, doc: update tests, reference " Srinidhi Kaushik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201001175443.GA28444@mail.clickyotomy.dev \
    --to=shrinidhi.kaushik@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=garima.singh@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    --cc=szeder.dev@gmail.com \
    --subject='Re: [PATCH v9 0/3] push: add "--[no-]force-if-includes"' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).