Re: [PATCH 4/5] rebase --keep-base: imply --reapply-cherry-picks

From: Junio C Hamano <gitster@pobox.com>
To: "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	Philippe Blain <levraiphilippeblain@gmail.com>,
	Denton Liu <liu.denton@gmail.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH 4/5] rebase --keep-base: imply --reapply-cherry-picks
Date: Mon, 15 Aug 2022 13:58:51 -0700	[thread overview]
Message-ID: <xmqqlerpz0j8.fsf@gitster.g> (raw)
In-Reply-To: <9cd4c372ee4b3e5ba45c66a43ad0edaf52f0eed9.1660576283.git.gitgitgadget@gmail.com> (Phillip Wood via GitGitGadget's message of "Mon, 15 Aug 2022 15:11:22 +0000")

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> As --keep-base does not rebase the branch it is confusing if it
> removes commits that have been cherry-picked to the upstream branch.
> As --reapply-cherry-picks is not supported by the "apply" backend this
> commit ensures that cherry-picks are reapplied by forcing the upstream
> commit to match the onto commit unless --no-reapply-cherry-picks is
> given.
>
> Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  Documentation/git-rebase.txt     |  2 +-
>  builtin/rebase.c                 | 15 ++++++++++++++-
>  t/t3416-rebase-onto-threedots.sh | 21 +++++++++++++++++++++
>  3 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 080658c8710..dc0c6c54e27 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -218,7 +218,7 @@ leave out at most one of A and B, in which case it defaults to HEAD.
>  	merge base of `<upstream>` and `<branch>`. Running
>  	`git rebase --keep-base <upstream> <branch>` is equivalent to
>  	running
> -	`git rebase --onto <upstream>...<branch> <upstream> <branch>`.
> +	`git rebase --reapply-cherry-picks --onto <upstream>...<branch> <upstream> <branch>`.
>  +
>  This option is useful in the case where one is developing a feature on
>  top of an upstream branch. While the feature is being worked on, the
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 86ea731ca3a..b6b3e00e3b1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1181,6 +1181,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	prepare_repo_settings(the_repository);
>  	the_repository->settings.command_requires_full_index = 0;
>  
> +	options.reapply_cherry_picks = -1;
>  	options.allow_empty_message = 1;
>  	git_config(rebase_config, &options);
>  	/* options.gpg_sign_opt will be either "-S" or NULL */
> @@ -1240,6 +1241,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		if (options.root)
>  			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--root");
>  	}
> +	/*
> +	 * --keep-base defaults to --reapply-cherry-picks as it is confusing if
> +	 * commits disappear when using this option.
> +	 */
> +	if (options.reapply_cherry_picks < 0)
> +		options.reapply_cherry_picks = keep_base;

It makes me wonder if an explicit "--no-reapply-cherry-picks" makes
sense in combination with "--keep-base".  If that happens, we do not
take this "By default, reapply is enabled with keep-base".

> @@ -1416,7 +1423,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	if (options.empty != EMPTY_UNSPECIFIED)
>  		imply_merge(&options, "--empty");
>  
> -	if (options.reapply_cherry_picks)
> +	/*
> +	 * --keep-base implements --reapply-cherry-picks by altering upstream so
> +	 * it works with both backends.
> +	 */
> +	if (options.reapply_cherry_picks && !keep_base)
>  		imply_merge(&options, "--reapply-cherry-picks");

Interesting.  The idea is that we shouldn't care how much progress
(which may include cherry-picks) the upstream side made, and it is
no use to compare the commits between the F (fork point) and O (our
tip) against the commits between updated U (upstream) and F (fork
point) to notice that X' is a cherry-pick from our X.

              o---X---o---O (our work)
             /
	----F----o----o----o----X'----U (upstream)

So almost ignoring U (except for obviously figure out F, possibly,
for the purpose of keep-base) is an effective way to keep X on our
history, and when it happens, we do not have to explicitly pass the
"--reapply" option to underlying rebase machinery.  Makes sense.

If an explicit "--no-reapply-cherry-picks" with "--keep-base" is
given, we still skip this and do not call imply_merge() ...

> @@ -1680,6 +1691,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	}
>  	if (keep_base) {
>  		oidcpy(&merge_base, &options.onto->object.oid);
> +		if (options.reapply_cherry_picks)
> +			options.upstream = options.onto;

... but this is also skipped in such a case.  I do not offhand know
if the combination makes practical sense, but this should allow the
combination to "work".  OK.

Thanks.