Re: [PATCH 2/4] docs: Clean up `--empty` formatting in `git-rebase` and `git-am`

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jan 23, 2024, 14:24 UTC
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.

Show 18 quoted lines

> 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.

Show 8 quoted lines

> +`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