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

From

Johannes Schindelin <johannes.schindelin@gmx.de>

Date

Oct 2, 2020, 13:52 UTC

Message-ID

<nycvar.QRO.7.76.6.2010021550170.50@tvgsbejvaqbjf.bet>

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

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

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