Re: [PATCH v2 6/8] cherry-pick: decouple `--allow-empty` and `--keep-redundant-commits`

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Brian

On 10/02/2024 07:43, Brian Lyles wrote:
> As noted in the git-cherry-pick(1) docs, `--keep-redundant-commits`
> implies `--allow-empty`, despite the two having distinct,
> non-overlapping meanings:
> 
> - `allow_empty` refers specifically to commits which start empty, as
>    indicated by the documentation for `--allow-empty` within
>    git-cherry-pick(1):
> 
>    "Note also, that use of this option only keeps commits that were
>    initially empty (i.e. the commit recorded the same tree as its
>    parent). Commits which are made empty due to a previous commit are
>    dropped. To force the inclusion of those commits use
>    --keep-redundant-commits."
> 
> - `keep_redundant_commits` refers specifically to commits that do not
>    start empty, but become empty due to the content already existing in
>    the target history. This is indicated by the documentation for
>    `--keep-redundant-commits` within git-cherry-pick(1):
> 
>    "If a commit being cherry picked duplicates a commit already in the
>    current history, it will become empty. By default these redundant
>    commits cause cherry-pick to stop so the user can examine the commit.
>    This option overrides that behavior and creates an empty commit
>    object. Implies --allow-empty."
> 
> This implication of `--allow-empty` therefore seems incorrect: One
> should be able to keep a commit that becomes empty without also being
> forced to pick commits that start as empty. However, the following
> series of commands results in both the commit that became empty and the
> commit that started empty being picked, despite only
> `--keep-redundant-commits` being specified:
> 
>      git init
>      echo "a" >test
>      git add test
>      git commit -m "Initial commit"
>      echo "b" >test
>      git commit -am "a -> b"
>      git commit --allow-empty -m "empty"
>      git cherry-pick --keep-redundant-commits HEAD^ HEAD
> 
> The same cherry-pick with `--allow-empty` would fail on the redundant
> commit, and with neither option would fail on the empty commit.
> 
> Do not imply `--allow-empty` when using `--keep-redundant-commits` with
> git-cherry-pick(1).
>
> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
> ---
> 
> This is the second half of the first commit[1] in v1, which has now been
> split up.
> 
> This commit proposes a breaking change, albeit one that seems correct
> and relatively minor to me. If this change is deemed too controversial,
> I am prepared to drop it from the series. See Junio's[2] and
> Phillip's[3] comments on v1 for additional context.

I agree that if we were starting from scratch there would be no reason 
to tie --apply-empty and --keep-redundant-commits together but I'm not 
sure it is worth the disruption of changing it now. We're about to add 
empty=keep which won't imply --allow-empty for anyone who wants that 
behavior and I still tend to think the practical effect of implying 
--allow-empty with --keep-redundant-commits is largely beneficial as I'm 
skeptical that users want to keep commits that become empty but not the 
ones that started empty.

Best Wishes

Phillip

> [1]: https://lore.kernel.org/git/20240119060721.3734775-2-brianmlyles@gmail.com/
> [2]: https://lore.kernel.org/git/xmqqy1cfnca7.fsf@gitster.g/
> [3]: https://lore.kernel.org/git/8ff4650c-f84f-41bd-a46c-3b845ff29b70@gmail.com/
> 
>   Documentation/git-cherry-pick.txt | 10 +++++++---
>   builtin/revert.c                  |  4 ----
>   t/t3505-cherry-pick-empty.sh      |  6 ++++++
>   3 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index fdcad3d200..c88bb88822 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -131,8 +131,8 @@ effect to your index in a row.
>   	even without this option.  Note also, that use of this option only
>   	keeps commits that were initially empty (i.e. the commit recorded the
>   	same tree as its parent).  Commits which are made empty due to a
> -	previous commit are dropped.  To force the inclusion of those commits
> -	use `--keep-redundant-commits`.
> +	previous commit will cause the cherry-pick to fail.  To force the
> +	inclusion of those commits, use `--keep-redundant-commits`.
> 
>   --allow-empty-message::
>   	By default, cherry-picking a commit with an empty message will fail.
> @@ -144,7 +144,11 @@ effect to your index in a row.
>   	current history, it will become empty.  By default these
>   	redundant commits cause `cherry-pick` to stop so the user can
>   	examine the commit. This option overrides that behavior and
> -	creates an empty commit object.  Implies `--allow-empty`.
> +	creates an empty commit object. Note that use of this option only
> +	results in an empty commit when the commit was not initially empty,
> +	but rather became empty due to a previous commit. Commits that were
> +	initially empty will cause the cherry-pick to fail. To force the
> +	inclusion of those commits use `--allow-empty`.
> 
>   --strategy=<strategy>::
>   	Use the given merge strategy.  Should only be used once.
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 89821bab95..d83977e36e 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -134,10 +134,6 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
>   	prepare_repo_settings(the_repository);
>   	the_repository->settings.command_requires_full_index = 0;
> 
> -	/* implies allow_empty */
> -	if (opts->keep_redundant_commits)
> -		opts->allow_empty = 1;
> -
>   	if (cleanup_arg) {
>   		opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
>   		opts->explicit_cleanup = 1;
> diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
> index eba3c38d5a..2709cfc677 100755
> --- a/t/t3505-cherry-pick-empty.sh
> +++ b/t/t3505-cherry-pick-empty.sh
> @@ -59,6 +59,12 @@ test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' '
>   	test_must_fail git cherry-pick empty-change-branch
>   '
> 
> +test_expect_success 'cherry pick an empty non-ff commit with --keep-redundant-commits' '
> +	git checkout main &&
> +	test_must_fail git cherry-pick --keep-redundant-commits empty-change-branch 2>output &&
> +	test_grep "The previous cherry-pick is now empty" output
> +'
> +
>   test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' '
>   	git checkout main &&
>   	git cherry-pick --allow-empty empty-change-branch