From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Thu, 18 Jan 2018 22:00:39 GMT
Subject: Re: [PATCH 10/8] [DO NOT APPLY, but improve?] rebase--interactive: introduce "stop" command
Message-ID: <nycvar.QRO.7.76.6.1801182254470.31@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
In-Reply-To: <20180118183618.39853-3-sbeller@google.com>

Hi Stefan,

On Thu, 18 Jan 2018, Stefan Beller wrote:

> Jake suggested using "x false" instead of "edit" for some corner cases.
> 
> I do prefer using "x false" for all kinds of things such as stopping
> before a commit (edit only let's you stop after a commit), and the
> knowledge that "x false" does the least amount of actions behind my back.
> 
> We should have that command as well, maybe?
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-rebase--interactive.sh |  1 +
>  sequencer.c                | 10 ++++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 3cd7446d0b..9eac53f0c5 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -166,6 +166,7 @@ l, label <label>= label current HEAD with a name
>  t, reset <label> = reset HEAD to a label
>  b, bud = reset HEAD to the revision labeled 'onto', no arguments
>  m, merge [<label-or-commit>]* = create a merge commit using a given commit's message
> +y, stay = stop for  shortcut for
>  
>  These lines can be re-ordered; they are executed from top to bottom.
>  " | git stripspace --comment-lines >>"$todo"
> diff --git a/sequencer.c b/sequencer.c
> index 2b4e6b1232..4b3b9fe59d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -782,6 +782,7 @@ enum todo_command {
>  	TODO_RESET,
>  	TODO_BUD,
>  	TODO_MERGE,
> +	TODO_STOP,

I see that your original idea was "stop", but then you probably realized
that there would be no good abbreviation for that, and changed your mind.

Personally, I would have called it `break`...

> @@ -2407,6 +2415,8 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>  				/* `current` will be incremented below */
>  				todo_list->current = -1;
>  			}
> +		} else if (item->command == TODO_STOP) {
> +			todo_list->current = -1;

That is incorrect, it will most likely write an unexpected `done` file.

Did you mean `return 0` instead?

Ciao,
Dscho