Re: [PATCH] git-rebase.sh: handle keep-empty like all other options
On 11/06/18 17:08, Elijah Newren wrote: > Hi Phillip, > > On Mon, Jun 11, 2018 at 8:16 AM, Phillip Wood > wrote: >> On 11/06/18 15:42, Elijah Newren wrote: > >>> 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. > > Ah, yes, you are right; I was implicitly thinking about cases where > the commit became empty rather than when the commit started > empty...and mis-read --keep-empty (which as I learned now is only for > started-empty cases). > >> 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. > > The am backend does not halt the rebase; it simply drops the commit > and says "No changes -- Patch already applied." > > It has always seemed jarring and confusing to me that one rebase mode > stops and asks the user what to do and the other assumes. I think > both should have the same default (and have options to pick the > alternate behavior). > > I'm less certain what the default should be, though. I'm not really sure either, on the one hand most of the time it is convenient for rebase just to skip over it, on the other if you have some important information in the commit message or a note then maybe you want rebase to stop so you can selvage that information. > >> cherry-pick has >> '--keep-redundant-commits' for this case but that has never been added to >> rebase. On path forwards is to always stop, and implement --keep-redundant-commits for rebase. That would be very easy for interactive rebases as it shares the same code as cherry-pick. I've just had a quick look at builtin/am.c and I think it would be fairly straightforward to add some code to check if it's rebasing and stop if the patch is already applied. Best Wishes Phillip > Thanks for the pointer. >
Re: [PATCH] git-rebase.sh: handle keep-empty like all other options
Hi Phillip, On Mon, Jun 11, 2018 at 8:16 AM, Phillip Wood wrote: > On 11/06/18 15:42, Elijah Newren wrote: >> 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. Ah, yes, you are right; I was implicitly thinking about cases where the commit became empty rather than when the commit started empty...and mis-read --keep-empty (which as I learned now is only for started-empty cases). > 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. The am backend does not halt the rebase; it simply drops the commit and says "No changes -- Patch already applied." It has always seemed jarring and confusing to me that one rebase mode stops and asks the user what to do and the other assumes. I think both should have the same default (and have options to pick the alternate behavior). I'm less certain what the default should be, though. > cherry-pick has > '--keep-redundant-commits' for this case but that has never been added to > rebase. Thanks for the pointer.
Re: [PATCH] git-rebase.sh: handle keep-empty like all other options
Hi Elijah
On 11/06/18 15:42, Elijah Newren wrote:
Hi Phillip,
On Sun, Jun 10, 2018 at 12:26 PM, Phillip Wood
wrote:
On 07/06/18 06:07, Elijah Newren wrote:
Signed-off-by: Elijah Newren
---
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
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?
Re: [PATCH] git-rebase.sh: handle keep-empty like all other options
Hi Phillip,
On Sun, Jun 10, 2018 at 12:26 PM, Phillip Wood
wrote:
> On 07/06/18 06:07, Elijah Newren wrote:
>>
>> Signed-off-by: Elijah Newren
>> ---
>> 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. But if we want to make all three available to
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?
Re: [PATCH] git-rebase.sh: handle keep-empty like all other options
Hi Elijah On 07/06/18 06:07, Elijah Newren wrote: Signed-off-by: Elijah Newren --- 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. Best Wishes Phillip ;; --allow-empty-message) allow_empty_message=--allow-empty-message @@ -480,11 +481,6 @@ then test -z "$interactive_rebase" && interactive_rebase=implied fi -if test -n "$keep_empty" -then - test -z "$interactive_rebase" && interactive_rebase=implied -fi - if test -n "$interactive_rebase" then type=interactive
