From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 23 Jan 2024 14:24:15 GMT
Subject: Re: [PATCH 2/4] docs: Clean up `--empty` formatting in `git-rebase` and `git-am`
Message-ID: <192892be-262c-487e-bb1d-6e50c01e2d66@gmail.com>
In-Reply-To: <20240119060721.3734775-3-brianmlyles@gmail.com>

Hi Brian

On 19/01/2024 05:59, brianmlyles@gmail.com wrote:
> From: Brian Lyles <brianmlyles@gmail.com>
> 
> Both of these pages document very similar `--empty` options, but with
> different styles. This commit aims to make them more consistent.

I think that's reasonable though the options they are worded as doing 
different things. For "am" it talks about the patch being empty - i.e. a 
patch of an empty commit whereas for "rebase" the option applies to 
non-empty commits that become empty. What does "am" do if you try to 
apply a patch whose changes are already present?

If you're aiming for consistency then it would be worth listing the 
possible values in the same order for each command.


> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index b4526ca246..3ee85f6d86 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -293,13 +293,20 @@ See also INCOMPATIBLE OPTIONS below.
>   	How to handle commits that are not empty to start and are not
>   	clean cherry-picks of any upstream commit, but which become
>   	empty after rebasing (because they contain a subset of already
> -	upstream changes).  With drop (the default), commits that
> -	become empty are dropped.  With keep, such commits are kept.
> -	With ask (implied by `--interactive`), the rebase will halt when
> -	an empty commit is applied allowing you to choose whether to
> -	drop it, edit files more, or just commit the empty changes.
> +	upstream changes):
> ++
> +--
> +`drop`;;
> +	The empty commit will be dropped. This is the default behavior.

I think it would be clearer to say "The commit" - I found "The empty 
commit" confusing as the commit that is being picked isn't empty.

> +`keep`;;
> +	The empty commit will be kept.
> +`ask`;;
> +	The rebase will halt when the empty commit is applied, allowing you to
> +	choose whether to drop it, edit files more, or just commit the empty
> +	changes. This option is implied when `--interactive` is specified.
>   	Other options, like `--exec`, will use the default of drop unless
>   	`-i`/`--interactive` is explicitly specified.

Thanks for adding a bit more detail about the default, however it looks 
to me like we keep commits that become empty when --exec is specified

	if (options.empty == EMPTY_UNSPECIFIED) {
		if (options.flags & REBASE_INTERACTIVE_EXPLICIT)
			options.empty = EMPTY_STOP;
		else if (options.exec.nr > 0)
			options.empty = EMPTY_KEEP;
		else
			options.empty = EMPTY_DROP;
	}

Off the top of my head I'm not sure why or if that is a good idea.

Best Wishes

Phillip