[PATCH] git-rebase--interactive.sh: Add new command "shell"

[PATCH] git-rebase--interactive.sh: Add new command "shell"

[Kevin Ballard]

Add a new command "shell", which takes an option commit. It simply exits
to the shell with the commit (if given) and a message telling the user how
to resume the rebase. This is effectively the same thing as "x false" but
much friendlier to the user.

Signed-off-by: Kevin Ballard <kevin@sb.org>
---
I discovered the need for this when I wanted to edit a commit, but apply
a fixup first. The only way with the existing tools was an exec command
that fails (e.g. "x false").

 git-rebase--interactive.sh |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9121bb6..3501757 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -566,6 +566,26 @@ do_next () {
 			exit 1
 		fi
 		;;
+	!|"shell")
+		read -r command comment < "$TODO"
+		mark_action_done
+		# can't use $sha1 here for same reason as "exec"
+		line=$(git rev-list --pretty=oneline -1 --abbrev-commit --abbrev=7 HEAD)
+		sha1="${line%% *}"
+		rest="${line#* }"
+		echo "$sha1" > "$DOTEST"/stopped-sha
+		warn "Stopped at $sha1... $rest"
+		if test -n "$comment"; then
+			warn
+			warn "	$comment"
+		fi
+		warn
+		warn "Once you are ready to continue, run"
+		warn
+		warn "	git rebase --continue"
+		warn
+		exit 0
+		;;
 	*)
 		warn "Unknown command: $command $sha1 $rest"
 		if git rev-parse --verify -q "$sha1" >/dev/null
@@ -1007,6 +1027,7 @@ first and then run 'git rebase --continue' again."
 #  s, squash = use commit, but meld into previous commit
 #  f, fixup = like "squash", but discard this commit's log message
 #  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
+#  !, shell = Exit to the shell
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
-- 
1.7.3.2.202.g3b863.dirty

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Kevin Ballard]

On Nov 3, 2010, at 10:17 PM, Kevin Ballard wrote:

> Add a new command "shell", which takes an option commit. It simply exits
> to the shell with the commit (if given) and a message telling the user how
> to resume the rebase. This is effectively the same thing as "x false" but
> much friendlier to the user.

That was supposed to say "optional comment", not "option commit". And again
below, "comment" not "commit".

-Kevin Ballard

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Matthieu Moy]

Kevin Ballard <kevin@sb.org> writes:

> Add a new command "shell", which takes an option commit. It simply exits
> to the shell with the commit (if given) and a message telling the user how
> to resume the rebase.

"shell" sounds like you're going to execute something in a shell, not
that you're going back to the shell. Looking at the commit message, I
thought you had missed the "exec" command and re-implemented it.

What about "pause", abbreviated as "p" for the command name?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Kevin Ballard]

On Nov 4, 2010, at 1:42 AM, Matthieu Moy wrote:

> Kevin Ballard <kevin@sb.org> writes:
> 
>> Add a new command "shell", which takes an option commit. It simply exits
>> to the shell with the commit (if given) and a message telling the user how
>> to resume the rebase.
> 
> "shell" sounds like you're going to execute something in a shell, not
> that you're going back to the shell. Looking at the commit message, I
> thought you had missed the "exec" command and re-implemented it.
> 
> What about "pause", abbreviated as "p" for the command name?

That sounds like a reasonable suggestion, except "p" is already taken by "pick".
I suppose this command could simply omit the short version.

---8<---
Subject: git-rebase--interactive.sh: Add new command "pause"

Add a new command "pause", which takes an optional comment. It simply exits
to the shell with the comment (if given) and a message telling the user how
to resume the rebase. This is effectively the same thing as "x false" but
much friendlier to the user.

Signed-off-by: Kevin Ballard <kevin@sb.org>
---
 git-rebase--interactive.sh |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a27952d..e29fd91 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -566,6 +566,26 @@ do_next () {
 			exit 1
 		fi
 		;;
+	pause)
+		read -r command comment < "$TODO"
+		mark_action_done
+		# can't use $sha1 here for same reason as "exec"
+		line=$(git rev-list --pretty=oneline -1 --abbrev-commit --abbrev=7 HEAD)
+		sha1="${line%% *}"
+		rest="${line#* }"
+		echo "$sha1" > "$DOTEST"/stopped-sha
+		warn "Stopped at $sha1... $rest"
+		if test -n "$comment"; then
+			warn
+			warn "	$comment"
+		fi
+		warn
+		warn "Once you are ready to continue, run"
+		warn
+		warn "	git rebase --continue"
+		warn
+		exit 0
+		;;
 	*)
 		warn "Unknown command: $command $sha1 $rest"
 		if git rev-parse --verify -q "$sha1" >/dev/null
@@ -998,6 +1018,7 @@ first and then run 'git rebase --continue' again."
 #  s, squash = use commit, but meld into previous commit
 #  f, fixup = like "squash", but discard this commit's log message
 #  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
+#  pause = exit to the shell
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
-- 
1.7.3.2.195.gc69dde

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 4, 2010 at 09:53, Kevin Ballard <kevin@sb.org> wrote:
> On Nov 4, 2010, at 1:42 AM, Matthieu Moy wrote:
>
>> Kevin Ballard <kevin@sb.org> writes:
>>
>>> Add a new command "shell", which takes an option commit. It simply exits
>>> to the shell with the commit (if given) and a message telling the user how
>>> to resume the rebase.
>>
>> "shell" sounds like you're going to execute something in a shell, not
>> that you're going back to the shell. Looking at the commit message, I
>> thought you had missed the "exec" command and re-implemented it.
>>
>> What about "pause", abbreviated as "p" for the command name?
>
> That sounds like a reasonable suggestion, except "p" is already taken by "pick".
> I suppose this command could simply omit the short version.

I thought "shell" would do exactly what your patch does. And it has
the "s" short version.

So +1 for "shell" from me and -1 for "pause", which *does* confuse me.
I'd expect that
to just sleep for a few seconds.

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Kevin Ballard]

On Nov 4, 2010, at 2:23 AM, Ævar Arnfjörð Bjarmason wrote:

> On Thu, Nov 4, 2010 at 09:53, Kevin Ballard <kevin@sb.org> wrote:
>> On Nov 4, 2010, at 1:42 AM, Matthieu Moy wrote:
>> 
>>> Kevin Ballard <kevin@sb.org> writes:
>>> 
>>>> Add a new command "shell", which takes an option commit. It simply exits
>>>> to the shell with the commit (if given) and a message telling the user how
>>>> to resume the rebase.
>>> 
>>> "shell" sounds like you're going to execute something in a shell, not
>>> that you're going back to the shell. Looking at the commit message, I
>>> thought you had missed the "exec" command and re-implemented it.
>>> 
>>> What about "pause", abbreviated as "p" for the command name?
>> 
>> That sounds like a reasonable suggestion, except "p" is already taken by "pick".
>> I suppose this command could simply omit the short version.
> 
> I thought "shell" would do exactly what your patch does. And it has
> the "s" short version.
> 
> So +1 for "shell" from me and -1 for "pause", which *does* confuse me.
> I'd expect that
> to just sleep for a few seconds.

"s" is actually taken by "squash". That's why my original patch used "!",
though a user might actually expect "!" to do what "x" does.

-Kevin Ballard

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 4, 2010 at 10:25, Kevin Ballard <kevin@sb.org> wrote:
>> I thought "shell" would do exactly what your patch does. And it has
>> the "s" short version.
>>
>> So +1 for "shell" from me and -1 for "pause", which *does* confuse me.
>> I'd expect that
>> to just sleep for a few seconds.
>
> "s" is actually taken by "squash". That's why my original patch used "!",
> though a user might actually expect "!" to do what "x" does.

Indeed, eek!

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Erik Faye-Lund]

On Thu, Nov 4, 2010 at 6:17 AM, Kevin Ballard <kevin@sb.org> wrote:
> Add a new command "shell", which takes an option commit. It simply exits
> to the shell with the commit (if given) and a message telling the user how
> to resume the rebase. This is effectively the same thing as "x false" but
> much friendlier to the user.
>

I'm sorry if I'm missing something, but how is this different from "edit"?

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Kevin Ballard]

On Nov 4, 2010, at 2:36 AM, Erik Faye-Lund wrote:

> On Thu, Nov 4, 2010 at 6:17 AM, Kevin Ballard <kevin@sb.org> wrote:
>> Add a new command "shell", which takes an option commit. It simply exits
>> to the shell with the commit (if given) and a message telling the user how
>> to resume the rebase. This is effectively the same thing as "x false" but
>> much friendlier to the user.
>> 
> 
> I'm sorry if I'm missing something, but how is this different from "edit"?

Edit cherry-picks a commit, then exits to the shell. I needed to exit to the
shell without cherry-picking a commit. As stated in the comments above the
diffstat on the patch, the original use case here was something along the
lines of

  edit 12345 some commit
  fixup 23456 another commit
  shell I want to amend the commit after the fixup

-Kevin Ballard

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Johannes Sixt]

Am 11/4/2010 9:53, schrieb Kevin Ballard:
> +#  pause = exit to the shell

The short form could be just the dash -. I'd describe the command as

#  pause,- = interrupt automatic processing of commits

or similar to avoid the term "shell".

-- Hannes

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Yann Dirson]

>> I'm sorry if I'm missing something, but how is this different from
>> "edit"?
>
>Edit cherry-picks a commit, then exits to the shell. I needed to exit
>to the shell without cherry-picking a commit.

Indeed, before "x false" was available, I had found out that "edit"
without an argument fails with a harmless error and indeed achieves that
"pause" mechanism which was really missing.

What about just fixing this so we can use "edit" ?  Do we really need
another command here ?

-- 
Yann Dirson - Bertin Technologies

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Erik Faye-Lund]

On Thu, Nov 4, 2010 at 11:25 AM, Yann Dirson <dirson@bertin.fr> wrote:
>>> I'm sorry if I'm missing something, but how is this different from
>>> "edit"?
>>
>>Edit cherry-picks a commit, then exits to the shell. I needed to exit
>>to the shell without cherry-picking a commit.
>

Then you do "edit" on the preceding commit instead, no?

> Indeed, before "x false" was available, I had found out that "edit"
> without an argument fails with a harmless error and indeed achieves that
> "pause" mechanism which was really missing.
>
> What about just fixing this so we can use "edit" ?  Do we really need
> another command here ?
>

Having an parameter-less "edit" would indeed be a bit more convenient.

Re: Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Eric Raible]

On 11:59 AM, Yann Dirson wrote:
>>> I'm sorry if I'm missing something, but how is this different from
>>> "edit"?
>>
>> Edit cherry-picks a commit, then exits to the shell. I needed to exit
>> to the shell without cherry-picking a commit.
> 
> Indeed, before "x false" was available, I had found out that "edit"
> without an argument fails with a harmless error and indeed achieves that
> "pause" mechanism which was really missing.
> 
> What about just fixing this so we can use "edit" ?  Do we really need
> another command here ?

FWIW: +1 for edit.

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Matthieu Moy]

Eric Raible <raible@nextest.com> writes:

> On 11:59 AM, Yann Dirson wrote:
>>>> I'm sorry if I'm missing something, but how is this different from
>>>> "edit"?
>>>
>>> Edit cherry-picks a commit, then exits to the shell. I needed to exit
>>> to the shell without cherry-picking a commit.
>> 
>> Indeed, before "x false" was available, I had found out that "edit"
>> without an argument fails with a harmless error and indeed achieves that
>> "pause" mechanism which was really missing.
>> 
>> What about just fixing this so we can use "edit" ?  Do we really need
>> another command here ?
>
> FWIW: +1 for edit.

I like the idea (and I won't fight for my "pause" proposal if others
don't find it intuitive), but I'm wondering how to write the quick
documentation (in the todo-list). And if we don't find a concise way
to document it, it may reveal that it's a bad idea ...

Maybe:

#  e <commit>, edit <commit> = use commit, but stop for amending
#  e, edit = stop for amending

but I find this rather ugly.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Eric Raible]

On 11/4/2010 10:34 AM, Matthieu Moy wrote:

> ... And if we don't find a concise way
> to document it, it may reveal that it's a bad idea ...
> 
> Maybe:
> 
> #  e <commit>, edit <commit> = use commit, but stop for amending
> #  e, edit = stop for amending
> 
> but I find this rather ugly.

How about:

#  e [<commit>], edit [<commit>] = use commit (if present) but pause to amend

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Jonathan Nieder]

Matthieu Moy wrote:

> #  e <commit>, edit <commit> = use commit, but stop for amending
> #  e, edit = stop for amending

Before it said:

# Commands:
#  p, pick = use commit
#  r, reword = use commit, but edit the commit message
#  e, edit = use commit, but stop for amending
#  s, squash = use commit, but meld into previous commit
#  f, fixup = like "squash", but discard this commit's log message
#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
#
# If you remove a line here THAT COMMIT WILL BE LOST.
# However, if you remove everything, the rebase will be aborted.

How about:

# Commands:
#  p, pick = use commit
#  r, reword = use commit, but edit the commit message
#  e, edit = use commit, but stop for amending
#  s, squash = use commit, but meld into previous commit
#  f, fixup = like "squash", but discard this commit's log message
#  x, exec = run command using shell, and stop if it fails
#
# The argument to edit is optional; if left out, it means to
# stop to examine or amend the previous commit.
#
# If you remove a line here, THAT COMMIT WILL BE LOST.
# However, if you remove everything, the rebase will be aborted.
# Use the noop command if you really want to remove all commits.

Ciao,
Jonathan
who is happy to help paint today

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Yann Dirson]

On Thu, Nov 04, 2010 at 01:10:20PM -0500, Jonathan Nieder wrote:
> How about:
> 
> # Commands:
> #  p, pick = use commit
> #  r, reword = use commit, but edit the commit message
> #  e, edit = use commit, but stop for amending
> #  s, squash = use commit, but meld into previous commit
> #  f, fixup = like "squash", but discard this commit's log message
> #  x, exec = run command using shell, and stop if it fails
> #
> # The argument to edit is optional; if left out, it means to
> # stop to examine or amend the previous commit.
> #
> # If you remove a line here, THAT COMMIT WILL BE LOST.
> # However, if you remove everything, the rebase will be aborted.
> # Use the noop command if you really want to remove all commits.

That may be too far from the "edit" line, although I do like the idea
of mentionning other uses than "amend".

Eric Raible suggested:
> How about:
>
> #  e [<commit>], edit [<commit>] = use commit (if present) but pause to amend

Other commands do not mention commit (or other things) as a synopsis would.
What about:

#  e, edit = use commit (if specified) but pause to amend/examine/test

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Eric Raible]

On 11/4/2010 1:53 PM, Yann Dirson wrote:

> Eric Raible suggested:
>> How about:
>>
>> #  e [<commit>], edit [<commit>] = use commit (if present) but pause to amend
> 
> Other commands do not mention commit (or other things) as a synopsis would.
> What about:
> 
> #  e, edit = use commit (if specified) but pause to amend/examine/test
> .

I like that color better.

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Kevin Ballard]

On Nov 4, 2010, at 1:53 PM, Yann Dirson wrote:

> Eric Raible suggested:
>> How about:
>> 
>> #  e [<commit>], edit [<commit>] = use commit (if present) but pause to amend
> 
> Other commands do not mention commit (or other things) as a synopsis would.
> What about:
> 
> #  e, edit = use commit (if specified) but pause to amend/examine/test

I like this. My only remaining concern is the original "shell" version let you
put in a comment (though this was not yet documented) that would be printed when
you were sent back to the shell. This was a useful reminder as to what step you
were on. But when we overload "edit", this functionality is lost. I won't fight
for it if nobody else here thinks it's worthwhile, but I did want to point that
out.

-Kevin Ballard

[PATCHv2] git-rebase--interactive.sh: extend "edit" command to be more useful

[Kevin Ballard]

Extend the "edit" command to simply stop for editing if no sha1 is
given. This behaves the same as "x false" but is a bit friendlier
for the user.

Signed-off-by: Kevin Ballard <kevin@sb.org>
---
 git-rebase--interactive.sh |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9121bb6..a8e00a2 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -477,10 +477,19 @@ do_next () {
 		comment_for_reflog edit
 
 		mark_action_done
-		pick_one $sha1 ||
-			die_with_patch $sha1 "Could not apply $sha1... $rest"
-		echo "$sha1" > "$DOTEST"/stopped-sha
-		make_patch $sha1
+		if test -n "$sha1"; then
+			pick_one $sha1 ||
+				die_with_patch $sha1 "Could not apply $sha1... $rest"
+			echo "$sha1" > "$DOTEST"/stopped-sha
+			make_patch $sha1
+		else
+			# we just want to exit to the shell
+			# we don't have a $sha1 or $rest, so recreate that
+			line=$(git rev-list --pretty=oneline -1 --abbrev-commit --abbrev=7 HEAD)
+			sha1="${line%% *}"
+			rest="${line#* }"
+			echo "$sha1" > "$DOTEST"/stopped-sha
+		fi
 		git rev-parse --verify HEAD > "$AMEND"
 		warn "Stopped at $sha1... $rest"
 		warn "You can amend the commit now, with"
@@ -1003,7 +1012,7 @@ first and then run 'git rebase --continue' again."
 # Commands:
 #  p, pick = use commit
 #  r, reword = use commit, but edit the commit message
-#  e, edit = use commit, but stop for amending
+#  e, edit = use commit (if specified), but pause to amend/examine/test
 #  s, squash = use commit, but meld into previous commit
 #  f, fixup = like "squash", but discard this commit's log message
 #  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
-- 
1.7.3.2.203.gd142e

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Johannes Sixt]

Am 11/4/2010 21:53, schrieb Yann Dirson:
> #  e, edit = use commit (if specified) but pause to amend/examine/test

That's fine. But how would you determine the "if specified"? In
particular, I like to replace the commit subject by instructions that
remember me what I intended to do after rebase stopped, and I would like
to do that in either of these two forms:

e merge foo-topic!

or

e - merge foo-topic!

-- Hannes

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Kevin Ballard]

On Nov 5, 2010, at 12:33 AM, Johannes Sixt wrote:

> Am 11/4/2010 21:53, schrieb Yann Dirson:
>> #  e, edit = use commit (if specified) but pause to amend/examine/test
> 
> That's fine. But how would you determine the "if specified"? In
> particular, I like to replace the commit subject by instructions that
> remember me what I intended to do after rebase stopped, and I would like
> to do that in either of these two forms:
> 
> e merge foo-topic!
> 
> or
> 
> e - merge foo-topic!

This was my complaint about overriding "edit" as well, but I kind of like
your second example. Can you come up with a simple way to explain it in
the instructions?

-Kevin Ballard

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Junio C Hamano]

Yann Dirson <ydirson@free.fr> writes:

> #  e, edit = use commit (if specified) but pause to amend/examine/test

When an end user is given

    pick one
    pick two
    pick three
    ...

and told the above, would it be crystal clear that, if he changed the insn
sheet to

    pick one
    edit
    pick three
    ...

then he will _lose_ the change made by foo, or will the user come back
here and complain that a precious change "two" is lost and it is git's
fault?

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Kevin Ballard]

On Nov 8, 2010, at 10:31 AM, Junio C Hamano wrote:

> Yann Dirson <ydirson@free.fr> writes:
> 
>> #  e, edit = use commit (if specified) but pause to amend/examine/test
> 
> When an end user is given
> 
>    pick one
>    pick two
>    pick three
>    ...
> 
> and told the above, would it be crystal clear that, if he changed the insn
> sheet to
> 
>    pick one
>    edit
>    pick three
>    ...
> 
> then he will _lose_ the change made by foo, or will the user come back
> here and complain that a precious change "two" is lost and it is git's
> fault?

On the one hand, once someone understands what the todo list is actually
doing, then it should be instantly obvious that removing the reference to
a commit will remove that commit entirely. On the other hand, I agree it
may be confusing to new git users (or new rebase users). Do you have an
alternative solution in mind?

-Kevin Ballard

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Yann Dirson]

On Mon, Nov 08, 2010 at 01:49:44PM -0800, Kevin Ballard wrote:
> On Nov 8, 2010, at 10:31 AM, Junio C Hamano wrote:
> 
> > Yann Dirson <ydirson@free.fr> writes:
> > 
> >> #  e, edit = use commit (if specified) but pause to amend/examine/test
> > 
> > When an end user is given
> > 
> >    pick one
> >    pick two
> >    pick three
> >    ...
> > 
> > and told the above, would it be crystal clear that, if he changed the insn
> > sheet to
> > 
> >    pick one
> >    edit
> >    pick three
> >    ...
> > 
> > then he will _lose_ the change made by foo, or will the user come back
> > here and complain that a precious change "two" is lost and it is git's
> > fault?
> 
> On the one hand, once someone understands what the todo list is actually
> doing, then it should be instantly obvious that removing the reference to
> a commit will remove that commit entirely. On the other hand, I agree it
> may be confusing to new git users (or new rebase users). Do you have an
> alternative solution in mind?

Maybe restating in an explanatory paragraph something like:

|Keep in mind that any commit in the original todo list, that would
|not be there after your edits, would not be included in the resulting
|rebased branch.  In case you realize afterwards that you need such a
|commit, you can still access it as an ancestor of @{1}, see
|git-reflog(1) for details.

Maybe we could list a copy of the todo list in the comments, as a
reference for double-checking.  Such a list could even be used for a
final check before applying, that would ask confirmation if the set of
patches has changed, and offer to edit again.  The same config item
(eg. advice.interactiveRebase ?) could be used to hide the note and
the check.

Now making "rebase -i" possibly interactive may cause problems, for
any porcelain scripts above it.  Not sure it'd be the way to do it.
Maybe add a "check" command to be inserted at bottom of todo list to
activate it, that would be here by default but commented out ?

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Jonathan Nieder]

Yann Dirson wrote:

> |Keep in mind that any commit in the original todo list, that would
> |not be there after your edits, would not be included in the resulting
> |rebased branch.  In case you realize afterwards that you need such a
> |commit, you can still access it as an ancestor of @{1}, see
> |git-reflog(1) for details.

Do you mean @{-1}?

> Maybe we could list a copy of the todo list in the comments, as a
> reference for double-checking.  Such a list could even be used for a
> final check before applying, that would ask confirmation if the set of
> patches has changed, and offer to edit again.  The same config item
> (eg. advice.interactiveRebase ?) could be used to hide the note and
> the check.

Mm, but intentionally dropping commits is common, no?

What would be nice is to be able to do

	git rebase --change-of-plans

and somehow get my editor of choice to open with the original todo
list (read-only) and the current todo list (read/write).

Well, a person can dream. :)

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Kevin Ballard]

On Nov 9, 2010, at 5:42 PM, Jonathan Nieder wrote:

> Yann Dirson wrote:
> 
>> |Keep in mind that any commit in the original todo list, that would
>> |not be there after your edits, would not be included in the resulting
>> |rebased branch.  In case you realize afterwards that you need such a
>> |commit, you can still access it as an ancestor of @{1}, see
>> |git-reflog(1) for details.
> 
> Do you mean @{-1}?

@{-1} is the previously-checked-out branch. @{1} is the previous commit
that the current branch was pointing to. I believe @{1} is correct here.

>> Maybe we could list a copy of the todo list in the comments, as a
>> reference for double-checking.  Such a list could even be used for a
>> final check before applying, that would ask confirmation if the set of
>> patches has changed, and offer to edit again.  The same config item
>> (eg. advice.interactiveRebase ?) could be used to hide the note and
>> the check.
> 
> Mm, but intentionally dropping commits is common, no?
> 
> What would be nice is to be able to do
> 
> 	git rebase --change-of-plans
> 
> and somehow get my editor of choice to open with the original todo
> list (read-only) and the current todo list (read/write).
> 
> Well, a person can dream. :)

Not a bad idea. It would be especially nice if you could then selectively
roll back to the state after previous entries in your todo list so you
could change something you've done without having to start all over again.

-Kevin Ballard

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Jonathan Nieder]

Junio C Hamano wrote:
> Yann Dirson <ydirson@free.fr> writes:

>> #  e, edit = use commit (if specified) but pause to amend/examine/test
[...]
>                     would it be crystal clear that, if he changed the insn
> sheet to
> 
>     pick one
>     edit
>     pick three
>     ...
> 
> then he will _lose_ the change made by foo, or will the user come back
> here and complain that a precious change "two" is lost and it is git's
> fault?

If we explain it clearly then I think yes, the end user would not
be confused.

The above description (that starts with "e, edit") looks more like a
reminder than a full explanation.  Can we rely on the perplexed
operator to read the text after the command list?

If so, some trailing explanation[1] might help.

# Commands:
#  p, pick = use commit
#  r, reword = use commit, but edit the commit message
#  e, edit = use commit (if specified), but stop to amend/examine/test
#  s, squash = use commit, but meld into previous commit
#  f, fixup = like "squash", but discard this commit's log message
#  x, exec = run command using shell, and stop if it fails
#
# The argument to edit is optional; if left out or equal to "-",
# it means to stop to examine or amend the previous commit.
#
# If you remove a line here, THAT COMMIT WILL BE LOST.
# However, if you remove everything, the rebase will be aborted.
# Use the noop command if you really want to remove all commits.

[1] http://thread.gmane.org/gmane.comp.version-control.git/160691/focus=160742

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Jonathan Nieder]

Kevin Ballard wrote:
> On Nov 9, 2010, at 5:42 PM, Jonathan Nieder wrote:
>> Yann Dirson wrote:

>>> |Keep in mind that any commit in the original todo list, that would
>>> |not be there after your edits, would not be included in the resulting
>>> |rebased branch.  In case you realize afterwards that you need such a
>>> |commit, you can still access it as an ancestor of @{1}, see
>>> |git-reflog(1) for details.
>> 
>> Do you mean @{-1}?
>
> @{-1} is the previously-checked-out branch. @{1} is the previous commit
> that the current branch was pointing to. I believe @{1} is correct here.

Ah, this is after a successful rebase, so @{1} is a synonym for ORIG_HEAD.
Sorry for the noise.

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Kevin Ballard]

On Nov 9, 2010, at 5:53 PM, Jonathan Nieder wrote:

> Junio C Hamano wrote:
>> Yann Dirson <ydirson@free.fr> writes:
> 
>>> #  e, edit = use commit (if specified) but pause to amend/examine/test
> [...]
>>                    would it be crystal clear that, if he changed the insn
>> sheet to
>> 
>>    pick one
>>    edit
>>    pick three
>>    ...
>> 
>> then he will _lose_ the change made by foo, or will the user come back
>> here and complain that a precious change "two" is lost and it is git's
>> fault?
> 
> If we explain it clearly then I think yes, the end user would not
> be confused.
> 
> The above description (that starts with "e, edit") looks more like a
> reminder than a full explanation.  Can we rely on the perplexed
> operator to read the text after the command list?
> 
> If so, some trailing explanation[1] might help.
> 
> # Commands:
> #  p, pick = use commit
> #  r, reword = use commit, but edit the commit message
> #  e, edit = use commit (if specified), but stop to amend/examine/test
> #  s, squash = use commit, but meld into previous commit
> #  f, fixup = like "squash", but discard this commit's log message
> #  x, exec = run command using shell, and stop if it fails
> #
> # The argument to edit is optional; if left out or equal to "-",
> # it means to stop to examine or amend the previous commit.
> #
> # If you remove a line here, THAT COMMIT WILL BE LOST.
> # However, if you remove everything, the rebase will be aborted.
> # Use the noop command if you really want to remove all commits.

I like it. Especially because if we support "-" in place of a sha1, then
we can treat the rest of the line like a comment and display it when
stopped, as the old "shell" version did.

-Kevin Ballard

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Yann Dirson]

On Tue, 09 Nov 2010 19:42:15 -0600
Jonathan Nieder <jrnieder@gmail.com> wrote:

> Yann Dirson wrote:
> 
> > |Keep in mind that any commit in the original todo list, that would
> > |not be there after your edits, would not be included in the
> > resulting |rebased branch.  In case you realize afterwards that you
> > need such a |commit, you can still access it as an ancestor of
> > @{1}, see |git-reflog(1) for details.
> 
> Do you mean @{-1}?
> 
> > Maybe we could list a copy of the todo list in the comments, as a
> > reference for double-checking.  Such a list could even be used for a
> > final check before applying, that would ask confirmation if the set
> > of patches has changed, and offer to edit again.  The same config
> > item (eg. advice.interactiveRebase ?) could be used to hide the
> > note and the check.
> 
> Mm, but intentionally dropping commits is common, no?

Yes, but for people new to the feature, who may not feel at ease right
away with it, it may make sense to get warned when some change will get
lost.

BTW, about people feeling at ease with "rebase -i", I often feel not
quite comfortable to explain why to reorder commits you have to use
this "rebase" feature which sounds so strange in itself to people used
to centralized VCS.  Would that make sense to have a standard command
to reduce some confusion, like (untested):

alias.reroll = rebase -i $(git merge-base HEAD @{upstream})

> What would be nice is to be able to do
> 
> 	git rebase --change-of-plans
> 
> and somehow get my editor of choice to open with the original todo
> list (read-only) and the current todo list (read/write).
> 
> Well, a person can dream. :)

Well, that's not far from my own dreams of --back, --next and the
like :)

-- 
Yann Dirson - Bertin Technologies

Re: [PATCH] git-rebase--interactive.sh: Add new command "shell"

[Matthieu Moy]

Yann Dirson <dirson@bertin.fr> writes:

> BTW, about people feeling at ease with "rebase -i", I often feel not
> quite comfortable to explain why to reorder commits you have to use
> this "rebase" feature

I feel a bit the same. Actually, I don't think I ever used "rebase -i"
to actually perform a rebase. I usually "git pull --rebase" to rebase,
and "rebase -i" to rewrite history without changing the origin of the
branch.

> alias.reroll = rebase -i $(git merge-base HEAD @{upstream})

Mercurial calls this "histedit" for example.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCHv3] git-rebase--interactive.sh: extend "edit" command to be more useful

[Kevin Ballard]

Extend the "edit" command to simply stop for editing if no sha1 is
given or if the sha1 is equal to "-". This behaves the same as "x false"
but is a bit friendlier for the user.

Signed-off-by: Kevin Ballard <kevin@sb.org>
---

Two changes since the last patch:
* Picked up the extended explanation suggested by Jonathan Nieder.
  I left off the last line about "noop" as that doesn't seem related.
* If the line given is "edit - some comments", emit "some comments" when
  stopped. This is undocumented, so if anyone has any suggestions for how
  it should be documented I'm all ears. I'm also not sure if it should use
  the output format I selected now, or if it should just emit the comment
  in place of the commit summary (e.g. Stopped at $sha1... $comment).

 git-rebase--interactive.sh |   30 +++++++++++++++++++++++++-----
 1 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 5934b97..176f735 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -469,12 +469,29 @@ do_next () {
 		comment_for_reflog edit
 
 		mark_action_done
-		pick_one $sha1 ||
-			die_with_patch $sha1 "Could not apply $sha1... $rest"
-		echo "$sha1" > "$DOTEST"/stopped-sha
-		make_patch $sha1
+		comment=''
+		if test -n "$sha1" -a "$sha1" != "-"; then
+			pick_one $sha1 ||
+				die_with_patch $sha1 "Could not apply $sha1... $rest"
+			echo "$sha1" > "$DOTEST"/stopped-sha
+			make_patch $sha1
+		else
+			# we just want to exit to the shell
+			# we don't have a valid $sha1 or $rest, so recreate that
+			# save the original $rest to a comment for later
+			comment="$rest"
+			line=$(git rev-list --pretty=oneline -1 --abbrev-commit --abbrev=7 HEAD)
+			sha1="${line%% *}"
+			rest="${line#* }"
+			echo "$sha1" > "$DOTEST"/stopped-sha
+		fi
 		git rev-parse --verify HEAD > "$AMEND"
 		warn "Stopped at $sha1... $rest"
+		if test -n "$comment"; then
+			warn
+			warn "	$comment"
+			warn
+		fi
 		warn "You can amend the commit now, with"
 		warn
 		warn "	git commit --amend"
@@ -1016,11 +1033,14 @@ first and then run 'git rebase --continue' again."
 # Commands:
 #  p, pick = use commit
 #  r, reword = use commit, but edit the commit message
-#  e, edit = use commit, but stop for amending
+#  e, edit = use commit (if specified), but stop to amend/examine/test
 #  s, squash = use commit, but meld into previous commit
 #  f, fixup = like "squash", but discard this commit's log message
 #  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
 #
+# The argument to edit is optional; if left out or equal to "-",
+# it means to stop to examine or amend the previous commit.
+#
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
 #
-- 
1.7.3.2.488.gc5e8

Re: [PATCHv3] git-rebase--interactive.sh: extend "edit" command to be more useful

[Jonathan Nieder]

Hi,

Kevin Ballard wrote:

> [Subject: [PATCHv3] git-rebase--interactive.sh: extend "edit" command to be more useful

Maybe something like

	rebase-i: treat "edit" without sha1 as a request to amend previous commit

would make the meaning more obvious in a shortlog.

> Extend the "edit" command to simply stop for editing if no sha1 is
> given or if the sha1 is equal to "-". This behaves the same as "x false"
> but is a bit friendlier for the user.

Nice.  I like the semantics.

> * Picked up the extended explanation suggested by Jonathan Nieder.
>   I left off the last line about "noop" as that doesn't seem related.

Right, please feel free to remind me if I forget to pick that up again.

> * If the line given is "edit - some comments", emit "some comments" when
>   stopped. This is undocumented

I think that's okay for now (though of course it would be best to explain
some example uses in Documentation/git-rebase.txt in the form of examples).

> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -469,12 +469,29 @@ do_next () {
> +			comment="$rest"
> +			line=$(git rev-list --pretty=oneline -1 --abbrev-commit --abbrev=7 HEAD)

Hmm, the script seems to assume rev-list will not fail throughout.  :/
Ok.

> +			sha1="${line%% *}"
> +			rest="${line#* }"
> +			echo "$sha1" > "$DOTEST"/stopped-sha

Maybe this can be done without relying on details of --pretty=oneline
format?

			sha1=$(git rev-parse --short HEAD)
			rest=$(git show -s --format=%s HEAD)

(Yes, elsewhere the script uses

	git rev-list --no-merges --pretty=oneline --abbrev-commit \
		--abbrev=7 --reverse --left-right --topo-order "$@" |
	sed -n "s/^>//p" |
	while read -r shortsha1 rest

but in that loop, avoiding an extra exec seems more important.)

> +		fi
>  		git rev-parse --verify HEAD > "$AMEND"
>  		warn "Stopped at $sha1... $rest"
> +		if test -n "$comment"; then
> +			warn
> +			warn "	$comment"
> +			warn

Thanks, looks good to me.

Ideas for tests?  (see t3404 for inspiration)

Re: [PATCHv3] git-rebase--interactive.sh: extend "edit" command to be more useful

[Kevin Ballard]

On Dec 3, 2010, at 12:06 AM, Jonathan Nieder wrote:

> Hi,
> 
> Kevin Ballard wrote:
> 
>> [Subject: [PATCHv3] git-rebase--interactive.sh: extend "edit" command to be more useful
> 
> Maybe something like
> 
> 	rebase-i: treat "edit" without sha1 as a request to amend previous commit
> 
> would make the meaning more obvious in a shortlog.

That seems a bit misleading, though. This command really has nothing to do with
amending the previous commit. You can do anything you want once you break back to
the shell. I personally used it to run git-merge at that point in the history.
For this reason I'm a bit uneasy about overloading "edit", but it does have the
benefit that people already know "edit" brings them to the shell.

>> Extend the "edit" command to simply stop for editing if no sha1 is
>> given or if the sha1 is equal to "-". This behaves the same as "x false"
>> but is a bit friendlier for the user.
> 
> Nice.  I like the semantics.
> 
>> * Picked up the extended explanation suggested by Jonathan Nieder.
>>  I left off the last line about "noop" as that doesn't seem related.
> 
> Right, please feel free to remind me if I forget to pick that up again.
> 
>> * If the line given is "edit - some comments", emit "some comments" when
>>  stopped. This is undocumented
> 
> I think that's okay for now (though of course it would be best to explain
> some example uses in Documentation/git-rebase.txt in the form of examples).

Yep, I definitely need to add documentation.

>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -469,12 +469,29 @@ do_next () {
>> +			comment="$rest"
>> +			line=$(git rev-list --pretty=oneline -1 --abbrev-commit --abbrev=7 HEAD)
> 
> Hmm, the script seems to assume rev-list will not fail throughout.  :/
> Ok.
> 
>> +			sha1="${line%% *}"
>> +			rest="${line#* }"
>> +			echo "$sha1" > "$DOTEST"/stopped-sha
> 
> Maybe this can be done without relying on details of --pretty=oneline
> format?
> 
> 			sha1=$(git rev-parse --short HEAD)
> 			rest=$(git show -s --format=%s HEAD)

Does this not similarly assume that rev-parse and show will not fail? Or was
the above comment only meant to point out this potential issue without
suggesting that it needed to be fixed?

> (Yes, elsewhere the script uses
> 
> 	git rev-list --no-merges --pretty=oneline --abbrev-commit \
> 		--abbrev=7 --reverse --left-right --topo-order "$@" |
> 	sed -n "s/^>//p" |
> 	while read -r shortsha1 rest
> 
> but in that loop, avoiding an extra exec seems more important.)
> 
>> +		fi
>> 		git rev-parse --verify HEAD > "$AMEND"
>> 		warn "Stopped at $sha1... $rest"
>> +		if test -n "$comment"; then
>> +			warn
>> +			warn "	$comment"
>> +			warn
> 
> Thanks, looks good to me.
> 
> Ideas for tests?  (see t3404 for inspiration)

I'll look into that. I wasn't really sure how to test this before, but t3404
does have some examples of testing the edit command already.

-Kevin Ballard

Re: [PATCHv3] git-rebase--interactive.sh: extend "edit" command to be more useful

[Jonathan Nieder]

Kevin Ballard wrote:
> On Dec 3, 2010, at 12:06 AM, Jonathan Nieder wrote:

>> Maybe something like
>> 
>> 	rebase-i: treat "edit" without sha1 as a request to amend previous commit
>> 
>> would make the meaning more obvious in a shortlog.
>
> That seems a bit misleading, though. This command really has nothing to do with
> amending the previous commit.

Okay, maybe

	rebase-i: extend "edit" to allow stopping without a commit to amend

Or something else entirely; I only meant that "to be more useful" is
a bit vague (it could be cut out without loss of meaning).

>> Maybe this can be done without relying on details of --pretty=oneline
>> format?
>> 
>> 			sha1=$(git rev-parse --short HEAD)
>> 			rest=$(git show -s --format=%s HEAD)
>
> Does this not similarly assume that rev-parse and show will not fail? Or was
> the above comment only meant to point out this potential issue without
> suggesting that it needed to be fixed?

Yes, that's right.  The exit status from rev-list is ignored
throughout the script; making that more robust is a separate topic.

BTW this suggestion about avoiding --pretty=oneline was nonsense ---
the output format from

	git rev-list --pretty=oneline

is guaranteed to stay the same because rev-list is plumbing.  Sorry
for the noise.

Good night,
Jonathan

Re: [PATCHv3] git-rebase--interactive.sh: extend "edit" command to be more useful

[Johannes Sixt]

Am 12/3/2010 9:06, schrieb Jonathan Nieder:
> Kevin Ballard wrote:
>> +			sha1="${line%% *}"
>> +			rest="${line#* }"
>> +			echo "$sha1" > "$DOTEST"/stopped-sha
> 
> Maybe this can be done without relying on details of --pretty=oneline
> format?

No. This is a matter of the syntax of the recipe file. If the details of
--pretty=oneline ever changed, then the way how the boilerplate recipe
file is generated would have to be changed accordingly.

> 
> 			sha1=$(git rev-parse --short HEAD)
> 			rest=$(git show -s --format=%s HEAD)

Shouldn't $sha1 be the one given in the recipe rather than current HEAD?

But most importantly, since $rest is echoed on the terminal, it MUST be
derived from the recipe ($line). Rationale: I replace the commit subject
in the recipe by a reminder what I intend to do when the "edit" command
stops---I don't care so much what the commit subject is.

-- Hannes

Re: [PATCHv3] git-rebase--interactive.sh: extend "edit" command to be more useful

[Jonathan Nieder]

Johannes Sixt wrote:
> Am 12/3/2010 9:06, schrieb Jonathan Nieder:

>> Maybe this can be done without relying on details of --pretty=oneline
>> format?
>
> No. This is a matter of the syntax of the recipe file.

My suggestion was nonsense for other reasons, too.

>> 
>> 			sha1=$(git rev-parse --short HEAD)
>> 			rest=$(git show -s --format=%s HEAD)
>
> Shouldn't $sha1 be the one given in the recipe rather than current HEAD?

This code branch is about mentally rewriting

	pick 87a78c
	fixup 987ca
	edit - time to test

to

	pick 87a78c
	fixup 987ca
	edit <whatever is HEAD at that moment>

and printing "time to test" as a reminder to the user.

> But most importantly, since $rest is echoed on the terminal, it MUST be
> derived from the recipe ($line). Rationale: I replace the commit subject
> in the recipe by a reminder what I intend to do when the "edit" command
> stops---I don't care so much what the commit subject is.

Kevin, this sounds like a vote for the "replace commit message" output
format.

Thanks, that was useful.
Jonathan

Re: [PATCHv3] git-rebase--interactive.sh: extend "edit" command to be more useful

[Kevin Ballard]

On Dec 3, 2010, at 2:00 AM, Jonathan Nieder wrote:

>> But most importantly, since $rest is echoed on the terminal, it MUST be
>> derived from the recipe ($line). Rationale: I replace the commit subject
>> in the recipe by a reminder what I intend to do when the "edit" command
>> stops---I don't care so much what the commit subject is.
> 
> Kevin, this sounds like a vote for the "replace commit message" output
> format.

The v3 patch will emit both a description of the commit it stopped on, as well
as the comment. The rationale for extracting the first line of HEAD is for when
the user doesn't provide any comment - e.g. they just add "edit". It may be
worth doing this only in that case, and if the user did provide a comment,
emit it in place of the first line of HEAD.

Given the recipe

	pick bc17bb7 git-rebase--interactive.sh: extend "edit" command to be more useful
	edit - foo

the edit command would print

	Stopped at bc17bb7... git-rebase--interactive.sh: extend "edit" command to be more useful
	
		foo
	
	You can amend the commit now...

The alternative is to make that same recipe emit

	Stopped at bc17bb7... foo
	
	You can amend the commit now...

I'm leaning towards making that change right now, but I'm not certain.
Do either of you have a preference?

-Kevin Ballard