diff mbox series

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

Message ID 20240119060721.3734775-3-brianmlyles@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/4] sequencer: Do not require `allow_empty` for redundant commit options | expand

Commit Message

Brian Lyles Jan. 19, 2024, 5:59 a.m. UTC 
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.

In a future commit, we'll be documenting a new `--empty` option for
`git-cherry-pick`, making the consistency even more relevant.

Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
---
 Documentation/git-am.txt     | 18 ++++++++++++------
 Documentation/git-rebase.txt | 17 ++++++++++++-----
 2 files changed, 24 insertions(+), 11 deletions(-)

Comments

Phillip Wood Jan. 23, 2024, 2:24 p.m. UTC | #1 
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
Brian Lyles Jan. 27, 2024, 9:22 p.m. UTC | #2 
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
Phillip Wood Feb. 1, 2024, 2:02 p.m. UTC | #3 
Hi Brian

On 27/01/2024 21:22, Brian Lyles wrote:
> 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?

Yes I agree the change is worthwhile but I think it would benefit from 
an updated commit message.

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

Alphabetical sounds reasonable, we could sort on the non deprecated 
names with stop and ask grouped together

drop;;
     ...
keep;;
     ...
stop;;
ask;;
     ...
     `ask` is a deprecated synonym of `stop`

>>> +`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, I'd missed that those were context lines in the diff.

Best Wishes

Phillip
diff mbox series

Patch

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index e080458d6c..77df5e606a 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -67,12 +67,18 @@  OPTIONS
 	This flag will be passed down to 'git mailinfo' (see linkgit:git-mailinfo[1]).
 
 --empty=(stop|drop|keep)::
-	By default, or when the option is set to 'stop', the command
-	errors out on an input e-mail message lacking a patch
-	and stops in the middle of the current am session. When this
-	option is set to 'drop', skip such an e-mail message instead.
-	When this option is set to 'keep', create an empty commit,
-	recording the contents of the e-mail message as its log.
+	How to handle an e-mail message lacking a patch:
++
+--
+`stop`;;
+	The command will fail, stopping in the middle of the current `am`
+	session. This is the default behavior.
+`drop`;;
+	The e-mail message will be skipped.
+`keep`;;
+	An empty commit will be created, with the contents of the e-mail
+	message as its log.
+--
 
 -m::
 --message-id::
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.
+`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.
+--
 +
 Note that commits which start empty are kept (unless `--no-keep-empty`
 is specified), and commits which are clean cherry-picks (as determined