threads / rfc / 42992

RFC patchrebase--interactive: Add "sign" command

Subject: [RFC/PATCH] rebase--interactive: Add "sign" command

## tl;dr

11 messages between Aug 3, 2016 and Aug 5, 2016. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Chris Packham· Aug 3, 2016, 08:47 UTC · lore

This is similar to the existing "reword" command in that it can be used to update the commit message the difference is that the editor presented to the user for the commit. It provides a useful shorthand for "exec git commit --amend --no-edit -s"

Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
Hi,

At $dayjob we have a patch based work-flow where committers sign patches on the way through. Occasionally when a larger patch series is involved it's easier pull from the submitter's repo and use git rebase -i to add the required sign off either by using 'reword' or 'exec git commit --amend -s'.

This is my attempt at making this a little less cumbersome by adding a 'sign' command. I decided not to add a short version of the command, partly because 's' was taken and partly because it is a bit of a niche use-case.

Thanks, Chris

 git-rebase--interactive.sh    | 10 +++++++++-
 t/lib-rebase.sh               |  2 +-
 t/t3404-rebase-interactive.sh |  9 +++++++++
 3 files changed, 19 insertions(+), 2 deletions(-)
Show changes to 3 files +19 −2

git-rebase--interactive.sh, t/lib-rebase.sh, t/t3404-rebase-interactive.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ded4595..1cd8bc6 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -148,6 +148,7 @@ append_todo_help () {
 Commands:
  p, pick = use commit
  r, reword = use commit, but edit the commit message
+ sign = use commit, add sign-off to 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
@@ -594,6 +595,13 @@ you are able to reword the commit.")"
 		}
 		record_in_rewritten $sha1
 		;;
+	sign)
+		comment_for_reflog sign
+		mark_action_done
+		do_pick $sha1 "$rest"
+		git commit --amend -s --no-post-rewrite --no-edit ${gpg_sign_opt:+"$gpg_sign_opt"}
+		record_in_rewritten $sha1
+		;;
 	edit|e)
 		comment_for_reflog edit
 
@@ -959,7 +967,7 @@ check_bad_cmd_and_sha () {
 			# Work around CR left by "read" (e.g. with Git for
 			# Windows' Bash).
 			;;
-		pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
+		pick|p|drop|d|reword|r|sign|edit|e|squash|s|fixup|f)
 			if ! check_commit_sha "${rest%%[ 	]*}" "$lineno" "$1"
 			then
 				retval=1
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 25a77ee..5a54228 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -47,7 +47,7 @@ set_fake_editor () {
 	action=pick
 	for line in $FAKE_LINES; do
 		case $line in
-		squash|fixup|edit|reword|drop)
+		squash|fixup|edit|reword|sign|drop)
 			action="$line";;
 		exec*)
 			echo "$line" | sed 's/_/ /g' >> "$1";;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 197914b..e473ffb 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -690,6 +690,15 @@ test_expect_success 'reword' '
 	git show HEAD~2 | grep "C changed"
 '
 
+test_expect_success 'sign-off' '
+	git checkout -b sign-off-branch master &&
+	set_fake_editor &&
+	FAKE_LINES="1 2 3 sign 4" git rebase -i A &&
+	git show HEAD | grep "Signed-off-by:" &&
+	test $(git rev-parse master) != $(git rev-parse HEAD) &&
+	test $(git rev-parse master^) = $(git rev-parse HEAD^)
+'
+
 test_expect_success 'rebase -i can copy notes' '
 	git config notes.rewrite.rebase true &&
 	git config notes.rewriteRef "refs/notes/*" &&
-- 
2.9.2.518.ged577c6.dirty
Johannes Schindelin· Aug 3, 2016, 14:31 UTC · re: Chris Packham · lore

Re: [RFC/PATCH] rebase--interactive: Add "sign" command

Hi Chris,
On Wed, 3 Aug 2016, Chris Packham wrote:
> This is similar to the existing "reword" command in that it can be used
> to update the commit message the difference is that the editor presented
> to the user for the commit. It provides a useful shorthand for "exec git
> commit --amend --no-edit -s"

I can understand how this "sign" command helps you. I myself wished for new commands when working on my Git garden shears [*1*] (essentially, what git rebase --interactive --preserve-merges *should* have been).

My solution was to introduce a new fake editor that calls the real editor and afterwards converts the "new" commands into exec lines.

Having said that, this patch clashes seriously with my current effort to move a lot of the interactive rebase from shell into plain C. It is actually ready, but getting this into the code base is really slow-going, unfortunately.

Now, after looking at your patch it looks to me as if this would be easily ported, so there is not a big deal here.

However, I could imagine that we actually want this to be more extensible. After all, all you are doing is to introduce a new rebase -i command that does nothing else than shelling out to a command. Why not introduce a much more flexible feature, where you add something like "rebase -i aliases"?

Maybe something like this:
[rebase "command"]
	sign = git commit --amend -s --no-post-rewrite --no-edit -S

I have not completely thought this through, but maybe this direction would make the interactive rebase even more powerful?

Ciao, Johannes

Johannes Schindelin· Aug 3, 2016, 15:19 UTC · re: Johannes Schindelin · lore

Re: [RFC/PATCH] rebase--interactive: Add "sign" command

Hi Chris,
On Wed, 3 Aug 2016, Johannes Schindelin wrote:
> I can understand how this "sign" command helps you. I myself wished for
> new commands when working on my Git garden shears [*1*] (essentially, what
> git rebase --interactive --preserve-merges *should* have been).

And of course I forgot to provide the footnote. Sigh. So here they are, the Git garden shears, intended to rebase a thicket of branches:

https://github.com/git-for-windows/build-extra/blob/master/shears.sh
The edit script will look somewhat like this:
	mark onto
	# branch "something"
	bud
	pick abcdef first commit
	pick abcde0 second commit
	mark something
	# branch "another-one"
	bud
	pick cafebabe yep, that's a different branch
	mark another-one
	bud
	merge -C 0123456 something
	merge -C 6543210 another-one
	cleanup something another-one

So you see, I needed to introduce new commands: bud, mark, merge and cleanup (I actually also added a "reset" one).

The fake editor I talked about is really the script itself, which detects that it was run as the fake editor, and which converts all those commands into the form "exec git .r <command> <parameters>...". The alias..r setting also points to the same script, so that I really only need one script. It's one big hack, but it works.

Needless to say, my idea to support new rebase -i commands via config settings would be something I would use myself in the Git garden shears.

Ciao, Johannes

Junio C Hamano· Aug 3, 2016, 16:08 UTC · re: Johannes Schindelin · lore

Re: [RFC/PATCH] rebase--interactive: Add "sign" command

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> ... my Git garden shears [*1*] (essentially, what
> git rebase --interactive --preserve-merges *should* have been).

Any plan to fold it into "git rebase -i" as a new (improved) mode of operation, by the way?

> However, I could imagine that we actually want this to be more extensible.
> After all, all you are doing is to introduce a new rebase -i command that
> does nothing else than shelling out to a command.
Yup, I tend to agree.

Adding "sign" feature (i.e. make it pass -S to "commit [--amend]") may be a good thing, but adding "sign" command to do so is not a great design.

There is no inherent reason why "sign" feature implies "--no-edit", and adding a "sign" command like this patch means that the next command somebody else proposes will be "sign-and-reword".

We should be able to treat Signing and Rewording as two orthogonal features, one that passes -S, and the other that refrains from passing --no-edit. Otherwise as the number of features grow, the number of commands will see combinatorial growth.

Thanks.
Johannes Schindelin· Aug 3, 2016, 16:15 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] rebase--interactive: Add "sign" command

Hi Junio,
On Wed, 3 Aug 2016, Junio C Hamano wrote:
Show 7 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > ... my Git garden shears [*1*] (essentially, what
> > git rebase --interactive --preserve-merges *should* have been).
> 
> Any plan to fold it into "git rebase -i" as a new (improved) mode of
> operation, by the way?

It was my plan to integrate it into the sequencer, once the rebase--helper landed in `master`. Little did I know how long it would take to get this done...

My loose idea was to deprecate --preserve-merges after introducing a --recreate-branches, or some such. But enough of that. The rebase--helper needs to get here first.

Show 5 quoted lines
> > However, I could imagine that we actually want this to be more extensible.
> > After all, all you are doing is to introduce a new rebase -i command that
> > does nothing else than shelling out to a command.
> 
> Yup, I tend to agree.
Awesome!

Ciao, Dscho

Jeff King· Aug 3, 2016, 18:08 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] rebase--interactive: Add "sign" command

On Wed, Aug 03, 2016 at 09:08:48AM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> > However, I could imagine that we actually want this to be more extensible.
> > After all, all you are doing is to introduce a new rebase -i command that
> > does nothing else than shelling out to a command.
> 
> Yup, I tend to agree.
> 
> Adding "sign" feature (i.e. make it pass -S to "commit [--amend]")
> may be a good thing, but adding "sign" command to do so is not a
> great design.

I'm not sure what you mean by "feature" here, but it reminded me of Michael's proposal to allow options to todo lines:

  http://public-inbox.org/git/530DA00E.4090402@alum.mit.edu/
which would allow:
  pick -S 1234abcd
If that's what you meant, I think it is a good idea. :)
-Peff
Chris Packham· Aug 4, 2016, 00:53 UTC · re: Jeff King · lore

Re: [RFC/PATCH] rebase--interactive: Add "sign" command

On Thu, Aug 4, 2016 at 6:08 AM, Jeff King <peff@peff.net> wrote:
Show 22 quoted lines
> On Wed, Aug 03, 2016 at 09:08:48AM -0700, Junio C Hamano wrote:
>
>> > However, I could imagine that we actually want this to be more extensible.
>> > After all, all you are doing is to introduce a new rebase -i command that
>> > does nothing else than shelling out to a command.
>>
>> Yup, I tend to agree.
>>
>> Adding "sign" feature (i.e. make it pass -S to "commit [--amend]")
>> may be a good thing, but adding "sign" command to do so is not a
>> great design.
>
> I'm not sure what you mean by "feature" here, but it reminded me of
> Michael's proposal to allow options to todo lines:
>
>   http://public-inbox.org/git/530DA00E.4090402@alum.mit.edu/
>
> which would allow:
>
>   pick -S 1234abcd
>
> If that's what you meant, I think it is a good idea. :)

That would definitely suit me. I see there was some discussion of which options were sensible to support. --signoff and --reset-author would be a good start from my point of view. Maybe that's something that could be build on top of Johannes work.

>
> -Peff
Junio C Hamano· Aug 4, 2016, 16:05 UTC · re: Jeff King · lore

Re: [RFC/PATCH] rebase--interactive: Add "sign" command

Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> On Wed, Aug 03, 2016 at 09:08:48AM -0700, Junio C Hamano wrote:
>
>> > However, I could imagine that we actually want this to be more extensible.
>> > After all, all you are doing is to introduce a new rebase -i command that
>> > does nothing else than shelling out to a command.
>> 
>> Yup, I tend to agree.
>> 
>> Adding "sign" feature (i.e. make it pass -S to "commit [--amend]")
>> may be a good thing, but adding "sign" command to do so is not a
>> great design.
>
> I'm not sure what you mean by "feature" here, but it reminded me of
> Michael's proposal to allow options to todo lines:
>
>   http://public-inbox.org/git/530DA00E.4090402@alum.mit.edu/
>
> which would allow:
>
>   pick -S 1234abcd
>
> If that's what you meant, I think it is a good idea. :)

Yes, by "feature" I meant "giving the ability to decide if the resulting commit gets signature", which can and should be orthogonal to the choice of using editor to reword the message when the commit is created or "--no-edit" is passed and the original message is used verbatim.

Johannes Schindelin· Aug 5, 2016, 16:04 UTC · re: Jeff King · lore

Re: [RFC/PATCH] rebase--interactive: Add "sign" command

Hi Peff,
On Wed, 3 Aug 2016, Jeff King wrote:
Show 23 quoted lines
> On Wed, Aug 03, 2016 at 09:08:48AM -0700, Junio C Hamano wrote:
> 
> > > However, I could imagine that we actually want this to be more
> > > extensible.  After all, all you are doing is to introduce a new
> > > rebase -i command that does nothing else than shelling out to a
> > > command.
> > 
> > Yup, I tend to agree.
> > 
> > Adding "sign" feature (i.e. make it pass -S to "commit [--amend]") may
> > be a good thing, but adding "sign" command to do so is not a great
> > design.
> 
> I'm not sure what you mean by "feature" here, but it reminded me of
> Michael's proposal to allow options to todo lines:
> 
>   http://public-inbox.org/git/530DA00E.4090402@alum.mit.edu/
> 
> which would allow:
> 
>   pick -S 1234abcd
> 
> If that's what you meant, I think it is a good idea. :)

I looked at the code in git-rebase--interactive.sh again and stumbled over something important: if you "pick" a commit, it *already* uses the information provided to the rebase command via the -S option, *unless* the pick fast-forwards.

That is, I came to believe that the "sign" command is unnecessary, and that the --force-rebase option in conjunction with the -S option is what should be used.

Ciao, Dscho

Chris Packham· Aug 4, 2016, 00:41 UTC · re: Johannes Schindelin · lore

Re: [RFC/PATCH] rebase--interactive: Add "sign" command

On Thu, Aug 4, 2016 at 2:31 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 8 quoted lines
> Hi Chris,
>
> On Wed, 3 Aug 2016, Chris Packham wrote:
>
>> This is similar to the existing "reword" command in that it can be used
>> to update the commit message the difference is that the editor presented
>> to the user for the commit. It provides a useful shorthand for "exec git
>> commit --amend --no-edit -s"
<snip>
Show 7 quoted lines
> Having said that, this patch clashes seriously with my current effort to
> move a lot of the interactive rebase from shell into plain C. It is
> actually ready, but getting this into the code base is really slow-going,
> unfortunately.
>
> Now, after looking at your patch it looks to me as if this would be easily
> ported, so there is not a big deal here.

Yeah sorry. I knew there was something in flight but ended up doing a quick hack on top of master.

Show 9 quoted lines
> However, I could imagine that we actually want this to be more extensible.
> After all, all you are doing is to introduce a new rebase -i command that
> does nothing else than shelling out to a command. Why not introduce a much
> more flexible feature, where you add something like "rebase -i aliases"?
>
> Maybe something like this:
>
> [rebase "command"]
>         sign = git commit --amend -s --no-post-rewrite --no-edit -S

I did briefly consider that. I ended up taking the shortcut because I had a patch series I needed to sign.

Elsewhere in this thread the idea of pick -S or reword -S was raised. I'd actually prefer that because there seems little between 'git config rebase.command.sign blah' and 'exec ~/sign.sh'

> I have not completely thought this through, but maybe this direction would
> make the interactive rebase even more powerful?

The uses I can think of are adding sign-off and running "make check". For me rebase -i doesn't need to be much more powerful than that.

Junio C Hamano· Aug 3, 2016, 15:58 UTC · re: Chris Packham · lore

Re: [RFC/PATCH] rebase--interactive: Add "sign" command

Chris Packham <judge.packham@gmail.com> writes:
> This is similar to the existing "reword" command in that it can be used
> to update the commit message the difference is that the editor presented
> to the user for the commit. It provides a useful shorthand for "exec git
> commit --amend --no-edit -s"
Hmm, what should those who want to amend and sign do, though?

← back to recent threads