Re: [PATCH] git-rebase.sh: handle keep-empty like all other options

From: Phillip Wood <phillip.wood@talktalk.net>
Date: Jun 11, 2018, 15:16 UTC
Message-ID: <7e3d71e6-ae97-f47b-382c-085f6a281792@talktalk.net>
In-Reply-To: <CABPp-BF7POrGTRbVOZ-AGjRHKcpExMQcA4uSPvrRONbnG2CE8w@mail.gmail.com>

Hi Elijah

On 11/06/18 15:42, Elijah Newren wrote:

Show 40 quoted lines

> Hi Phillip,
> 
> On Sun, Jun 10, 2018 at 12:26 PM, Phillip Wood
> <phillip.wood@talktalk.net> wrote:
>> On 07/06/18 06:07, Elijah Newren wrote:
>>>
>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>> ---
>>>    git-rebase.sh | 6 +-----
>>>    1 file changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/git-rebase.sh b/git-rebase.sh
>>> index 40be59ecc4..a56b286372 100755
>>> --- a/git-rebase.sh
>>> +++ b/git-rebase.sh
>>> @@ -276,6 +276,7 @@ do
>>>                  ;;
>>>          --keep-empty)
>>>                  keep_empty=yes
>>> +               test -z "$interactive_rebase" &&
>>> interactive_rebase=implied
>>
>>
>> I think you need to wait until all the options have been parsed before
>> setting the implied interactive rebase in case the user specifies has
>> '--keep-empty' in an alias and specifies '--no-keep-empty' with some am
>> options on the command line.
> 
> Ah, indeed you are right.  Let's drop this patch then.
> 
> However, we have a bigger problem with empty commits, in that there
> are really three modes rather than two:
>    * Automatically drop empty commits (default for am-based rebase)
>    * Automatically keep empty commits (as done with --keep-empty)
>    * Halt the rebase and tell the user how to specify if they want to
> keep it (default for interactive rebases)
> 
> Currently, only the first option is available to am-based rebases, and
> only the second two options are available to interactive-based
> rebases.

I'm not sure that's the case, my understanding is that for an interactive rebase unless you give '--keep-empty' then any empty commits will be commented out in the todo list and therefore dropped unless they're uncommented when editing the list. The third case happens when a commit becomes empty when it's rebased (i.e. the original commit is not empty), I'm not sure what the am backend does for this. cherry-pick has '--keep-redundant-commits' for this case but that has never been added to rebase.

Best Wishes

Phillip

But if we want to make all three available to

Show 8 quoted lines

> interactive-based rebases, what should the command line option look
> like?  --empty={drop,ask,keep}?
> 
> (And deprecate but continue to support --[no-]keep-empty?)
> 
> And should the two rebase modes really have a different default?  What
> should the default be?
>