From: phillip.wood123@gmail.com
Date: Wed, 13 Mar 2024 16:10:35 GMT
Subject: Re: [PATCH v3 7/7] cherry-pick: add `--empty` for more robust redundant commit handling
Message-ID: <d7c926ce-8a2d-4828-a3b0-3c4a9bcfe92a@gmail.com>
In-Reply-To: <20240310184602.539656-8-brianmlyles@gmail.com>

Hi Brian

On 10/03/2024 18:42, Brian Lyles wrote:
> As with git-rebase(1) and git-am(1), git-cherry-pick(1) can result in a
> commit being made redundant if the content from the picked commit is
> already present in the target history. However, git-cherry-pick(1) does
> not have the same options available that git-rebase(1) and git-am(1) have.
> 
> There are three things that can be done with these redundant commits:
> drop them, keep them, or have the cherry-pick stop and wait for the user
> to take an action. git-rebase(1) has the `--empty` option added in commit
> e98c4269c8 (rebase (interactive-backend): fix handling of commits that
> become empty, 2020-02-15), which handles all three of these scenarios.
> Similarly, git-am(1) got its own `--empty` in 7c096b8d61 (am: support
> --empty=<option> to handle empty patches, 2021-12-09).
> 
> git-cherry-pick(1), on the other hand, only supports two of the three
> possiblities: Keep the redundant commits via `--keep-redundant-commits`,
> or have the cherry-pick fail by not specifying that option. There is no
> way to automatically drop redundant commits.
> 
> In order to bring git-cherry-pick(1) more in-line with git-rebase(1) and
> git-am(1), this commit adds an `--empty` option to git-cherry-pick(1). It
> has the same three options (keep, drop, and stop), and largely behaves
> the same. The notable difference is that for git-cherry-pick(1), the
> default will be `stop`, which maintains the current behavior when the
> option is not specified.
> 
> Like the existing `--keep-redundant-commits`, `--empty=keep` will imply
> `--allow-empty`.

I think this is reasonable. git rebase defaults to keeping commits that 
start empty so now that "git cherry-pick --empty=keep" implies 
"--allow-empty" it will behave the same as "git rebase --empty=keep" as 
well as matching the behavior of "git cherry-pick --keep-redundant-commits".

> The `--keep-redundant-commits` option will be documented as a deprecated
> synonym of `--empty=keep`, and will be supported for backwards
> compatibility for the time being.
> 
> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
> ---
> 
> Differences from v2:
> 
> - `--empty=keep` will now imply `--allow-empty`, consistent with
>    `--keep-redundant-commits`. See [1] for more information.
> - Tests for persistence of the new behaviors after `--continue`, etc.
>    are more focused on user-visible behaviors rather than implementation
>    details.
> - The new empty_action enum uses -1 for unspecified instead of 0.

Thanks for reworking the tests. I've left a small comment below but this 
is looking good.

> +--empty=(drop|keep|stop)::
> +	How to handle commits being cherry-picked that are redundant with
> +	changes already in the current history.
> ++
> +--
> +`drop`;;
> +	The commit will be dropped.
> +`keep`;;
> +	The commit will be kept. Implies `--allow-empty`.
> +`stop`;;
> +	The cherry-pick will stop when the commit is applied, allowing
> +	you to examine the commit. This is the default behavior.
> +--
> ++
> +Note that this option specifies how to handle a commit that 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`.

I found this last paragraph is slightly confusing now --empty=keep 
implies --allow-empty. Maybe we could change the middle sentence to say 
something like

     With the exception of `--empty=keep` commits that were initially
     empty will cause the cherry-pick to fail.

> +	if (opts->action == REPLAY_PICK) {
> +		opts->drop_redundant_commits = (empty_opt == DROP_EMPTY_COMMIT);
> +		opts->keep_redundant_commits = opts->keep_redundant_commits || (empty_opt == KEEP_EMPTY_COMMIT);
> +	}
> +
>   	/* implies allow_empty */
>   	if (opts->keep_redundant_commits)
>   		opts->allow_empty = 1;

--empty=keep sets opts->keep_redundant_commits above so this makes it 
imply --allow-empty - good.

Best Wishes

Phillip