From: phillip.wood123@gmail.com
Date: Thu, 22 Feb 2024 16:35:14 GMT
Subject: Re: [PATCH v2 5/8] sequencer: do not require `allow_empty` for redundant commit options
Message-ID: <84db20bc-94d0-43be-b4e0-f3f9245de63f@gmail.com>
In-Reply-To: <20240210074859.552497-6-brianmlyles@gmail.com>

Hi Brian

On 10/02/2024 07:43, Brian Lyles wrote:
> A consumer of the sequencer that wishes to take advantage of either the
> `keep_redundant_commits` or `drop_redundant_commits` feature must also
> specify `allow_empty`. However, these refer to two distinct types of
> empty commits:
> 
> - `allow_empty` refers specifically to commits which start empty
> - `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
> 
> Conceptually, there is no reason that the behavior for handling one of
> these should be entangled with the other. It is particularly unintuitive
> to require `allow_empty` in order for `drop_redundant_commits` to have
> an effect: in order to prevent redundant commits automatically,
> initially-empty commits would need to be kept automatically as well.
> 
> Instead, rewrite the `allow_empty()` logic to remove the over-arching
> requirement that `allow_empty` be specified in order to reach any of the
> keep/drop behaviors. Only if the commit was originally empty will
> `allow_empty` have an effect.
> 
> Note that no behavioral changes should result from this commit -- it
> merely sets the stage for future commits.

Thanks for clarifying that. I think splitting this change out is a good 
idea. The patch looks good.

Best Wishes

Phillip

> In one such future commit, an
> `--empty` option will be added to git-cherry-pick(1), meaning that
> `drop_redundant_commits` will be used by that command.
> 
> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
> ---
> 
> This is the first half of the first commit[1] in v1, which has now been
> split up. While the next commit may be considered somewhat
> controversial, this part of the change should not be.
> 
> [1]: https://lore.kernel.org/git/20240119060721.3734775-2-brianmlyles@gmail.com/
> 
>   sequencer.c | 23 +++++++----------------
>   1 file changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index b1b19512de..3f41863dae 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1725,34 +1725,25 @@ static int allow_empty(struct repository *r,
>   	int index_unchanged, originally_empty;
> 
>   	/*
> -	 * Four cases:
> +	 * For a commit that is initially empty, allow_empty determines if it
> +	 * should be kept or not
>   	 *
> -	 * (1) we do not allow empty at all and error out.
> -	 *
> -	 * (2) we allow ones that were initially empty, and
> -	 *     just drop the ones that become empty
> -	 *
> -	 * (3) we allow ones that were initially empty, but
> -	 *     halt for the ones that become empty;
> -	 *
> -	 * (4) we allow both.
> +	 * For a commit that becomes empty, keep_redundant_commits and
> +	 * drop_redundant_commits determine whether the commit should be kept or
> +	 * dropped. If neither is specified, halt.
>   	 */
> -	if (!opts->allow_empty)
> -		return 0; /* let "git commit" barf as necessary */
> -
>   	index_unchanged = is_index_unchanged(r);
>   	if (index_unchanged < 0)
>   		return index_unchanged;
>   	if (!index_unchanged)
>   		return 0; /* we do not have to say --allow-empty */
> 
> -	if (opts->keep_redundant_commits)
> -		return 1;
> -
>   	originally_empty = is_original_commit_empty(commit);
>   	if (originally_empty < 0)
>   		return originally_empty;
>   	if (originally_empty)
> +		return opts->allow_empty;
> +	else if (opts->keep_redundant_commits)
>   		return 1;
>   	else if (opts->drop_redundant_commits)
>   		return 2;