[PATCH 0/4] ack recoding in commit log

[PATCH 0/4] ack recoding in commit log

[Michael S. Tsirkin]

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

I like making acks commits,
this way they are easy to keep track of
as part of git history.


Since response mail happens to have appropriate
subject matching the patch, it's a natural fit to
then use git rebase mechanics if we want to smash
these acks into the original commit.

I have been using these patches without any problems
for a while 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: yes, I think notes+git replace could be used for this too,
my workflow always includes a rebase with --autosquash
before publishing anyway, so dealing with ack as with any
other commit is nicer.


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              | 79 ++++++++++++++++++++++++++++++++++++++++++++
 git-rebase--interactive.sh   | 34 +++++++++++++++----
 t/t3415-rebase-autosquash.sh | 15 +++++++++
 4 files changed, 161 insertions(+), 12 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 | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6ec9d3c..821872c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -140,6 +140,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
 
@@ -412,6 +413,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:"
@@ -453,7 +463,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
@@ -521,8 +531,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
 			;;
@@ -546,7 +559,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" \
@@ -557,7 +570,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
@@ -690,7 +703,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 ||
@@ -732,7 +745,7 @@ rearrange_squash () {
 	while read -r pick sha1 message
 	do
 		case "$message" in
-		"squash! "*|"fixup! "*)
+		"squash! "*|"fixup! "*|"ack! "*)
 			action="${message%%!*}"
 			rest=$message
 			prefix=
@@ -740,7 +753,7 @@ rearrange_squash () {
 			while :
 			do
 				case "$rest" in
-				"squash! "*|"fixup! "*)
+				"squash! "*|"fixup! "* |"ack! "*)
 					prefix="$prefix${rest%%!*},"
 					rest="${rest#*! }"
 					;;
@@ -975,6 +988,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 $shortsha1 $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 2a93c64..c27aef4 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -384,7 +384,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.
 
 --root::
 	Rebase all commits reachable from <branch>, instead of
@@ -398,13 +398,13 @@ squash/fixup series.
 
 --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.
@@ -624,6 +624,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 utomticlly 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 41370ab..9d7db13 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.
I don't use these flags much so they likely work
less well.

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 | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)
 create mode 100755 contrib/git-ack

diff --git a/contrib/git-ack b/contrib/git-ack
new file mode 100755
index 0000000..4aeb16a
--- /dev/null
+++ b/contrib/git-ack
@@ -0,0 +1,84 @@
+msg=`mktemp`
+patch=`mktemp`
+info=`git mailinfo $msg $patch`
+subj=`echo "$info"|sed -n 's/^Subject: //p'`
+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 --cached HEAD
+then
+    nop < /dev/null
+else
+    echo "DIFF in cache. Not acked, reset or commit!"
+    exit 1
+fi
+GIT_DIR=`pwd`/${GIT_DIR}
+
+usage () {
+	echo "Usage: git ack " \
+            "[-s|--save|-a|--append|-r|--restore |-c|--clear]\n" >& 2;
+        exit 1;
+}
+
+append=
+save=
+clear=
+
+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
+fi
+
+if
+    test "$restore"
+then
+    msg = ${GIT_DIR}/ACKS
+fi
+
+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 2/4] git-rebase: document ack

[Eric Sunshine]

On Sun, May 18, 2014 at 5:17 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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 2a93c64..c27aef4 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -384,7 +384,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.
>
>  --root::
>         Rebase all commits reachable from <branch>, instead of
> @@ -398,13 +398,13 @@ squash/fixup series.
>
>  --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.
> @@ -624,6 +624,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 utomticlly changed from `pick` to `ack`

s/utomticlly/automatically/

> +to cause them to be squashed into the acked commit.
>
>  RECOVERING FROM UPSTREAM REBASE
>  -------------------------------
> --
> MST
>

Re: [PATCH 3/4] rebase: test ack

[Junio C Hamano]

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

> test ack! handling
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Will queue with this squashed in.

4/4 seems to have some style issues as well, but I didn't look very
closely.

Thanks.

 t/t3415-rebase-autosquash.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 9d7db13..dcdba6f 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -75,18 +75,18 @@ test_expect_success 'auto squash (option)' '
 '
 
 test_expect_success 'auto ack' '
-	ack="Acked-by: xyz"
-	msg=$(test_write_lines "ack! first commit" "" "$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
+	git show -s first-commit | grep -v ^commit >expected-msg &&
+	echo "    $ack" >>expected-msg &&
+	git show -s HEAD^ | grep -v ^commit >actual-msg &&
+	test_cmp actual-msg expected-msg
 '
 
 test_expect_success 'auto squash (config)' '

Re: [PATCH 3/4] rebase: test ack

[Michael S. Tsirkin]

On Mon, May 19, 2014 at 02:34:26PM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > test ack! handling
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Will queue with this squashed in.

Thanks! And sorry about the style issues.

> 4/4 seems to have some style issues as well, but I didn't look very
> closely.

I'll try to clean it for the next submission.
I'll be glad to hear about them as well.
Thanks!

> Thanks.
> 
>  t/t3415-rebase-autosquash.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index 9d7db13..dcdba6f 100755
> --- a/t/t3415-rebase-autosquash.sh
> +++ b/t/t3415-rebase-autosquash.sh
> @@ -75,18 +75,18 @@ test_expect_success 'auto squash (option)' '
>  '
>  
>  test_expect_success 'auto ack' '
> -	ack="Acked-by: xyz"
> -	msg=$(test_write_lines "ack! first commit" "" "$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
> +	git show -s first-commit | grep -v ^commit >expected-msg &&
> +	echo "    $ack" >>expected-msg &&
> +	git show -s HEAD^ | grep -v ^commit >actual-msg &&
> +	test_cmp actual-msg expected-msg
>  '
>  
>  test_expect_success 'auto squash (config)' '

Re: [PATCH 3/4] rebase: test ack

[Michael S. Tsirkin]

On Mon, May 19, 2014 at 02:34:26PM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > test ack! handling
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Will queue with this squashed in.
> 
> 4/4 seems to have some style issues as well, but I didn't look very
> closely.
> 
> Thanks.

Just to clarify I can post v2 of 4/4 without reposting 1-3 since they
are queued?

>  t/t3415-rebase-autosquash.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index 9d7db13..dcdba6f 100755
> --- a/t/t3415-rebase-autosquash.sh
> +++ b/t/t3415-rebase-autosquash.sh
> @@ -75,18 +75,18 @@ test_expect_success 'auto squash (option)' '
>  '
>  
>  test_expect_success 'auto ack' '
> -	ack="Acked-by: xyz"
> -	msg=$(test_write_lines "ack! first commit" "" "$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
> +	git show -s first-commit | grep -v ^commit >expected-msg &&
> +	echo "    $ack" >>expected-msg &&
> +	git show -s HEAD^ | grep -v ^commit >actual-msg &&
> +	test_cmp actual-msg expected-msg
>  '
>  
>  test_expect_success 'auto squash (config)' '

Re: [PATCH 3/4] rebase: test ack

[Junio C Hamano]

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

> Just to clarify I can post v2 of 4/4 without reposting 1-3 since they
> are queued?

If you need to update anything queued only on 'pu' but not yet in
'next', it is customary to resend the whole for everybody to see
(what is already in 'next' should only be built upon incrementally
and not updated with replacement patches), while noting which ones
are the same as before.  Christian Couder has been doing it nicely
in his recent rerolls (if the series were not in 'next', that is).

Thanks.

Re: [PATCH 3/4] rebase: test ack

[Michael S. Tsirkin]

On Tue, May 20, 2014 at 08:13:27AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > Just to clarify I can post v2 of 4/4 without reposting 1-3 since they
> > are queued?
> 
> If you need to update anything queued only on 'pu' but not yet in
> 'next', it is customary to resend the whole for everybody to see
> (what is already in 'next' should only be built upon incrementally
> and not updated with replacement patches), while noting which ones
> are the same as before.  Christian Couder has been doing it nicely
> in his recent rerolls (if the series were not in 'next', that is).
> 
> Thanks.

Actually I don't see anything like it in pu.
What I would like is for 1-3 to be in pu,
4/4 was for illustrative purposes it's not yet
ready for that, and 1-3 are useful by themselves.
I could then iterate on 4/4 without reposting 1-3.

Re: [PATCH 3/4] rebase: test ack

[Junio C Hamano]

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

> On Tue, May 20, 2014 at 08:13:27AM -0700, Junio C Hamano wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > Just to clarify I can post v2 of 4/4 without reposting 1-3 since they
>> > are queued?
>> 
>> If you need to update anything queued only on 'pu' but not yet in
>> 'next', it is customary to resend the whole for everybody to see
>> (what is already in 'next' should only be built upon incrementally
>> and not updated with replacement patches), while noting which ones
>> are the same as before.  Christian Couder has been doing it nicely
>> in his recent rerolls (if the series were not in 'next', that is).
>> 
>> Thanks.
>
> Actually I don't see anything like it in pu.

The way I usually work is to apply a non-fix (i.e. enhancement)
series on a topic branch forked from 'master' (or the last tagged
version contained in its tip) and see if it makes sense, and then
try-merge the result to 'next' to see if it is free of potential
funny interactions with other topics that are already in flight.
After that happens, the topic branch is merged to somewhere in 'pu'.

It is possible that I did not have time to go through all the steps
above (after all, I had to make another -rc release and there was an
unexpected last-minute change of plans in the morning that blew a
few hours of work).  Or there may have been some merge conflicts
that I didn't feel like resolving for various reasons (e.g. if I
knew the series would be rerolled anyway, it can wait; if the other
topic that interacts with your series has been cooking sufficiently
long in 'next' and if it is very close to the final release of this
cycle, it may be easier to wait for the other topic to graduate to
'master', which would happen soon after this cycle finishes, and ask
you to rebase your series).

I don't remember which ;-)

Re: [PATCH 3/4] rebase: test ack

[Michael S. Tsirkin]

On Wed, May 21, 2014 at 09:54:47AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Tue, May 20, 2014 at 08:13:27AM -0700, Junio C Hamano wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> > Just to clarify I can post v2 of 4/4 without reposting 1-3 since they
> >> > are queued?
> >> 
> >> If you need to update anything queued only on 'pu' but not yet in
> >> 'next', it is customary to resend the whole for everybody to see
> >> (what is already in 'next' should only be built upon incrementally
> >> and not updated with replacement patches), while noting which ones
> >> are the same as before.  Christian Couder has been doing it nicely
> >> in his recent rerolls (if the series were not in 'next', that is).
> >> 
> >> Thanks.
> >
> > Actually I don't see anything like it in pu.
> 
> The way I usually work is to apply a non-fix (i.e. enhancement)
> series on a topic branch forked from 'master' (or the last tagged
> version contained in its tip) and see if it makes sense, and then
> try-merge the result to 'next' to see if it is free of potential
> funny interactions with other topics that are already in flight.
> After that happens, the topic branch is merged to somewhere in 'pu'.
> 
> It is possible that I did not have time to go through all the steps
> above (after all, I had to make another -rc release and there was an
> unexpected last-minute change of plans in the morning that blew a
> few hours of work).  Or there may have been some merge conflicts
> that I didn't feel like resolving for various reasons (e.g. if I
> knew the series would be rerolled anyway, it can wait; if the other
> topic that interacts with your series has been cooking sufficiently
> long in 'next' and if it is very close to the final release of this
> cycle, it may be easier to wait for the other topic to graduate to
> 'master', which would happen soon after this cycle finishes, and ask
> you to rebase your series).
> 
> I don't remember which ;-)
> 

Oh sorry, didn't mean to try to pressure you. I was just surprised
not to see it there. I know this applies cleanly to next so I'll just
wait for 2.0 to be out.

-- 
MST

Re: [PATCH 3/4] rebase: test ack

[Junio C Hamano]

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

> On Wed, May 21, 2014 at 09:54:47AM -0700, Junio C Hamano wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Tue, May 20, 2014 at 08:13:27AM -0700, Junio C Hamano wrote:
>> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> >> 
>> >> > Just to clarify I can post v2 of 4/4 without reposting 1-3 since they
>> >> > are queued?
>> >> 
>> >> If you need to update anything queued only on 'pu' but not yet in
>> >> 'next', it is customary to ...
>> >
>> > Actually I don't see anything like it in pu.
>> ... 
> Oh sorry, didn't mean to try to pressure you. I was just surprised
> not to see it there. I know this applies cleanly to next so I'll just
> wait for 2.0 to be out.

Oh, no.  No pressure felt and no need to be sorry about anything.

I described a preferred procedure when the topic appeared in 'pu',
and I didn't answer your question for topics that are not even in
'pu' yet.

Being in 'pu' and not in 'next' is not much different from not being
in 'pu', so the preferred procedure is to send out the entire series
(unless it is a large 47-patch series ;-) to give everybody another
chance to comment, and it would be extra nice if you indicated which
ones are unchanged since the previous round to help those who did
already saw them.

Thanks.

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

[Fabian Ruch]

Hi Michael,

I have some inline comments below. Also, some parts of the patch do not
adhere to the style rules

 - tabs for indentation
 - $(...) for command substitution
 - no space after redirection operators
 - double-quotes around redirection targets

for shell scripts (from the file `Documentation/CodingGuidelines`).

On 05/18/2014 11:17 PM, Michael S. Tsirkin wrote:
> diff --git a/contrib/git-ack b/contrib/git-ack
> new file mode 100755
> index 0000000..4aeb16a
> --- /dev/null
> +++ b/contrib/git-ack
> @@ -0,0 +1,84 @@
> +msg=`mktemp`
> +patch=`mktemp`
> +info=`git mailinfo $msg $patch`
> +subj=`echo "$info"|sed -n 's/^Subject: //p'`
> +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 --cached HEAD

If I am not mistaken, the exit code of `git-diff(1)` doesn't change
according to whether there are differences or not, unless the option
`--exit-code` is given.

> +then
> +    nop < /dev/null

Is it correct that this is a do-nothing operation? Is that a common
idiom? I found the null command (`:`, colon) to be used in many places
instead.

> +else
> +    echo "DIFF in cache. Not acked, reset or commit!"
> +    exit 1
> +fi
> +GIT_DIR=`pwd`/${GIT_DIR}

This seems incorrect to me. If `GIT_DIR` is already set, it might point
to an absolute path and not `.git`. If the environment variable is not
set, the state file `ACKS` ends up in the working directory.

Maybe `git-sh-setup(1)` can be of help. It uses

    git rev-parse --git-dir

to probe the path to the .git directory.

> +
> +usage () {
> +	echo "Usage: git ack " \
> +            "[-s|--save|-a|--append|-r|--restore |-c|--clear]\n" >& 2;
> +        exit 1;
> +}
> +
> +append=
> +save=
> +clear=

The restore flag seems to be missing from this list of declarations.

> +
> +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
> +fi
> +
> +if
> +    test "$restore"
> +then
> +    msg = ${GIT_DIR}/ACKS
> +fi
> +
> +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

   Fabian

Re: [PATCH 0/4] ack recoding in commit log

[Fabian Ruch]

Hi Michael,

On 05/18/2014 11:17 PM, Michael S. Tsirkin wrote:
> As a maintainer, I often get patches by mail, then
> acked-by,reviewed-by etc responses are sent by separate
> mail.
> 
> I like making acks commits,
> this way they are easy to keep track of
> as part of git history.

In order to fully understand your additions, I think, I need some
clarification on the term "ack commit". What is an ack commit exactly?
Suppose our principal commit has the commit message

    Some changes

    The changes are...

    Signed-off-by: A U Thor <author@example.com>

and we receive an email from Somebody saying

    > Some changes
    >
    > The changes are...
    >
    > Signed-off-by: A U Thor <author@example.com>

    Reviewed-by: Somebody <somebody@example.com>

. Now, if I understand correctly, we create an empty commit on top of
the principal commit using the following commit message.

    Some changes

    Reviewed-by: Somebody <somebody@example.com>

Is this commit then called an ack commit?

Can an ack commit be non-empty?

Is a commit still an ack if its description mentions additional text
between the subject and the tag lines?

Maybe the ack command for todo lists and ack commits have little to do
with one another. If we stick to the term "ack commit", then the command
name suggests that it takes the tags from some commit b and appends them
to the list of tags in the previous commit's (a) message:

    pick a A commit
    ack  b The next commit

However, this obviously does not work by just appending messages. For
instance, there could be additional text before or after some tag line
in either commit message. If we treat the workflow you described as a
very specific use case of the ack command instead, it seems reasonable
to add such a todo list functionality for melding commits by silently
appending messages. However, we might consider parametrizing a single
squash command instead of defining just another name that one has to
keep in mind for melding commits:

    pick             a A commit
    squash --no-edit b The next commit

> Since response mail happens to have appropriate
> subject matching the patch, it's a natural fit to
> then use git rebase mechanics if we want to smash
> these acks into the original commit.
> 
> I have been using these patches without any problems
> for a while 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
> 
> [..]

Thanks for your time,
   Fabian

Re: [PATCH 0/4] ack recoding in commit log

[Michael S. Tsirkin]

On Wed, Jun 11, 2014 at 10:05:46AM +0200, Fabian Ruch wrote:
> Hi Michael,
> 
> On 05/18/2014 11:17 PM, Michael S. Tsirkin wrote:
> > As a maintainer, I often get patches by mail, then
> > acked-by,reviewed-by etc responses are sent by separate
> > mail.
> > 
> > I like making acks commits,
> > this way they are easy to keep track of
> > as part of git history.
> 
> In order to fully understand your additions, I think, I need some
> clarification on the term "ack commit".

I think the two additions should be judged separately, they aren't
related.

So I am not sure we need to spend time discussing
what is an ack commit.

What I mean is that I prefer using git history for
recording activity, and git rebase for modifying
history.
I think I am not alone in this.

To record acks, I wrote this tool to
create empty commits, that only record log
changes. They are named specially such that
rebase knows not to discard them, and to
automate the editing stage.

I added some features to the tool since.
Did not yet address all comments I got here, but just in case
you are curious it's attached at the end.

> What is an ack commit exactly?
> Suppose our principal commit has the commit message
> 
>     Some changes
> 
>     The changes are...
> 
>     Signed-off-by: A U Thor <author@example.com>
> 
> and we receive an email from Somebody saying
> 
>     > Some changes
>     >
>     > The changes are...
>     >
>     > Signed-off-by: A U Thor <author@example.com>
> 
>     Reviewed-by: Somebody <somebody@example.com>

Other possible cases I handle:
    people reply with:

    For the series:
        Reviewed-by: Somebody <somebody@example.com>

This uses the save/restore flags.

or an informal
    Ack the series
(this one I handle by creating ACKS file manually,
 then applying it with git ack restore)

Someone sends a fixup! patch, and someone else
replies to the fixup with Reviewed-by: tag.
(this was not handled correctly in the version
you reviewed, I attach latest one below where it is
handled correctly).


> . Now, if I understand correctly, we create an empty commit on top of
> the principal commit using the following commit message.
> 
>     Some changes
> 
>     Reviewed-by: Somebody <somebody@example.com>
> 
> Is this commit then called an ack commit?

git ack creates commits like this:

----
ack! Some changes

Reviewed-by: Somebody <somebody@example.com>
---

Other work-flows


> Can an ack commit be non-empty?

At the moment git ack does not create non empty commits.

> Is a commit still an ack if its description mentions additional text
> between the subject and the tag lines?

At the moment git ack does not create such commits.

> Maybe the ack command for todo lists and ack commits have little to do
> with one another.

I use them together but ack command is more powerful than git ack.
Same as commit --fixup, that is not the only way to
use fixup!.


> If we stick to the term "ack commit", then the command
> name suggests that it takes the tags from some commit b and appends them
> to the list of tags in the previous commit's (a) message:
> 
>     pick a A commit
>     ack  b The next commit
> 
> However, this obviously does not work by just appending messages. For
> instance, there could be additional text before or after some tag line
> in either commit message. If we treat the workflow you described as a
> very specific use case of the ack command instead, it seems reasonable
> to add such a todo list functionality for melding commits by silently
> appending messages. However, we might consider parametrizing a single
> squash command instead of defining just another name that one has to
> keep in mind for melding commits:

Hmm, I don't see why is flag any easier to remember than a separate command
frankly, and there's help text at the end to remind you.

> 
>     pick             a A commit
>     squash --no-edit b The next commit

That's too much typing I think, I just want to use a
single letter.
You could argue the same thing for fixup right?
It's really squash but discards the log, and there
is no requirement that it fixes anything, it can
add functionality or whatever.
So why not
squash --discardlog b The next commit

I think that would be bad usability, and so would --no-edit.

--no-edit also does not convey the fact that it allows
empty commits.


I don't mind renaming the command to something else
but I could not come up with a better name so far.

> > Since response mail happens to have appropriate
> > subject matching the patch, it's a natural fit to
> > then use git rebase mechanics if we want to smash
> > these acks into the original commit.
> > 
> > I have been using these patches without any problems
> > for a while 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
> > 
> > [..]
> 
> Thanks for your time,
>    Fabian


## latest git-ack
## changes: handle fixup!/squash!/ack! messages in mail

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 --cached HEAD
then
    nop < /dev/null
else
    echo "DIFF in cache. Not acked, reset or commit!"
    exit 1
fi
GIT_DIR=`pwd`/.git

usage () {
	echo "Usage: git ack " \
            "[-s|--save|-a|--append|-r|--restore |-c|--clear]\n" >& 2;
        exit 1;
}

append=
save=
clear=

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
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

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".

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]

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 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

[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 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

[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]

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

[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

[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

[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

[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

[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