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

[Brian Lyles <brianmlyles@gmail.com>]

Hi Phillip

On Tue, Jan 23, 2024 at 8:24 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> 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?

Hm -- as you mention, this does appear to have a different meaning for
git-am(1) than it does for git-rebase(1). Regardless of the `--empty`
value passed to git-am(1), a non-empty patch that is already present
appears to error and stop.

That is an unfortunate difference. I think that my updated version of
the git-am(1) docs is still easier to read, and preserves the original
meaning. So I'm inclined to say that it's still an improvement worth
making, and perhaps my commit message should just clarify that.
Thoughts?

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

That makes sense. I had initially maintained the existing order in which
these were documented, keeping the default option first. I think that
the updated layout makes the order less relevant by making it easier to
read and identify the default anyway.

I could see alphabetical being better, though with the changes later in
this series we'd end up with the deprecated `ask` being first or
out-or-order at the end. What are your thoughts on the ideal order for
these?

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

I could see that -- I'll adjust this for v2 of the patch.

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

The two lines indicating this behavior are actually pre-existing -- I
did not change them in this patch and thus didn't even think to fact
check them.

Upon testing this, I've confirmed that you are correct about the actual
behavior. I will address this in a separate commit in v2.

Thanks,
Brian Lyles