From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 01 Feb 2024 14:02:28 GMT
Subject: Re: [PATCH 2/4] docs: Clean up `--empty` formatting in `git-rebase` and `git-am`
Message-ID: <0ed4a9b2-2318-4424-8173-42c0e61c8dda@gmail.com>
In-Reply-To: <CAHPHrSdOVoBPR9vJou_Bxmq=4QW_z6nhnzxfmZ1Am0i-GJuz4g@mail.gmail.com>

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