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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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