git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCH v9 1/3] push: add reflog check for "--force-if-includes"
Date: Fri, 2 Oct 2020 15:52:43 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2010021550170.50@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20201001082118.19441-2-shrinidhi.kaushik@gmail.com>

Hi Srinidhi,


On Thu, 1 Oct 2020, Srinidhi Kaushik wrote:

> Add a check to verify if the remote-tracking ref of the local branch
> is reachable from one of its "reflog" entries.
>
> The check iterates through the local ref's reflog to see if there
> is an entry for the remote-tracking ref and collecting any commits
> that are seen, into a list; the iteration stops if an entry in the
> reflog matches the remote ref or if the entry timestamp is older
> the latest entry of the remote ref's "reflog". If there wasn't an
> entry found for the remote ref, "in_merge_bases_many()" is called
> to check if it is reachable from the list of collected commits.
>
> When a local branch that is based on a remote ref, has been rewound
> and is to be force pushed on the remote, "--force-if-includes" runs
> a check that ensures any updates to the remote-tracking ref that may
> have happened (by push from another repository) in-between the time
> of the last update to the local branch (via "git-pull", for instance)
> and right before the time of push, have been integrated locally
> before allowing a forced update.
>
> If the new option is passed without specifying "--force-with-lease",
> or specified along with "--force-with-lease=<refname>:<expect>" it
> is a "no-op".
>
> Calls to "in_merge_bases_many()" return different results depending
> on whether the "commit-graph" feature is enabled or not -- it is
> temporarily disabled when the check runs [1].
>
> [1] https://lore.kernel.org/git/xmqqtuvhn6yx.fsf@gitster.c.googlers.com

I can verify that the multiple calls to `in_merge_bases_many()` lead to a
problem, and I intend to debug this further, but it is the wrong function
to call to begin with.

With these two patches, the tests pass for me, and they also reduce the
complexity quite a bit (Junio, could I ask you to put them on top of
sk/force-if-includes?):

-- snipsnap --
From 0e7bd31c4cb0ae08ad772ac230eea2dd7a884886 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 2 Oct 2020 15:33:05 +0200
Subject: [PATCH 1/2] fixup??? push: add reflog check for "--force-if-includes"

This follows the pattern used elsewhere.

Maybe we should also rename this to `commit_array`? It is not a linked
list, after all.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 remote.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 37533cafc44..2c6c63aa906 100644
--- a/remote.c
+++ b/remote.c
@@ -2441,6 +2441,7 @@ struct reflog_commit_list {
 	struct commit **item;
 	size_t nr, alloc;
 };
+#define REFLOG_COMMIT_LIST_INIT { NULL, 0, 0 }

 /* Append a commit to the list. */
 static void append_commit(struct reflog_commit_list *list,
@@ -2514,7 +2515,7 @@ static int is_reachable_in_reflog(const char *local, const struct ref *remote)
 	struct commit *commit;
 	struct commit **chunk;
 	struct check_and_collect_until_cb_data cb;
-	struct reflog_commit_list list = { NULL, 0, 0 };
+	struct reflog_commit_list list = REFLOG_COMMIT_LIST_INIT;
 	size_t size = 0;
 	int ret = 0;

--
2.28.0.windows.1.18.g5300e52e185


From 10ea5640015f4bc7144e8e5b025e31294329c600 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 2 Oct 2020 15:35:58 +0200
Subject: [PATCH 2/2] fixup??? push: add reflog check for "--force-if-includes"

We should not call `in_merge_bases_many()` repeatedly: there is a much
better API for that: `get_reachable_subset()`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 remote.c | 43 ++++---------------------------------------
 1 file changed, 4 insertions(+), 39 deletions(-)

diff --git a/remote.c b/remote.c
index 2c6c63aa906..881415921e2 100644
--- a/remote.c
+++ b/remote.c
@@ -2513,10 +2513,9 @@ static int is_reachable_in_reflog(const char *local, const struct ref *remote)
 {
 	timestamp_t date;
 	struct commit *commit;
-	struct commit **chunk;
 	struct check_and_collect_until_cb_data cb;
 	struct reflog_commit_list list = REFLOG_COMMIT_LIST_INIT;
-	size_t size = 0;
+	struct commit_list *reachable;
 	int ret = 0;

 	commit = lookup_commit_reference(the_repository, &remote->old_oid);
@@ -2542,61 +2541,27 @@ static int is_reachable_in_reflog(const char *local, const struct ref *remote)
 	 * Check if the remote commit is reachable from any
 	 * of the commits in the collected list, in batches.
 	 */
-	for (chunk = list.item; chunk < list.item + list.nr; chunk += size) {
-		size = list.item + list.nr - chunk;
-		if (MERGE_BASES_BATCH_SIZE < size)
-			size = MERGE_BASES_BATCH_SIZE;
-
-		if ((ret = in_merge_bases_many(commit, size, chunk)))
-			break;
-	}
+	reachable = get_reachable_subset(list.item, list.nr, &commit, 1, 0);
+	ret = !!reachable;
+	free_commit_list(reachable);

 cleanup_return:
 	free_reflog_commit_list(&list);
 	return ret;
 }

-/* 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;
-	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;
-}
-
 /*
  * Check for reachability of a remote-tracking
  * ref in the reflog entries of its local ref.
  */
 static void check_if_includes_upstream(struct ref *remote)
 {
-	int prev;
 	struct ref *local = get_local_ref(remote->name);
 	if (!local)
 		return;

-	/*
-	 * TODO: Remove "toggle_commit_graph()" calls around the check.
-	 * Depending on whether "commit-graph" enabled or not,
-	 * "in_merge_bases_many()" returns different results;
-	 * disable it temporarily when the check runs.
-	 */
-	prev = toggle_commit_graph(the_repository, 1);
 	if (is_reachable_in_reflog(local->name, remote) <= 0)
 		remote->unreachable = 1;
-	toggle_commit_graph(the_repository, prev);
 }

 static void apply_cas(struct push_cas_option *cas,
--
2.28.0.windows.1.18.g5300e52e185


  reply	other threads:[~2020-10-02 13:52 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 [this message]
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
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=nycvar.QRO.7.76.6.2010021550170.50@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=shrinidhi.kaushik@gmail.com \
    --subject='Re: [PATCH v9 1/3] push: add reflog check for "--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).