[PATCH 0/4] support for ack commits

[PATCH 0/4] support for ack commits

[Michael S. Tsirkin]

This is a repost after rebasing, and addressing comments by Eric Sunshine and
Fabian Ruch.  I'd like to try getting this upstream so  I can stop maintaining
it. So reposting - rebased to latest master, with a better motivation in the
cover letter.

As a maintainer, I get patches by mail, then
acked-by,reviewed-by etc responses are sent by separate
mail.

The result is that I have a patch applied, and now
I need to find it and apply the ack responses to it.

The flow I use to handle this, is to record an
empty commit (which I'm calling an ack commit)
with just the ack in the log, and ack! tag
and the subject of the original patch in the
subject.

Sometimes, I would also make a small change with
that ack commit, typically using commit --amend,
for example if the ack mail says:

	Subject: Re: [PATCH] xyz

	please rename xyz to foo. With that:

	Acked-by: Michael S. Tsirkin <mst@redhat.com>

I would apply ack and make the change as part of that.

Later, once in a while I rebase and squash the ack commits
into the regular one: the rebase autosquash mechanics
find the original commit and update the commit log,
appending the ack.

from example, we start with an email:
	From: Michael S. Tsirkin <mst@redhat.com>
	Subject: [PATCH] foo.c: change b to c
	Date:   Wed Apr 6 22:07:34 2016 +0300

	    foo.c: change b to c
	    
	    Change BBBBBBBBBBBBBBBBB to CCCCCCCCCCCCCCCCC
	    
	    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

	---

	diff --git a/foo.c b/foo.c
	index 8e5be91..34654fc 100644
	--- a/foo.c
	+++ b/foo.c
	@@ -676,6 +676,6 @@ FFFFFFFFFFFFFFFFFFFFFFF(
		AAAAAAAAAAAAAAAAAAAAAAAAA
	 
	-       BBBBBBBBBBBBBBBBB
	-       BBBBBBBBBBBBBBBBB
	+       CCCCCCCCCCCCCCCCC
	+       CCCCCCCCCCCCCCCCC

		DDDDDDDDDDDDDDDDD

and I apply it using git am.


then I get an email:

	Subject: Re: [PATCH] foo.c: change b to c

        > 	    foo.c: change b to c
        > 	    
        > 	    Change BBBBBBBBBBBBBBBBB to CCCCCCCCCCCCCCCCC
        > 	    
        > 	    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
        > 
        > 	---
        > 
        > 	diff --git a/foo.c b/foo.c
        > 	index 8e5be91..34654fc 100644
        > 	--- a/foo.c
        > 	+++ b/foo.c
        > 	@@ -676,6 +676,6 @@ FFFFFFFFFFFFFFFFFFFFFFF(
        > 		AAAAAAAAAAAAAAAAAAAAAAAAA
        > 	 
        > 	-       BBBBBBBBBBBBBBBBB
        > 	-       BBBBBBBBBBBBBBBBB
        > 	+       CCCCCCCCCCCCCCCCC
        > 	+       CCCCCCCCCCCCCCCCC
        > 
        > 		DDDDDDDDDDDDDDDDD
	    
	Acked-by: Junio C Hamano <gitster@pobox.com>

I then create an empty commit using the subject and the ack line:

	commit 4d54b237d8d03323933e27119272e93cf33b4e98
	Author: Michael S. Tsirkin <mst@redhat.com>
	Date:   Wed Apr 6 22:07:34 2016 +0300

	ack! foo.c: change b to c

	Acked-by: Junio C Hamano <gitster@pobox.com>

(with no change) and then after rebase -i --autosquash it is combined
with the original commit by squashing changes (easy as the
second one has an empty change), and appending the commit log
from the second one to first one:

commit ef7b6d457c28bcd06d0118a889c7070fc800f3d5
Author: Michael S. Tsirkin <mst@redhat.com>
Date:   Wed Apr 6 14:55:59 2016 +0300

    foo.c: change b to c
    
    Change BBBBBBBBBBBBBBBBB to CCCCCCCCCCCCCCCCC
    
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
    Acked-by: Junio C Hamano <gitster@pobox.com>

diff --git a/foo.c b/foo.c
index 8e5be91..34654fc 100644
--- a/foo.c
+++ b/foo.c
@@ -676,6 +676,6 @@ FFFFFFFFFFFFFFFFFFFFFFF(
	AAAAAAAAAAAAAAAAAAAAAAAAA
 
-       BBBBBBBBBBBBBBBBB
-       BBBBBBBBBBBBBBBBB
+       CCCCCCCCCCCCCCCCC
+       CCCCCCCCCCCCCCCCC

	DDDDDDDDDDDDDDDDD


The empty ack commits can be created by hand. I have also
written a small script for that - included
as patch 4/4 but it is still rather rough so only putting it
under contrib for now - would like to try
and merge the rebase machinery in place first.
Long term, it might be cleaner to teach git-am about an --ack flag.
But it is already helpful to explain how this is intended to be used.
That script can be used in one of two ways:

	1. pipe the mail with ack to it. we extract
	   subject and prepend ack!, extract the ack
	   trailer line that needs to be appended to commit,
	   and record the result as en empty commit.
	2. pipe the mail with ack to it with flag -s,
	   this saves the ack trailer into a file.
           then pipe the original patch(es) to it
           with flag -r, now the subject is taken from
           patch, with ack! prepended, but the ack trailer
           is from the ack email.
           this is useful to handle series acks, similar to:
		   For series:
			Acked-by: Michael S. Tsirkin <mst@redhat.com>



So what is an ack commit from point of view of rebase?

1. subject is ack! <original patch>

2. commit can be empty and it does not mean it needs to be skipped

3. when squashing into parent commit, subject and empty line
   after it should be skipped, and the rest of commit log
   should be appended to commit log of parent, as is.


I have been using these patches without any problems for more than a
year now, and find the approach very convenient.

Included:
        rebase: new ack! action to handle ack commits
                this part seems ready for merge to me,
                please review and comment

        git-ack: new tool to record an ack
                this does not have proper documentation
                and tests yet, I definitely intend to
                do this but wanted to see whether people
                like the UI first.
                posting for early review and feedback


Note: people mostly using pull requests for communication
will not find this approach useful.
As it's optional, this should not be a problem.

Note: it was suggested that "squash! --noedit" would be easier
to maintain than "ack!" - I think this would be less
user-friendly, so I left this suggestion out for now.


Michael S. Tsirkin (4):
  rebase -i: add ack action
  git-rebase: document ack
  rebase: test ack
  git-ack: record an ack

 Documentation/git-rebase.txt | 45 +++++++++++++++++++---
 contrib/git-ack              | 90 ++++++++++++++++++++++++++++++++++++++++++++
 git-rebase--interactive.sh   | 36 ++++++++++++++----
 t/t3415-rebase-autosquash.sh | 15 ++++++++
 4 files changed, 173 insertions(+), 13 deletions(-)
 create mode 100755 contrib/git-ack

-- 
MST

[PATCH 1/4] rebase -i: add ack action

[Michael S. Tsirkin]

This implements a new ack! action for git rebase -i
It is essentially a middle ground between fixup! and squash!:
- commits are squashed silently without editor being started
- commit logs are concatenated (with action line being discarded)
- because of the above, empty commits aren't discarded,
  their log is also included.

I am using it as follows:
	git am -s < mailbox #creates first commit
	hack ...
	get mail with Ack
	git commit --allow-empty -m `cat <<-EOF
	ack! first

	Acked-by: maintainer
	EOF`
	repeat cycle
	git rebase --autosquash -i origin/master
	before public branch push

The "cat" command above is actually a script that
parses the Ack mail to create the empty commit -
to be submitted separately.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 git-rebase--interactive.sh | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 4cde685..6a766ca 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -150,6 +150,7 @@ Commands:
  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
+ a, ack = like "squash", but append commit body only to previous commit
  f, fixup = like "squash", but discard this commit's log message
  x, exec = run command (the rest of the line) using shell
  d, drop = remove commit
@@ -438,6 +439,15 @@ update_squash_messages () {
 		echo
 		commit_message $2
 		;;
+	ack)
+		if test -f "$fixup_msg"
+		then
+			commit_message $2 | git stripspace --strip-comments | sed -e '1,2d' >> "$fixup_msg"
+		fi
+		printf '%s\n' "$comment_char This is the $(nth_string $count) commit message:"
+		echo
+		commit_message $2
+		;;
 	fixup)
 		echo
 		printf '%s\n' "$comment_char The $(nth_string $count) commit message will be skipped:"
@@ -479,7 +489,7 @@ record_in_rewritten() {
 	echo "$oldsha1" >> "$rewritten_pending"
 
 	case "$(peek_next_command)" in
-	squash|s|fixup|f)
+	squash|s|fixup|f|ack|a)
 		;;
 	*)
 		flush_rewritten_pending
@@ -551,8 +561,11 @@ do_next () {
 		warn "Stopped at $sha1... $rest"
 		exit_with_patch $sha1 0
 		;;
-	squash|s|fixup|f)
+	squash|s|fixup|f|ack|a)
 		case "$command" in
+		ack|a)
+			squash_style=ack
+			;;
 		squash|s)
 			squash_style=squash
 			;;
@@ -576,7 +589,7 @@ do_next () {
 			die_failed_squash $sha1 "$rest"
 		fi
 		case "$(peek_next_command)" in
-		squash|s|fixup|f)
+		squash|s|fixup|f|ack|a)
 			# This is an intermediate commit; its message will only be
 			# used in case of trouble.  So use the long version:
 			do_with_author output git commit --amend --no-verify -F "$squash_msg" \
@@ -587,7 +600,7 @@ do_next () {
 			# This is the final command of this squash/fixup group
 			if test -f "$fixup_msg"
 			then
-				do_with_author git commit --amend --no-verify -F "$fixup_msg" \
+				do_with_author git commit --quiet --amend --no-verify -F "$fixup_msg" \
 					${gpg_sign_opt:+"$gpg_sign_opt"} ||
 					die_failed_squash $sha1 "$rest"
 			else
@@ -717,7 +730,7 @@ skip_unnecessary_picks () {
 	done <"$todo" >"$todo.new" 3>>"$done" &&
 	mv -f "$todo".new "$todo" &&
 	case "$(peek_next_command)" in
-	squash|s|fixup|f)
+	squash|s|fixup|f|ack|a)
 		record_in_rewritten "$onto"
 		;;
 	esac ||
@@ -764,7 +777,7 @@ rearrange_squash () {
 	do
 		test -z "${format}" || message=$(git log -n 1 --format="%s" ${sha1})
 		case "$message" in
-		"squash! "*|"fixup! "*)
+		"squash! "*|"fixup! "*|"ack! "*)
 			action="${message%%!*}"
 			rest=$message
 			prefix=
@@ -772,7 +785,7 @@ rearrange_squash () {
 			while :
 			do
 				case "$rest" in
-				"squash! "*|"fixup! "*)
+				"squash! "*|"fixup! "* |"ack! "*)
 					prefix="$prefix${rest%%!*},"
 					rest="${rest#*! }"
 					;;
@@ -904,7 +917,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|edit|e|squash|s|fixup|f|ack|a)
 			if ! check_commit_sha "${rest%%[ 	]*}" "$lineno" "$1"
 			then
 				retval=1
@@ -1196,6 +1209,13 @@ do
 		comment_out=
 	fi
 
+	# keep empty ack! commits around: useful to add text to commit log
+	case "$rest" in
+	"ack! "*)
+		comment_out=
+		;;
+	esac
+
 	if test t != "$preserve_merges"
 	then
 		printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo"
-- 
MST

[PATCH 2/4] git-rebase: document ack

[Michael S. Tsirkin]

document ack! behaviour and use

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 Documentation/git-rebase.txt | 45 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 0387b40..257d75c 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -402,7 +402,7 @@ or by giving more than one `--exec`:
 +
 If `--autosquash` is used, "exec" lines will not be appended for
 the intermediate commits, and will only appear at the end of each
-squash/fixup series.
+squash/fixup/ack series.
 +
 This uses the `--interactive` machinery internally, but it can be run
 without an explicit `--interactive`.
@@ -419,13 +419,13 @@ without an explicit `--interactive`.
 
 --autosquash::
 --no-autosquash::
-	When the commit log message begins with "squash! ..." (or
-	"fixup! ..."), and there is a commit whose title begins with
+	When the commit log message begins with "squash! ..." ("fixup! ..."
+	or "ack! ..."), and there is a commit whose title begins with
 	the same ..., automatically modify the todo list of rebase -i
 	so that the commit marked for squashing comes right after the
 	commit to be modified, and change the action of the moved
-	commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
-	"fixup! " or "squash! " after the first, in case you referred to an
+	commit from `pick` to `squash` (`fixup` or `ack`).  Ignores subsequent
+	"ack! ", "fixup! " or "squash! " after the first, in case you referred to an
 	earlier fixup/squash with `git commit --fixup/--squash`.
 +
 This option is only valid when the '--interactive' option is used.
@@ -649,6 +649,41 @@ consistent (they compile, pass the testsuite, etc.) you should use
 'git stash' to stash away the not-yet-committed changes
 after each commit, test, and amend the commit if fixes are necessary.
 
+----------------
+RECORDING ACKS
+----------------
+
+Interactive mode with --autosquash can be used to concatenate
+commit log for several commits, which is useful to record
+extra information about the commit, such as ack signatures.
+This allows, for example, the following workflow:
+
+1. receive patches by mail and commit
+2. receive by mail ack signatures for the patches
+3. prepare a series for submission
+4. submit
+
+where point 2. consists of several instances of
+	i) create a (possibly empty) commit with signature
+	  in the commit message
+
+Sometimes the ack signature added in i. cannot be amended to the
+commit it acks, because that commit is buried deeply in a
+patch series.  That is exactly what rebase --autosquash
+option is for: use it
+after plenty of "i"s, to automaticlly rearrange
+commits, and squashing multiple sign-off commits into
+the commit that is signed.
+
+Start it with the last commit you want to retain as-is:
+
+	git rebase --autosquash -i <after-this-commit>
+
+An editor will be fired up with all the commits in your current branch
+which come after the given commit. Ack commits will be
+re-arranged to come after the commit that is acked,
+and the action will be automatically changed from `pick` to `ack`
+to cause them to be squashed into the acked commit.
 
 RECOVERING FROM UPSTREAM REBASE
 -------------------------------
-- 
MST

[PATCH 3/4] rebase: test ack

[Michael S. Tsirkin]

test ack! handling

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 t/t3415-rebase-autosquash.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 8f53e54..e78897d 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -74,6 +74,21 @@ test_expect_success 'auto squash (option)' '
 	test_auto_squash final-squash --autosquash
 '
 
+test_expect_success 'auto ack' '
+	ack="Acked-by: xyz" &&
+	msg=$(test_write_lines "ack! first commit" "" "$ack") &&
+	git reset --hard base &&
+	git commit --allow-empty -m "$msg" -- &&
+	git tag ack &&
+	test_tick &&
+	git rebase --autosquash -i HEAD^^^ &&
+	git log --oneline >actual &&
+	git show -s first-commit | grep -v ^commit > expected-msg &&
+	echo "    $ack" >> expected-msg &&
+	git show -s HEAD^ | grep -v ^commit > actual-msg &&
+	diff actual-msg expected-msg
+'
+
 test_expect_success 'auto squash (config)' '
 	git config rebase.autosquash true &&
 	test_auto_squash final-squash-config-true &&
-- 
MST

[PATCH 4/4] git-ack: record an ack

[Michael S. Tsirkin]

This is a simple script that I use by piping
incoming mail with an ack to it.
It produces an empty ack commit suitable for
squshing with git rebase -i -autosquash.

Works best if people ack individual commits: you simply
pipe each ack to git ack, before pushing your branch,
rebase.

Some people ack series by responding to cover letter
or to commit 1.
To address this usecase, there are two additional
flags: -s saves the ack signature in a file (you can
save several in a row), -r creates an ack for
a given patch using the saved signature.
Thus: pipe ack(s) to git ack -s, then select and pipe
each individual patch to git ack -r.

If it's found useful, this script can either
become a first-class command (with documentation
and tests) or be integrated as a flag into git am.

Limitations: requires that index is clean, this is
so we can create an empty commit recording the ack.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 contrib/git-ack | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100755 contrib/git-ack

diff --git a/contrib/git-ack b/contrib/git-ack
new file mode 100755
index 0000000..d8cba95
--- /dev/null
+++ b/contrib/git-ack
@@ -0,0 +1,91 @@
+msg=$(mktemp)
+patch=$(mktemp)
+info=$(git mailinfo $msg $patch)
+subj=$(echo "$info" | sed -n 's/^Subject: //p')
+#strip ack!/fixup!/squash! prefix
+subj=$(echo "$subj" | sed "s/^fixup![ 	]*//")
+subj=$(echo "$subj" | sed "s/^squash![ 	]*//")
+subj=$(echo "$subj" | sed "s/^ack![ 	]*//")
+author=$(echo "$info" | sed -n 's/^Author: //p')
+email=$(echo "$info" | sed -n 's/^Email: //p')
+auth="$author <$email>"
+date=$(echo "$info" | sed -n 's/^Date: //p')
+sign=$(mktemp)
+echo "ack! $subj" >"$sign"
+echo "" >> $sign
+if
+	git diff --exit-code --cached HEAD
+then
+	:
+else
+	echo "DIFF in cache. Not acked, reset or commit!"
+	exit 1
+fi
+GIT_DIR=$(git rev-parse --git-dir)
+
+usage () {
+	echo "Usage: git ack " \
+		"[-s|--save|-a|--append|-r|--restore |-c|--clear]\n" >&2;
+	exit 1;
+}
+
+append=
+save=
+clear=
+restore=
+
+while test $# != 0
+do
+	case "$1" in
+	-a|--append)
+		append="y"
+		;;
+	-s|--s)
+		save="y"
+		;;
+	-r|--restore)
+		restore="y"
+		;;
+	-c|--clear)
+		clear="y"
+		;;
+	*)
+		usage ;;
+	esac
+	shift
+done
+
+if
+	test "$clear"
+then
+	rm -f "${GIT_DIR}/ACKS"
+fi
+
+if
+	test "$save"
+then
+	if
+		test "$append"
+	then
+		cat $msg >>"${GIT_DIR}/ACKS"
+	else
+		cat $msg >"${GIT_DIR}/ACKS"
+	fi
+	exit 0
+fi
+
+if
+	test "$restore"
+then
+	msg=${GIT_DIR}/ACKS
+fi
+
+echo $msg > /dev/tty
+if
+	grep '^[A-Z][A-Za-z-]*-by:' $msg >> $sign
+then
+	git commit --allow-empty -F $sign --author="$auth" --date="$date"
+else
+	echo "No signature found!"
+	exit 2
+fi
-- 
MST

Re: [PATCH 1/4] rebase -i: add ack action

[Johannes Schindelin]

Hi Michael,

On Sun, 10 Apr 2016, Michael S. Tsirkin wrote:

> This implements a new ack! action for git rebase -i
> It is essentially a middle ground between fixup! and squash!:
> - commits are squashed silently without editor being started
> - commit logs are concatenated (with action line being discarded)
> - because of the above, empty commits aren't discarded,
>   their log is also included.
> 
> I am using it as follows:
> 	git am -s < mailbox #creates first commit
> 	hack ...
> 	get mail with Ack
> 	git commit --allow-empty -m `cat <<-EOF
> 	ack! first
> 
> 	Acked-by: maintainer
> 	EOF`
> 	repeat cycle
> 	git rebase --autosquash -i origin/master
> 	before public branch push
> 
> The "cat" command above is actually a script that
> parses the Ack mail to create the empty commit -
> to be submitted separately.

This looks awfully complicated, still, and not very generic.

How about making it easier to use, and much, much more generic, like this?

1. introducing an `--add-footer` flag to `git commit` that you could use
like this:

	git commit --amend --add-footer "Acked-by: Bugs Bunny"

2. introducing an `--exec-after` flag to `git commit` that would be a new
sibling of `--fixup` and `--squash` and would work like this:

	git commit --exec-after HEAD~5 \
		'git commit --amend --add-footer "Acked-by: Bugs Bunny"'

(it should imply `--allow-empty`, of course, and probably even fail if
anything was staged for commit at that point.) The commit message would
then look something like

	exec-after! Fix broken breakage

	git commit --amend --add-footer "Acked-by: Bugs Bunny"

This way would obviously benefit a lot more users. For example, you could
easily say (and alias)

	git commit --amend --add-footer 'Reviewed-by: Arrested Developer"

i.e. support all kind of use cases where developers need to slap on
footers in a quick & easy way.

And the --exec-after option would obviously have *a lot* more use cases
than just squashing in ACKs.

Ciao,
Johannes

Re: [PATCH 1/4] rebase -i: add ack action

[Michael S. Tsirkin]

On Mon, Apr 11, 2016 at 01:02:07PM +0200, Johannes Schindelin wrote:
> Hi Michael,
> 
> On Sun, 10 Apr 2016, Michael S. Tsirkin wrote:
> 
> > This implements a new ack! action for git rebase -i
> > It is essentially a middle ground between fixup! and squash!:
> > - commits are squashed silently without editor being started
> > - commit logs are concatenated (with action line being discarded)
> > - because of the above, empty commits aren't discarded,
> >   their log is also included.
> > 
> > I am using it as follows:
> > 	git am -s < mailbox #creates first commit
> > 	hack ...
> > 	get mail with Ack
> > 	git commit --allow-empty -m `cat <<-EOF
> > 	ack! first
> > 
> > 	Acked-by: maintainer
> > 	EOF`
> > 	repeat cycle
> > 	git rebase --autosquash -i origin/master
> > 	before public branch push
> > 
> > The "cat" command above is actually a script that
> > parses the Ack mail to create the empty commit -
> > to be submitted separately.
> 
> This looks awfully complicated, still, and not very generic.
> 
> How about making it easier to use, and much, much more generic, like this?

I can look at using a different syntax but the below does not
support the workflow I described, which is a standard
email based one: get email, handle it.

> 1. introducing an `--add-footer` flag to `git commit` that you could use
> like this:
> 
> 	git commit --amend --add-footer "Acked-by: Bugs Bunny"
> 2. introducing an `--exec-after` flag to `git commit` that would be a new
> sibling of `--fixup` and `--squash` and would work like this:
> 
> 	git commit --exec-after HEAD~5 \
> 		'git commit --amend --add-footer "Acked-by: Bugs Bunny"'

But it wouldn't address my use-case where I get an ack
by email. If I have to dig up the relevant commit(s) by hand
anyway, then what was the point?

> 
> (it should imply `--allow-empty`, of course, and probably even fail if
> anything was staged for commit at that point.) The commit message would
> then look something like
> 
> 	exec-after! Fix broken breakage
> 
> 	git commit --amend --add-footer "Acked-by: Bugs Bunny"

So if I happen to fetch a branch from someone
and rebase it, stuff gets auto-executed on my local system?
That looks scary. 

> This way would obviously benefit a lot more users.

It might benefit others who have the commit handy but it does not look
like it helps email based workflow.

> For example, you could
> easily say (and alias)
> 
> 	git commit --amend --add-footer 'Reviewed-by: Arrested Developer"
> 
> i.e. support all kind of use cases where developers need to slap on
> footers in a quick & easy way.
>
> And the --exec-after option would obviously have *a lot* more use cases
> than just squashing in ACKs.
> 
> Ciao,
> Johannes

So far I only see examples of adding footers. If that's all we can think
up, why code in all this genericity?  All these small scripts scattered
around just make things hard to use, and add security issues.


-- 
MSR

Re: [PATCH 1/4] rebase -i: add ack action

[Christian Neukirchen]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> How about making it easier to use, and much, much more generic, like this?
>
> 1. introducing an `--add-footer` flag to `git commit` that you could use
> like this:
>
> 	git commit --amend --add-footer "Acked-by: Bugs Bunny"

I have a script where I currently do this ala:

GIT_EDITOR="git -c trailer.closes.ifExists=replace interpret-trailers \
        --trailer 'Closes: #$PR [via git-merge-pr]' --in-place" \
git commit --quiet --amend

But I think it could be a good addition to porcelain.
(interpret-trailers still feels hard to drive by a script...)

-- 
Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org

Re: [PATCH 1/4] rebase -i: add ack action

[Johannes Schindelin]

Hi Michael,

On Mon, 11 Apr 2016, Michael S. Tsirkin wrote:

> So far I only see examples of adding footers. If that's all we can think
> up, why code in all this genericity?

Because as far as I can see, the only benefitor of your patches would be
you.

Ciao,
Johannes

Re: [PATCH 1/4] rebase -i: add ack action

[Michael S. Tsirkin]

Repost, sorry about the noise.

On Mon, Apr 11, 2016 at 05:36:45PM +0200, Johannes Schindelin wrote:
> Hi Michael,
> 
> On Mon, 11 Apr 2016, Michael S. Tsirkin wrote:
> 
> > So far I only see examples of adding footers. If that's all we can think
> > up, why code in all this genericity?
> 
> Because as far as I can see, the only benefitor of your patches would be
> you.
> 
> Ciao,
> Johannes

This seems unlikely.  Just merging the patches won't benefit me directly
- I have maintained them in my tree for a couple of years now with very
little effort.  For sure, I could benefit if they get merged and then
someone improves them further - that was the point of posting them - but
then I'm not the only benefitor.

The workflow including getting acks for patches by email is not handled
well by upstream git right now.  It would surprise me if no one uses it
if it's upstream, as you seem to suggest.  But maybe most people moved
on and just do pull requests instead.

-- 
MST

Re: [PATCH 1/4] rebase -i: add ack action

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> Repost, sorry about the noise.
>
> On Mon, Apr 11, 2016 at 05:36:45PM +0200, Johannes Schindelin wrote:
>> Hi Michael,
>> 
>> On Mon, 11 Apr 2016, Michael S. Tsirkin wrote:
>> 
>> > So far I only see examples of adding footers. If that's all we can think
>> > up, why code in all this genericity?
>> 
>> Because as far as I can see, the only benefitor of your patches would be
>> you.
>> 
>> Ciao,
>> Johannes
>
> This seems unlikely.  Just merging the patches won't benefit me directly
> - I have maintained them in my tree for a couple of years now with very
> little effort.  For sure, I could benefit if they get merged and then
> someone improves them further - that was the point of posting them - but
> then I'm not the only benefitor.
>
> The workflow including getting acks for patches by email is not handled
> well by upstream git right now.  It would surprise me if no one uses it
> if it's upstream, as you seem to suggest.  But maybe most people moved
> on and just do pull requests instead.

I doubt I would use this in its current form myself.

Patch series I receive are all queued on their own separate topic
branches, and having to switch branches only to create a fake empty
commit to record received Acked-by and Reviewed-by is a chore that
serves only half of what needs to be done.  Once I decide to switch
back to the topic branch after receiving Acked-by and Reviewed-by,
I'd rather "rebase -i" to directly record them at that point, with
"reword".

If the "trailers" stuff is packaged into an easier-to-use format to
use with "git commit --amend", I may use that together with "exec"
to automatically add these while doing so, but again, I do not see
any need for fake empty commits out of received e-mails in the
resulting workflow.

That does not at all mean nobody other than Michael would use it,
though.

Re: [PATCH 1/4] rebase -i: add ack action

[Michael S. Tsirkin]

On Mon, Apr 11, 2016 at 12:48:22PM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > Repost, sorry about the noise.
> >
> > On Mon, Apr 11, 2016 at 05:36:45PM +0200, Johannes Schindelin wrote:
> >> Hi Michael,
> >> 
> >> On Mon, 11 Apr 2016, Michael S. Tsirkin wrote:
> >> 
> >> > So far I only see examples of adding footers. If that's all we can think
> >> > up, why code in all this genericity?
> >> 
> >> Because as far as I can see, the only benefitor of your patches would be
> >> you.
> >> 
> >> Ciao,
> >> Johannes
> >
> > This seems unlikely.  Just merging the patches won't benefit me directly
> > - I have maintained them in my tree for a couple of years now with very
> > little effort.  For sure, I could benefit if they get merged and then
> > someone improves them further - that was the point of posting them - but
> > then I'm not the only benefitor.
> >
> > The workflow including getting acks for patches by email is not handled
> > well by upstream git right now.  It would surprise me if no one uses it
> > if it's upstream, as you seem to suggest.  But maybe most people moved
> > on and just do pull requests instead.
> 
> I doubt I would use this in its current form myself.
> 
> Patch series I receive are all queued on their own separate topic
> branches, and having to switch branches only to create a fake empty
> commit to record received Acked-by and Reviewed-by is a chore that
> serves only half of what needs to be done.

Interesting. An empty commit would be rather easy to create on any
branch, not just the current one, using git-commit-tree.
Does it sounds interesting if I teach
git ack to get an active branch as a parameter?


> Once I decide to switch
> back to the topic branch after receiving Acked-by and Reviewed-by,
> I'd rather "rebase -i" to directly record them at that point, with
> "reword".
> 
> If the "trailers" stuff is packaged into an easier-to-use format to
> use with "git commit --amend", I may use that together with "exec"
> to automatically add these while doing so, but again, I do not see
> any need for fake empty commits out of received e-mails in the
> resulting workflow.
> 
> That does not at all mean nobody other than Michael would use it,
> though.

Re: [PATCH 1/4] rebase -i: add ack action

[Matthieu Moy]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Apr 11, 2016 at 12:48:22PM -0700, Junio C Hamano wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > Repost, sorry about the noise.
>> >
>> > On Mon, Apr 11, 2016 at 05:36:45PM +0200, Johannes Schindelin wrote:
>> >> Hi Michael,
>> >> 
>> >> On Mon, 11 Apr 2016, Michael S. Tsirkin wrote:
>> >> 
>> >> > So far I only see examples of adding footers. If that's all we can think
>> >> > up, why code in all this genericity?
>> >> 
>> >> Because as far as I can see, the only benefitor of your patches would be
>> >> you.
>> >> 
>> >> Ciao,
>> >> Johannes
>> >
>> > This seems unlikely.  Just merging the patches won't benefit me directly
>> > - I have maintained them in my tree for a couple of years now with very
>> > little effort.  For sure, I could benefit if they get merged and then
>> > someone improves them further - that was the point of posting them - but
>> > then I'm not the only benefitor.
>> >
>> > The workflow including getting acks for patches by email is not handled
>> > well by upstream git right now.  It would surprise me if no one uses it
>> > if it's upstream, as you seem to suggest.  But maybe most people moved
>> > on and just do pull requests instead.
>> 
>> I doubt I would use this in its current form myself.
>> 
>> Patch series I receive are all queued on their own separate topic
>> branches, and having to switch branches only to create a fake empty
>> commit to record received Acked-by and Reviewed-by is a chore that
>> serves only half of what needs to be done.
>
> Interesting. An empty commit would be rather easy to create on any
> branch, not just the current one, using git-commit-tree.

This "modify a branch without checking-it out" makes me think of "git
notes". It may make sense to teach "git rebase -i" to look for notes in
rebased commits and append them to the commit message when applying.
Just an idea, not necessarily a good one ;-).

> Does it sounds interesting if I teach
> git ack to get an active branch as a parameter?

I think "ack" is not a good name for this feature: you use it to append
"Acked-by", but it can be used to append any trailer (for example,
Reviewed-by: would make complete sense too). I think using a better name
would help the discussion (to remove the "it's my use-case" biais).
Perhaps "append"?

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

Re: [PATCH 1/4] rebase -i: add ack action

[Michael S. Tsirkin]

On Mon, Apr 11, 2016 at 10:03:39PM +0200, Matthieu Moy wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Mon, Apr 11, 2016 at 12:48:22PM -0700, Junio C Hamano wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> > Repost, sorry about the noise.
> >> >
> >> > On Mon, Apr 11, 2016 at 05:36:45PM +0200, Johannes Schindelin wrote:
> >> >> Hi Michael,
> >> >> 
> >> >> On Mon, 11 Apr 2016, Michael S. Tsirkin wrote:
> >> >> 
> >> >> > So far I only see examples of adding footers. If that's all we can think
> >> >> > up, why code in all this genericity?
> >> >> 
> >> >> Because as far as I can see, the only benefitor of your patches would be
> >> >> you.
> >> >> 
> >> >> Ciao,
> >> >> Johannes
> >> >
> >> > This seems unlikely.  Just merging the patches won't benefit me directly
> >> > - I have maintained them in my tree for a couple of years now with very
> >> > little effort.  For sure, I could benefit if they get merged and then
> >> > someone improves them further - that was the point of posting them - but
> >> > then I'm not the only benefitor.
> >> >
> >> > The workflow including getting acks for patches by email is not handled
> >> > well by upstream git right now.  It would surprise me if no one uses it
> >> > if it's upstream, as you seem to suggest.  But maybe most people moved
> >> > on and just do pull requests instead.
> >> 
> >> I doubt I would use this in its current form myself.
> >> 
> >> Patch series I receive are all queued on their own separate topic
> >> branches, and having to switch branches only to create a fake empty
> >> commit to record received Acked-by and Reviewed-by is a chore that
> >> serves only half of what needs to be done.
> >
> > Interesting. An empty commit would be rather easy to create on any
> > branch, not just the current one, using git-commit-tree.
> 
> This "modify a branch without checking-it out" makes me think of "git
> notes". It may make sense to teach "git rebase -i" to look for notes in
> rebased commits and append them to the commit message when applying.
> Just an idea, not necessarily a good one ;-).

Two things making it harder
	- machinery to look for commits is part of git rebase anyway
	- notes are expected to come after --- at the moment


> > Does it sounds interesting if I teach
> > git ack to get an active branch as a parameter?
> 
> I think "ack" is not a good name for this feature: you use it to append
> "Acked-by", but it can be used to append any trailer (for example,
> Reviewed-by: would make complete sense too).

Yes - I use it to append all trailers.

> I think using a better name
> would help the discussion (to remove the "it's my use-case" biais).
> Perhaps "append"?

Or "trailer".

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

Re: [PATCH 1/4] rebase -i: add ack action

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
>> Interesting. An empty commit would be rather easy to create on any
>> branch, not just the current one, using git-commit-tree.
>
> This "modify a branch without checking-it out" makes me think of "git
> notes". It may make sense to teach "git rebase -i" to look for notes in
> rebased commits and append them to the commit message when applying.
> Just an idea, not necessarily a good one ;-).

Yeah that may actually fly well, as a note is designed to attach to
an exact commit, not to a branch, so that feels more natural.

As to the "use commit-tree", well, personally I am not interested in
a solution that can work well in my workflow ONLY if I further script
it.  That's half-solution and unless that half is done very well,
I'd rather do a full solution better.

	Note: this is a continuation of "I personally would not use
	it, even though other people might" discussion.

I was also wondering if I should just script around filter-branch,
if all I am futzing with is the data in the trailer block, doing the
munging of the trailer block with interpret-trailers, naturally.

In any case, a recent occasion that I had to do something related to
this topic may illustrate the boundary of requirements:

    Two developers, Michael and David, are involved.  David sends a
    24-patch series, some of which were written by Michael and
    others by David.  The in-body "From:" lines are set right and
    the resulting patches record authorship correctly.

    Michael reminds David that patches authored by Michael still
    need to be signed-off by David.  David sends a single message
    "those by Michael in this series are signed off by me".

    Michael also says that he reviewed all patches authored by
    David, i.e. "Add Acked-by Michael to all patches in this series
    authored by David".

Now this is an extreme case where a simple "OK I received an
e-mailed Ack, so I can rely on the subject line matching to mark it
to be squashed" approach will never work (i.e. if we were automating
it I'd expect that the script in DSL to the automation machinery to
take at last as many (conceptual) bits as the above problem
description).

Re: [PATCH 1/4] rebase -i: add ack action

[Michael S. Tsirkin]

On Tue, Apr 12, 2016 at 09:07:30AM -0700, Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> >
> >> Interesting. An empty commit would be rather easy to create on any
> >> branch, not just the current one, using git-commit-tree.
> >
> > This "modify a branch without checking-it out" makes me think of "git
> > notes". It may make sense to teach "git rebase -i" to look for notes in
> > rebased commits and append them to the commit message when applying.
> > Just an idea, not necessarily a good one ;-).
> 
> Yeah that may actually fly well, as a note is designed to attach to
> an exact commit, not to a branch, so that feels more natural.

We'd have to invent a way to show that in the rebase -i output though.


> As to the "use commit-tree", well, personally I am not interested in
> a solution that can work well in my workflow ONLY if I further script
> it.  That's half-solution and unless that half is done very well,
> I'd rather do a full solution better.

Absolutely. But that's not what I meant. I will add a flag to git-ack to
select a branch and use commit-tree to put the ack commit there
*internally*. Would this do everything you need? How do you select
a branch? Automatically or do you remember the mapping from topic
to branch name?

> 	Note: this is a continuation of "I personally would not use
> 	it, even though other people might" discussion.
> 
> I was also wondering if I should just script around filter-branch,
> if all I am futzing with is the data in the trailer block, doing the
> munging of the trailer block with interpret-trailers, naturally.
> 
> In any case, a recent occasion that I had to do something related to
> this topic may illustrate the boundary of requirements:
> 
>     Two developers, Michael and David, are involved.  David sends a
>     24-patch series, some of which were written by Michael and
>     others by David.  The in-body "From:" lines are set right and
>     the resulting patches record authorship correctly.
> 
>     Michael reminds David that patches authored by Michael still
>     need to be signed-off by David.  David sends a single message
>     "those by Michael in this series are signed off by me".
> 
>     Michael also says that he reviewed all patches authored by
>     David, i.e. "Add Acked-by Michael to all patches in this series
>     authored by David".
> 
> Now this is an extreme case where a simple "OK I received an
> e-mailed Ack, so I can rely on the subject line matching to mark it
> to be squashed" approach will never work (i.e. if we were automating
> it I'd expect that the script in DSL to the automation machinery to
> take at last as many (conceptual) bits as the above problem
> description).

So here's how I solve the second part for now - that
is very common: I expect Michael to write something like
For series:
Acked-by: Michael S. Tsirkin <mst@redhat.com>

then I run git ack -s to put the signature in a file .git/ACKS.

(git ack -s is just writing acks into .git/ACKS so
if the email format is wrong I just edit it manually).

And then I tag the series in email and run git ack -r to
add the ack tag.

For first part, that is less common but also happens
(for example I get "for patches 1,7 and 23 in series: ACK") -
I would do git ack -s
to store David's signoff, then tag just messages by David
(probably just using limit ~b From:\ David in mutt)
and pipe them to git ack -r.

Does this sound user-friendly enough? What would you do
differently?

-- 
MST

Re: [PATCH 1/4] rebase -i: add ack action

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

>> As to the "use commit-tree", well, personally I am not interested in
>> a solution that can work well in my workflow ONLY if I further script
>> it.  That's half-solution and unless that half is done very well,
>> I'd rather do a full solution better.
>
> Absolutely. But that's not what I meant. I will add a flag to git-ack to
> select a branch and use commit-tree to put the ack commit there
> *internally*. Would this do everything you need? How do you select
> a branch? Automatically or do you remember the mapping from topic
> to branch name?
> ...
> For first part, that is less common but also happens
> (for example I get "for patches 1,7 and 23 in series: ACK") -
> I would do git ack -s
> to store David's signoff, then tag just messages by David
> (probably just using limit ~b From:\ David in mutt)
> and pipe them to git ack -r.
>
> Does this sound user-friendly enough? What would you do
> differently?

Because my workflow is I usually comment on and review many topics
without applying them to anywhere, by the time I start applying
patches, I often know which one got Acked and Reviewed already.

So for them the workflow would be

 0. Think which maintenance track the topic should be based on.

 1. Fork

    $ git checkout -b <new topic> maint-<appropriate track>

 2. In my MUA, pipe the message into this pipeline

    Meta/add-by -r peff@ -a Tsirkin | git am -s3c

where the "add-by" script (found in 'todo') expands the given names
using .mailmap and appends appropriate trailers (the latter should
eventually be updated to use interpret-trailers when the tool
matures, but I did not think it was there yet when I last updated
the "add-by" script).

So for a use case where I work off of my MUA, I have no use for your
"git ack".