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

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

Hi Johannes,

On 10/02/2020 15:52, Johannes Schindelin wrote:
> 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?):

Thanks for looking into this. :)
 
> -- 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.

Makes sense. I'll change it.
 
> 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()`.

Perfect. I wasn't aware of this.

> 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;
> -}
> -

OK. The tests are passing with or without "GIT_TEST_COMMIT_GRAPH"
by switching to "get_reachable_subset()" we don't have to toggle
te feature during the check.

>  /*
>   * 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
> 

Again, thank you so much working on this! If you'd like, I can go ahead
and apply these patches and rename "reflog_commit_list" to "commit_array"
in the next series (v10).

Thanks.
-- 
Srinidhi Kaushik