[RFC PATCH 0/4] interpret-trailers: introduce "move" action

[RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Paolo Bonzini]

The purpose of this action is for scripts to be able to keep the
user's Signed-off-by at the end.  For example say I have a script
that adds a Reviewed-by tag:

    #! /bin/sh
    them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
    trailer="Reviewed-by: $them"
    git log -1 --pretty=format:%B | \
      git interpret-trailers --where end --if-exists doNothing --trailer "$trailer" | \
      git commit --amend -F-

Now, this script will leave my Signed-off-by line in a non-canonical
place, like

   Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
   Reviewed-by: Junio C Hamano <gitster@pobox.com>

This new option enables the following improvement:

    #! /bin/sh
    me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')
    them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
    trailer="Reviewed-by: $them"
    sob="Signed-off-by: $me"
    git log -1 --pretty=format:%B | \
      git interpret-trailers --where end --if-exists doNothing --trailer "$trailer" \
                             --where end --if-exists move --if-missing doNothing --trailer "$sob" | \
      git commit --amend -F-

which lets me keep the SoB line at the end, as it should be.
Posting as RFC because it's possible that I'm missing a simpler
way to achieve this...

Paolo Bonzini (4):
  trailer: push free_arg_item up
  trailer: simplify check_if_different
  trailer: create a new function to handle adding trailers
  trailer: add "move" configuration for trailer.ifExists

 Documentation/git-interpret-trailers.txt |  13 ++-
 t/t7513-interpret-trailers.sh            |  37 +++++++
 trailer.c                                | 169 ++++++++++++++++++-------------
 trailer.h                                |   1 +
 4 files changed, 149 insertions(+), 71 deletions(-)

-- 
2.14.2

[PATCH 1/4] trailer: push free_arg_item up

[Paolo Bonzini]

All callees of process_trailers_lists are calling free_arg_item.
Just do it in process_trailers_lists itself.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 trailer.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/trailer.c b/trailer.c
index 3ba157ed0..4ba28ae33 100644
--- a/trailer.c
+++ b/trailer.c
@@ -178,7 +178,6 @@ static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
 	new->token = arg_tok->token;
 	new->value = arg_tok->value;
 	arg_tok->token = arg_tok->value = NULL;
-	free_arg_item(arg_tok);
 	return new;
 }
 
@@ -271,7 +270,6 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
 {
 	switch (arg_tok->conf.if_exists) {
 	case EXISTS_DO_NOTHING:
-		free_arg_item(arg_tok);
 		break;
 	case EXISTS_REPLACE:
 		apply_item_command(in_tok, arg_tok);
@@ -287,15 +285,11 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
 		apply_item_command(in_tok, arg_tok);
 		if (check_if_different(in_tok, arg_tok, 1, head))
 			add_arg_to_input_list(on_tok, arg_tok);
-		else
-			free_arg_item(arg_tok);
 		break;
 	case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
 		apply_item_command(in_tok, arg_tok);
 		if (check_if_different(on_tok, arg_tok, 0, head))
 			add_arg_to_input_list(on_tok, arg_tok);
-		else
-			free_arg_item(arg_tok);
 		break;
 	default:
 		die("BUG: trailer.c: unhandled value %d",
@@ -311,7 +305,6 @@ static void apply_arg_if_missing(struct list_head *head,
 
 	switch (arg_tok->conf.if_missing) {
 	case MISSING_DO_NOTHING:
-		free_arg_item(arg_tok);
 		break;
 	case MISSING_ADD:
 		where = arg_tok->conf.where;
@@ -374,6 +367,8 @@ static void process_trailers_lists(struct list_head *head,
 
 		if (!applied)
 			apply_arg_if_missing(head, arg_tok);
+
+		free_arg_item(arg_tok);
 	}
 }
 
-- 
2.14.2

[PATCH 2/4] trailer: simplify check_if_different

[Paolo Bonzini]

The check_all argument is pointless, because the function degenerates
to !same_trailer when check_all==0 (if same_trailer fails, it always
ends up returning 1).  Remove it, switching the check_all==0 caller
to use same_trailer directly.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 trailer.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/trailer.c b/trailer.c
index 4ba28ae33..91f89db7f 100644
--- a/trailer.c
+++ b/trailer.c
@@ -194,14 +194,11 @@ static void add_arg_to_input_list(struct trailer_item *on_tok,
 
 static int check_if_different(struct trailer_item *in_tok,
 			      struct arg_item *arg_tok,
-			      int check_all,
 			      struct list_head *head)
 {
 	enum trailer_where where = arg_tok->conf.where;
 	struct list_head *next_head;
-	do {
-		if (same_trailer(in_tok, arg_tok))
-			return 0;
+	while (!same_trailer(in_tok, arg_tok)) {
 		/*
 		 * if we want to add a trailer after another one,
 		 * we have to check those before this one
@@ -209,10 +206,10 @@ static int check_if_different(struct trailer_item *in_tok,
 		next_head = after_or_end(where) ? in_tok->list.prev
 						: in_tok->list.next;
 		if (next_head == head)
-			break;
+			return 1;
 		in_tok = list_entry(next_head, struct trailer_item, list);
-	} while (check_all);
-	return 1;
+	}
+	return 0;
 }
 
 static char *apply_command(const char *command, const char *arg)
@@ -283,12 +280,12 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
 		break;
 	case EXISTS_ADD_IF_DIFFERENT:
 		apply_item_command(in_tok, arg_tok);
-		if (check_if_different(in_tok, arg_tok, 1, head))
+		if (check_if_different(in_tok, arg_tok, head))
 			add_arg_to_input_list(on_tok, arg_tok);
 		break;
 	case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
 		apply_item_command(in_tok, arg_tok);
-		if (check_if_different(on_tok, arg_tok, 0, head))
+		if (!same_trailer(on_tok, arg_tok))
 			add_arg_to_input_list(on_tok, arg_tok);
 		break;
 	default:
-- 
2.14.2

[PATCH 3/4] trailer: create a new function to handle adding trailers

[Paolo Bonzini]

Create a new function apply_arg that takes care of computing the new
trailer's "neighbor", checking for duplicates through a pluggable
callback, and adding the new argument according to "trailer.where".

Rename after_or_end, and don't use it in apply_arg.  It's a coincidence
that the conditions for "scan backwards" and "add after" are the same.

This simplifies find_same_and_apply_arg so that it does exactly what
the name says.  apply_arg_if_missing can also use the new function;
before, it was redoing add_arg_to_input_list's job in a slightly
different fashion.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 trailer.c | 125 +++++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 75 insertions(+), 50 deletions(-)

diff --git a/trailer.c b/trailer.c
index 91f89db7f..ce0d94074 100644
--- a/trailer.c
+++ b/trailer.c
@@ -58,7 +58,7 @@ static const char *git_generated_prefixes[] = {
 		pos != (head); \
 		pos = is_reverse ? pos->prev : pos->next)
 
-static int after_or_end(enum trailer_where where)
+static int scan_backwards(enum trailer_where where)
 {
 	return (where == WHERE_AFTER) || (where == WHERE_END);
 }
@@ -181,18 +181,8 @@ static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
 	return new;
 }
 
-static void add_arg_to_input_list(struct trailer_item *on_tok,
-				  struct arg_item *arg_tok)
-{
-	int aoe = after_or_end(arg_tok->conf.where);
-	struct trailer_item *to_add = trailer_from_arg(arg_tok);
-	if (aoe)
-		list_add(&to_add->list, &on_tok->list);
-	else
-		list_add_tail(&to_add->list, &on_tok->list);
-}
-
 static int check_if_different(struct trailer_item *in_tok,
+			      struct trailer_item *neighbor,
 			      struct arg_item *arg_tok,
 			      struct list_head *head)
 {
@@ -203,8 +193,8 @@ static int check_if_different(struct trailer_item *in_tok,
 		 * if we want to add a trailer after another one,
 		 * we have to check those before this one
 		 */
-		next_head = after_or_end(where) ? in_tok->list.prev
-						: in_tok->list.next;
+		next_head = scan_backwards(where) ? in_tok->list.prev
+						  : in_tok->list.next;
 		if (next_head == head)
 			return 1;
 		in_tok = list_entry(next_head, struct trailer_item, list);
@@ -212,6 +202,14 @@ static int check_if_different(struct trailer_item *in_tok,
 	return 0;
 }
 
+static int check_if_different_neighbor(struct trailer_item *in_tok,
+				       struct trailer_item *neighbor,
+				       struct arg_item *arg_tok,
+				       struct list_head *head)
+{
+	return !same_trailer(neighbor, arg_tok);
+}
+
 static char *apply_command(const char *command, const char *arg)
 {
 	struct strbuf cmd = STRBUF_INIT;
@@ -260,33 +258,80 @@ static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg
 	}
 }
 
+static int apply_arg(struct trailer_item *in_tok,
+		     struct arg_item *arg_tok,
+		     struct list_head *head,
+		     int (*check)(struct trailer_item *in_tok,
+				  struct trailer_item *neighbor,
+				  struct arg_item *arg_tok,
+				  struct list_head *head),
+		     int replace)
+{
+	struct trailer_item *to_add, *neighbor;
+	struct list_head *place;
+	int add_after;
+
+	enum trailer_where where = arg_tok->conf.where;
+	int middle = (where == WHERE_AFTER) || (where == WHERE_BEFORE);
+
+	/*
+	 * No other trailer to apply arg_tok one before/after.  Put it
+	 * before/after _all_ other trailers.
+	 */
+	if (!in_tok && middle) {
+		where = (where == WHERE_AFTER) ? WHERE_END : WHERE_START;
+		middle = 0;
+	}
+
+	if (list_empty(head)) {
+		add_after = 1;
+		place = head;
+		neighbor = NULL;
+	} else if (middle) {
+		add_after = (where == WHERE_AFTER);
+		place = &in_tok->list;
+		neighbor = in_tok;
+	} else {
+		add_after = (where == WHERE_END);
+		place = (where == WHERE_END) ? head->prev : head->next;
+		neighbor = list_entry(place, struct trailer_item, list);
+	}
+
+	apply_item_command(in_tok, arg_tok);
+	if (check && !check(in_tok, neighbor, arg_tok, head))
+		return 0;
+
+	to_add = trailer_from_arg(arg_tok);
+	if (add_after)
+		list_add(&to_add->list, place);
+	else
+		list_add_tail(&to_add->list, place);
+
+	if (replace) {
+		list_del(&in_tok->list);
+		free_trailer_item(in_tok);
+	}
+	return 1;
+}
+
 static void apply_arg_if_exists(struct trailer_item *in_tok,
 				struct arg_item *arg_tok,
-				struct trailer_item *on_tok,
 				struct list_head *head)
 {
 	switch (arg_tok->conf.if_exists) {
 	case EXISTS_DO_NOTHING:
 		break;
 	case EXISTS_REPLACE:
-		apply_item_command(in_tok, arg_tok);
-		add_arg_to_input_list(on_tok, arg_tok);
-		list_del(&in_tok->list);
-		free_trailer_item(in_tok);
+		apply_arg(in_tok, arg_tok, head, NULL, 1);
 		break;
 	case EXISTS_ADD:
-		apply_item_command(in_tok, arg_tok);
-		add_arg_to_input_list(on_tok, arg_tok);
+		apply_arg(in_tok, arg_tok, head, NULL, 0);
 		break;
 	case EXISTS_ADD_IF_DIFFERENT:
-		apply_item_command(in_tok, arg_tok);
-		if (check_if_different(in_tok, arg_tok, head))
-			add_arg_to_input_list(on_tok, arg_tok);
+		apply_arg(in_tok, arg_tok, head, check_if_different, 0);
 		break;
 	case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
-		apply_item_command(in_tok, arg_tok);
-		if (!same_trailer(on_tok, arg_tok))
-			add_arg_to_input_list(on_tok, arg_tok);
+		apply_arg(in_tok, arg_tok, head, check_if_different_neighbor, 0);
 		break;
 	default:
 		die("BUG: trailer.c: unhandled value %d",
@@ -297,24 +342,12 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
 static void apply_arg_if_missing(struct list_head *head,
 				 struct arg_item *arg_tok)
 {
-	enum trailer_where where;
-	struct trailer_item *to_add;
-
 	switch (arg_tok->conf.if_missing) {
 	case MISSING_DO_NOTHING:
 		break;
 	case MISSING_ADD:
-		where = arg_tok->conf.where;
-		apply_item_command(NULL, arg_tok);
-		to_add = trailer_from_arg(arg_tok);
-		if (after_or_end(where))
-			list_add_tail(&to_add->list, head);
-		else
-			list_add(&to_add->list, head);
+		apply_arg(NULL, arg_tok, head, NULL, 0);
 		break;
-	default:
-		die("BUG: trailer.c: unhandled value %d",
-		    arg_tok->conf.if_missing);
 	}
 }
 
@@ -323,26 +356,18 @@ static int find_same_and_apply_arg(struct list_head *head,
 {
 	struct list_head *pos;
 	struct trailer_item *in_tok;
-	struct trailer_item *on_tok;
 
 	enum trailer_where where = arg_tok->conf.where;
-	int middle = (where == WHERE_AFTER) || (where == WHERE_BEFORE);
-	int backwards = after_or_end(where);
-	struct trailer_item *start_tok;
+	int backwards = scan_backwards(where);
 
 	if (list_empty(head))
 		return 0;
 
-	start_tok = list_entry(backwards ? head->prev : head->next,
-			       struct trailer_item,
-			       list);
-
 	list_for_each_dir(pos, head, backwards) {
 		in_tok = list_entry(pos, struct trailer_item, list);
 		if (!same_token(in_tok, arg_tok))
 			continue;
-		on_tok = middle ? in_tok : start_tok;
-		apply_arg_if_exists(in_tok, arg_tok, on_tok, head);
+		apply_arg_if_exists(in_tok, arg_tok, head);
 		return 1;
 	}
 	return 0;
-- 
2.14.2

[PATCH 4/4] trailer: add "move" configuration for trailer.ifExists

[Paolo Bonzini]

In some cases, people apply patches to a queue branch immediately
with "git am -3 -s", and later collect Reviewed-by or Acked-by
trailers as they come in from the mailing list.

In this case, "where=after" does not have the desired behavior,
because it will add the trailer in an unorthodox position, after the
committer's Signed-off-by line.  The "move" configuration is intended
to be applied in such a case to the Signed-off-by header, like

    git interpret-trailers \
	--where end --if-missing doNothing --if-exists move
	"Signed-off-by: A U Thor <au@thor.example.org>"

or perhaps with an automated configuration

    [trailer "move-sob"]
    command = "'echo \"$(git config user.name) <$(git config user.email)>\"'"
    key = Signed-off-by
    where = end
    ifMissing = doNothing
    ifExists = move

Though this of course makes the most sense if the last Signed-off-by is
from the committer itself, and thus is not necessarily a good idea for
everyone.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Documentation/git-interpret-trailers.txt | 13 ++++++++---
 t/t7513-interpret-trailers.sh            | 37 ++++++++++++++++++++++++++++++++
 trailer.c                                | 26 +++++++++++++++++-----
 trailer.h                                |  1 +
 4 files changed, 69 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 9dd19a1dd..1cdde492c 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -165,7 +165,7 @@ trailer.ifexists::
 	same <token> in the message.
 +
 The valid values for this option are: `addIfDifferentNeighbor` (this
-is the default), `addIfDifferent`, `add`, `replace` or `doNothing`.
+is the default), `addIfDifferent`, `add`, `replace`, `move` or `doNothing`.
 +
 With `addIfDifferentNeighbor`, a new trailer will be added only if no
 trailer with the same (<token>, <value>) pair is above or below the line
@@ -182,13 +182,20 @@ deleted and the new trailer will be added. The deleted trailer will be
 the closest one (with the same <token>) to the place where the new one
 will be added.
 +
+With `move`, an existing trailer with the same (<token>, <value>) will be
+deleted and the new trailer will be added.  If more equal pairs exists,
+the deleted trailer will be the closest one to the place where the new one
+will be added.
++
 With `doNothing`, nothing will be done; that is no new trailer will be
 added if there is already one with the same <token> in the message.
 
 trailer.ifmissing::
 	This option makes it possible to choose what action will be
-	performed when there is not yet any trailer with the same
-	<token> in the message.
+	performed when the `ifexists` action fails.  This means that
+	there is not yet any trailer with the same <token> in the message
+	or, for the `move` action only, there is no identical trailer
+	in the message.
 +
 The valid values for this option are: `add` (this is the default) and
 `doNothing`.
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 164719d1c..a0f21fefd 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1052,6 +1052,43 @@ test_expect_success 'using "ifExists = doNothing"' '
 	test_cmp expected actual
 '
 
+test_expect_success 'require token and value match when "ifExists = move"' '
+	git config trailer.fix.ifExists "move" &&
+	git config trailer.fix.where "start" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: 22
+		Fixes: Z
+		Acked-by= Z
+		Reviewed-by:
+		Signed-off-by: Z
+		Fixes: 53
+	EOF
+	(cat complex_message; echo Fixes: 53) | \
+	git interpret-trailers \
+		--trailer "fix=22" \
+		>actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'look for all trailers when "ifExists = move"' '
+	git config trailer.fix.ifMissing "doNothing" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: 22
+		Fixes: Z
+		Acked-by= Z
+		Reviewed-by:
+		Signed-off-by: Z
+		Fixes: 53
+	EOF
+	(cat complex_message; echo Fixes: 22; echo Fixes: 53) | \
+	git interpret-trailers \
+		--trailer "fix=22" \
+		>actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'the default is "ifMissing = add"' '
 	git config trailer.cc.key "Cc: " &&
 	git config trailer.cc.where "before" &&
diff --git a/trailer.c b/trailer.c
index ce0d94074..4915bda8f 100644
--- a/trailer.c
+++ b/trailer.c
@@ -202,6 +202,14 @@ static int check_if_different(struct trailer_item *in_tok,
 	return 0;
 }
 
+static int check_if_same(struct trailer_item *in_tok,
+			 struct trailer_item *neighbor,
+			 struct arg_item *arg_tok,
+			 struct list_head *head)
+{
+	return same_trailer(in_tok, arg_tok);
+}
+
 static int check_if_different_neighbor(struct trailer_item *in_tok,
 				       struct trailer_item *neighbor,
 				       struct arg_item *arg_tok,
@@ -255,6 +263,8 @@ static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg
 		}
 		arg_tok->value = apply_command(arg_tok->conf.command, arg);
 		free((char *)arg);
+		free(arg_tok->conf.command);
+		arg_tok->conf.command = NULL;
 	}
 }
 
@@ -314,9 +324,9 @@ static int apply_arg(struct trailer_item *in_tok,
 	return 1;
 }
 
-static void apply_arg_if_exists(struct trailer_item *in_tok,
-				struct arg_item *arg_tok,
-				struct list_head *head)
+static int apply_arg_if_exists(struct trailer_item *in_tok,
+			       struct arg_item *arg_tok,
+			       struct list_head *head)
 {
 	switch (arg_tok->conf.if_exists) {
 	case EXISTS_DO_NOTHING:
@@ -324,6 +334,9 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
 	case EXISTS_REPLACE:
 		apply_arg(in_tok, arg_tok, head, NULL, 1);
 		break;
+	case EXISTS_MOVE:
+		/* Look for another trailer if the match fails.  */
+		return apply_arg(in_tok, arg_tok, head, check_if_same, 1);
 	case EXISTS_ADD:
 		apply_arg(in_tok, arg_tok, head, NULL, 0);
 		break;
@@ -337,6 +350,7 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
 		die("BUG: trailer.c: unhandled value %d",
 		    arg_tok->conf.if_exists);
 	}
+	return 1;
 }
 
 static void apply_arg_if_missing(struct list_head *head,
@@ -367,8 +381,8 @@ static int find_same_and_apply_arg(struct list_head *head,
 		in_tok = list_entry(pos, struct trailer_item, list);
 		if (!same_token(in_tok, arg_tok))
 			continue;
-		apply_arg_if_exists(in_tok, arg_tok, head);
-		return 1;
+		if (apply_arg_if_exists(in_tok, arg_tok, head))
+			return 1;
 	}
 	return 0;
 }
@@ -423,6 +437,8 @@ int trailer_set_if_exists(enum trailer_if_exists *item, const char *value)
 		*item = EXISTS_ADD;
 	else if (!strcasecmp("replace", value))
 		*item = EXISTS_REPLACE;
+	else if (!strcasecmp("move", value))
+		*item = EXISTS_MOVE;
 	else if (!strcasecmp("doNothing", value))
 		*item = EXISTS_DO_NOTHING;
 	else
diff --git a/trailer.h b/trailer.h
index 6d7f8c2a5..0fbdab38d 100644
--- a/trailer.h
+++ b/trailer.h
@@ -16,6 +16,7 @@ enum trailer_if_exists {
 	EXISTS_ADD_IF_DIFFERENT,
 	EXISTS_ADD,
 	EXISTS_REPLACE,
+	EXISTS_MOVE,
 	EXISTS_DO_NOTHING
 };
 enum trailer_if_missing {
-- 
2.14.2

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Junio C Hamano]

Paolo Bonzini <pbonzini@redhat.com> writes:

> The purpose of this action is for scripts to be able to keep the
> user's Signed-off-by at the end.  For example say I have a script
> that adds a Reviewed-by tag:
>
>     #! /bin/sh
>     them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
>     trailer="Reviewed-by: $them"
>     git log -1 --pretty=format:%B | \
>       git interpret-trailers --where end --if-exists doNothing --trailer "$trailer" | \
>       git commit --amend -F-
>
> Now, this script will leave my Signed-off-by line in a non-canonical
> place, like
>
>    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>    Reviewed-by: Junio C Hamano <gitster@pobox.com>
>
> This new option enables the following improvement:
>
>     #! /bin/sh
>     me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')
>     them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
>     trailer="Reviewed-by: $them"
>     sob="Signed-off-by: $me"
>     git log -1 --pretty=format:%B | \
>       git interpret-trailers --where end --if-exists doNothing --trailer "$trailer" \
>                              --where end --if-exists move --if-missing doNothing --trailer "$sob" | \
>       git commit --amend -F-
>
> which lets me keep the SoB line at the end, as it should be.
> Posting as RFC because it's possible that I'm missing a simpler
> way to achieve this...

While I think "move" may turn out to be handy in some use case, an
example to move S-o-b does not sound convincing to me at all.  

If anything, the above (assuming that you wrote a patch, sent out
for a review with or without signing it off, and then after getting
a review, you are adding reviewed-by to the commit) does not
demonstrate the need for "move".  The use of "move" in the example
looks like a mere workaround that reviewed-by was added at the wrong
place (i.e. --where end) in the first place.

But that is not the primary reason why I find the example using
S-o-b convincing.  If the patch in your example originally did not
have just one S-o-b by you, but yours was at the end of the chain of
patch passing, use of "move" may become even more problematic.  Your
friend may write an original, sign it off and pass it to you, who
then signs it off and sends to the mailng list.  It gets picked up
by somebody else, who tweaks and adds her sign off, then you pick it
up and relay it to the final destination (i.e. the first sign-off is
by your friend, then you have two sign-offs of yours, one sign off
from somebody else in between, and the chain records how the patch
"flowed").  And then Linus says "yeah, this is good, I throughly
reviewed it."  Where would you place that reviewed-by?  Before your
second (and last) sign-off?  What makes that last one special?
Would it more faithfully reflect the order of events if you added
Linus's reviewed-by and then your own sign-off to conclude the
chain?

So I am not opposed to the idea of "move", but I am not sure in what
situation it is useful and what use case it makes it easier to use.
The example makes me suspect that what we want is not a new
operation, but a way to specify "where" in a richer way.

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Paolo Bonzini]

On 06/10/2017 08:44, Junio C Hamano wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>> The purpose of this action is for scripts to be able to keep the
>> user's Signed-off-by at the end.
>>
>>     #! /bin/sh
>>     me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')
>>     them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
>>     trailer="Reviewed-by: $them"
>>     sob="Signed-off-by: $me"
>>     git log -1 --pretty=format:%B | \
>>       git interpret-trailers --where end --if-exists doNothing --trailer "$trailer" \
>>                              --where end --if-exists move --if-missing doNothing --trailer "$sob" | \
>>       git commit --amend -F-
>>
>> which lets me keep the SoB line at the end, as it should be.
>> Posting as RFC because it's possible that I'm missing a simpler
>> way to achieve this...
>
> If anything, the above (assuming that you wrote a patch, sent out
> for a review with or without signing it off, and then after getting
> a review, you are adding reviewed-by to the commit) does not
> demonstrate the need for "move".  The use of "move" in the example
> looks like a mere workaround that reviewed-by was added at the wrong
> place (i.e. --where end) in the first place.

Yes, I agree.  Though I also tried implementing "--where beforeLastSOB", 
and in the end decided against it because there were more corner cases.
I include the patch for reference at the end of this message, to be
applied on top of these four.

> But that is not the primary reason why I find the example using
> S-o-b convincing.  If the patch in your example originally did not
> have just one S-o-b by you, but yours was at the end of the chain of
> patch passing, use of "move" may become even more problematic.  Your
> friend may write an original, sign it off and pass it to you, who
> then signs it off and sends to the mailng list.  It gets picked up
> by somebody else, who tweaks and adds her sign off, then you pick it
> up and relay it to the final destination (i.e. the first sign-off is
> by your friend, then you have two sign-offs of yours, one sign off
> from somebody else in between, and the chain records how the patch
> "flowed").  And then Linus says "yeah, this is good, I throughly
> reviewed it."  Where would you place that reviewed-by?  Before your
> second (and last) sign-off?  What makes that last one special?

So:

   Signed-off-by: Me
   Signed-off-by: Friend
   Signed-off-by: Me       <<<
   Reviewed-by: Linus
   Signed-off-by: Me

I do think the SoB line marked with "<<<" is a bit "special".  SoB lines 
before it represent the path followed by the contribution, according to 
clause (c) of the Developer Certificate of Origin.  Multiple 
*consecutive* SoB lines from the same person do not add much, while 
multiple separate SoB lines from the same person must be kept.

Of course, using "--where start" for Reviewed/Tested/Acked-by lines *is* 
an option.  On the other hand, for "Cc: stable@vger.kernel.org" the 
placement hints at *who* decided the patch to be worth of inclusion in a 
stable version.  That person might be the right one to bug if the patch 
doesn't apply and needs a manual backport.  It's not science of course, 
but in practice I found the "always apply with -s, and use 'move' to 
keep my SoB at the end" workflow to be the least error-prone.

> Would it more faithfully reflect the order of events if you added
> Linus's reviewed-by and then your own sign-off to conclude the
> chain?

Possibly, but the DCO doesn't care and SoB lines are first and firemost 
about the DCO.

On the other hand, "move" does not provide exactly what we want in the 
case where the user's SoB is there, but is not the last.  So, the above 
script pretty much assumes that you apply the patch with "-s"; if you 
didn't, you'd need something more like "moveLast".  It is trivial to 
implement "moveLast" on top of the first three patches in this series, 
but things start getting a bit out of hand perhaps...

Paolo

> So I am not opposed to the idea of "move", but I am not sure in what
> situation it is useful and what use case it makes it easier to use.
> The example makes me suspect that what we want is not a new
> operation, but a way to specify "where" in a richer way.

----------- 8< ------------
From a238b973821586eaaf26d608172cdc72f19d6071 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Wed, 12 Jul 2017 18:11:44 +0200
Subject: [PATCH] trailer: add "beforeLastSOB" configuration for trailer.where

In some cases, people apply patches to a queue branch immediately
with "git am -3 -s", and later collect Reviewed-by or Acked-by
trailers as they come in from the mailing list.

In this case, "where=after" does not have the desired behavior,
because it will add the trailer in an unorthodox position, after the
committer's Signed-off-by line.  Introduce a "beforeLastSOB" value
for trailer.where; this of course makes the most sense if the
last Signed-off-by is from the committer itself.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Documentation/git-interpret-trailers.txt |  4 ++
 t/t7513-interpret-trailers.sh            | 86 +++++++++++++++++++++++++++++++-
 trailer.c                                | 41 ++++++++++++++-
 trailer.h                                |  3 +-
 4 files changed, 130 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 1cdde492c..e012f11c1 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -158,6 +158,10 @@ last trailer with the same <token>.
 +
 If it is `before`, then each new trailer will appear just before the
 first trailer with the same <token>.
++
+If it is `beforeLastSOB`, then each new trailer will appear just
+before the last Signed-off-by line.  If there is no such line, the
+trailer will appear at the end of the existing trailers.
 
 trailer.ifexists::
 	This option makes it possible to choose what action will be
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index a0f21fefd..06d6226cd 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -727,6 +727,25 @@ test_expect_success 'using "where = after"' '
 	test_cmp expected actual
 '
 
+test_expect_success 'using "where = beforeLastSOB"' '
+	git config trailer.review.key "Reviewed-by" &&
+	git config trailer.review.where "beforeLastSOB" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by= Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Reviewed-by: Johannes
+		Signed-off-by: Junio
+	EOF
+	git interpret-trailers --trailer "Signed-off-by: Junio" \
+		--trailer "Reviewed-by: Johannes" \
+		complex_message >actual &&
+	git config trailer.ack.where "after" &&
+	test_cmp expected actual
+'
+
 test_expect_success 'using "where = end"' '
 	git config trailer.review.key "Reviewed-by" &&
 	git config trailer.review.where "end" &&
@@ -833,6 +852,26 @@ test_expect_success 'default "ifExists" is now "addIfDifferent"' '
 	test_cmp expected actual
 '
 
+test_expect_success 'using "ifExists = addIfDifferent" with "where = beforeLastSOB"' '
+	git config trailer.ack.ifExists "addIfDifferent" &&
+	git config trailer.ack.where "beforeLastSOB" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by= Z
+		Reviewed-by:
+		Signed-off-by: Z
+		Acked-by= Johannes
+		Acked-by= Peff
+		Signed-off-by: Junio
+	EOF
+	git interpret-trailers --trailer "Signed-off-by: Junio" \
+		--trailer "ack: Johannes" --trailer "ack: Johannes" \
+		--trailer "ack: Peff" --trailer "ack: Johannes" \
+		complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'using "ifExists = addIfDifferent" with "where = end"' '
 	git config trailer.ack.ifExists "addIfDifferent" &&
 	git config trailer.ack.where "end" &&
@@ -892,6 +931,27 @@ test_expect_success 'using "ifExists = addIfDifferentNeighbor" with "where = end
 	test_cmp expected actual
 '
 
+test_expect_success 'using "ifExists = addIfDifferentNeighbor" with "where = beforeLastSOB"' '
+	git config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	git config trailer.ack.where "beforeLastSOB" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by= Z
+		Reviewed-by:
+		Signed-off-by: Z
+		Acked-by= Johannes
+		Acked-by= Peff
+		Acked-by= Johannes
+		Signed-off-by: Junio
+	EOF
+	git interpret-trailers --trailer "Signed-off-by: Junio" \
+		--trailer "ack: Johannes" --trailer "ack: Johannes" \
+		--trailer "ack: Peff" --trailer "ack: Johannes" \
+		complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'using "ifExists = addIfDifferentNeighbor"  with "where = after"' '
 	git config trailer.ack.ifExists "addIfDifferentNeighbor" &&
 	git config trailer.ack.where "after" &&
@@ -1211,6 +1271,30 @@ test_expect_success 'using "ifMissing = doNothing"' '
 	test_cmp expected actual
 '
 
+test_expect_success 'using "where = beforeLastSOB" to add Signed-off-by' '
+	git config trailer.sign.key "Signed-off-by: " &&
+	git config trailer.sign.ifExists "addIfDifferentNeighbor" &&
+	git config trailer.sign.where "beforeLastSOB" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by= Z
+		Reviewed-by:
+		Signed-off-by: Junio
+		Signed-off-by: Johannes
+		Signed-off-by: Another
+		Signed-off-by: Junio
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers --trailer "Signed-off-by: Junio" |
+	git interpret-trailers --trailer "Signed-off-by: Junio" \
+		--trailer "Signed-off-by: Johannes" \
+		--trailer "Signed-off-by: Another" \
+		--trailer "Signed-off-by: Junio" \
+		complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'default "where" is now "after"' '
 	git config trailer.where "after" &&
 	git config --unset trailer.ack.where &&
@@ -1237,9 +1321,7 @@ test_expect_success 'default "where" is now "after"' '
 '
 
 test_expect_success 'with simple command' '
-	git config trailer.sign.key "Signed-off-by: " &&
 	git config trailer.sign.where "after" &&
-	git config trailer.sign.ifExists "addIfDifferentNeighbor" &&
 	git config trailer.sign.command "echo \"A U Thor <author@example.com>\"" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
diff --git a/trailer.c b/trailer.c
index 4915bda8f..c1ab69e58 100644
--- a/trailer.c
+++ b/trailer.c
@@ -60,7 +60,12 @@ static const char *git_generated_prefixes[] = {
 
 static int scan_backwards(enum trailer_where where)
 {
-	return (where == WHERE_AFTER) || (where == WHERE_END);
+	/*
+	 * beforeLastSOB is almost like "end", only the placement of the new
+	 * trailer varies.
+	 */
+	return (where == WHERE_AFTER) || (where == WHERE_END) ||
+		(where == WHERE_BEFORE_LAST_SOB);
 }
 
 /*
@@ -100,6 +105,24 @@ static int same_trailer(struct trailer_item *a, struct arg_item *b)
 	return same_token(a, b) && same_value(a, b);
 }
 
+static int is_sob(struct trailer_item *in_tok)
+{
+	return !strncasecmp(in_tok->token, "Signed-off-by", 13);
+}
+
+static struct list_head *find_last_sob(struct list_head *head)
+{
+	struct list_head *pos;
+	struct trailer_item *in_tok;
+
+	list_for_each_prev(pos, head) {
+		in_tok = list_entry(pos, struct trailer_item, list);
+		if (is_sob(in_tok))
+			return pos;
+	}
+	return head;
+}
+
 static inline int is_blank_line(const char *str)
 {
 	const char *s = str;
@@ -297,6 +320,20 @@ static int apply_arg(struct trailer_item *in_tok,
 		add_after = 1;
 		place = head;
 		neighbor = NULL;
+	} else if (where == WHERE_BEFORE_LAST_SOB) {
+		add_after = 0;
+		place = find_last_sob(head);
+
+		/*
+		 * Trying to compare two trailers of the same kind provides the
+		 * most sensible results for addIfDifferentNeighbor (see testsuite).
+		 * So when *not* adding a SOB, beforeLastSOB compares to the element
+		 * before.
+		 */
+		if (strcasecmp(arg_tok->token, "Signed-off-by"))
+			neighbor = list_entry(place->prev, struct trailer_item, list);
+		else
+			neighbor = in_tok;
 	} else if (middle) {
 		add_after = (where == WHERE_AFTER);
 		place = &in_tok->list;
@@ -420,6 +457,8 @@ int trailer_set_where(enum trailer_where *item, const char *value)
 		*item = WHERE_END;
 	else if (!strcasecmp("start", value))
 		*item = WHERE_START;
+	else if (!strcasecmp("beforeLastSOB", value))
+		*item = WHERE_BEFORE_LAST_SOB;
 	else
 		return -1;
 	return 0;
diff --git a/trailer.h b/trailer.h
index 0fbdab38d..e30f59e47 100644
--- a/trailer.h
+++ b/trailer.h
@@ -8,7 +8,8 @@ enum trailer_where {
 	WHERE_END,
 	WHERE_AFTER,
 	WHERE_BEFORE,
-	WHERE_START
+	WHERE_START,
+	WHERE_BEFORE_LAST_SOB
 };
 enum trailer_if_exists {
 	EXISTS_DEFAULT,
-- 
2.14.2

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Christian Couder]

On Thu, Oct 5, 2017 at 3:22 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The purpose of this action is for scripts to be able to keep the
> user's Signed-off-by at the end.  For example say I have a script
> that adds a Reviewed-by tag:
>
>     #! /bin/sh
>     them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
>     trailer="Reviewed-by: $them"
>     git log -1 --pretty=format:%B | \
>       git interpret-trailers --where end --if-exists doNothing --trailer "$trailer" | \
>       git commit --amend -F-
>
> Now, this script will leave my Signed-off-by line in a non-canonical
> place, like
>
>    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>    Reviewed-by: Junio C Hamano <gitster@pobox.com>
>
> This new option enables the following improvement:
>
>     #! /bin/sh
>     me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')
>     them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
>     trailer="Reviewed-by: $them"
>     sob="Signed-off-by: $me"
>     git log -1 --pretty=format:%B | \
>       git interpret-trailers --where end --if-exists doNothing --trailer "$trailer" \
>                              --where end --if-exists move --if-missing doNothing --trailer "$sob" | \
>       git commit --amend -F-
>
> which lets me keep the SoB line at the end, as it should be.
> Posting as RFC because it's possible that I'm missing a simpler
> way to achieve this...

Did you try using `--where end --if-exists replace --trailer "$sob"`?

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Paolo Bonzini]

On 06/10/2017 12:30, Christian Couder wrote:
> On Thu, Oct 5, 2017 at 3:22 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> The purpose of this action is for scripts to be able to keep the
>> user's Signed-off-by at the end.  For example say I have a script
>> that adds a Reviewed-by tag:
>>
>>     #! /bin/sh
>>     them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
>>     trailer="Reviewed-by: $them"
>>     git log -1 --pretty=format:%B | \
>>       git interpret-trailers --where end --if-exists doNothing --trailer "$trailer" | \
>>       git commit --amend -F-
>>
>> Now, this script will leave my Signed-off-by line in a non-canonical
>> place, like
>>
>>    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>    Reviewed-by: Junio C Hamano <gitster@pobox.com>
>>
>> This new option enables the following improvement:
>>
>>     #! /bin/sh
>>     me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')
>>     them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
>>     trailer="Reviewed-by: $them"
>>     sob="Signed-off-by: $me"
>>     git log -1 --pretty=format:%B | \
>>       git interpret-trailers --where end --if-exists doNothing --trailer "$trailer" \
>>                              --where end --if-exists move --if-missing doNothing --trailer "$sob" | \
>>       git commit --amend -F-
>>
>> which lets me keep the SoB line at the end, as it should be.
>> Posting as RFC because it's possible that I'm missing a simpler
>> way to achieve this...
> 
> Did you try using `--where end --if-exists replace --trailer "$sob"`?

Yes, it's a different behavior; "--if-exists replace" matches on others'
SoB as well, so it would eat the original author's SoB if I didn't have one.

So "move" does get it wrong for

    Signed-off-by: Me
    Signed-off-by: Friend

(Me gets moved last, which may not be what you want) but "replace" gets
it wrong in the arguably more common case of

    Signed-off-by: Friend

which is damaged to just "Signed-off-by: Me".

Paolo

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Christian Couder]

On Fri, Oct 6, 2017 at 12:40 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 06/10/2017 12:30, Christian Couder wrote:

>> Did you try using `--where end --if-exists replace --trailer "$sob"`?
>
> Yes, it's a different behavior; "--if-exists replace" matches on others'
> SoB as well, so it would eat the original author's SoB if I didn't have one.
>
> So "move" does get it wrong for
>
>     Signed-off-by: Me
>     Signed-off-by: Friend
>
> (Me gets moved last, which may not be what you want) but "replace" gets
> it wrong in the arguably more common case of
>
>     Signed-off-by: Friend
>
> which is damaged to just "Signed-off-by: Me".

Ok. I think you might want something called for example
"replaceIfIdenticalClose" where "IdenticalClose" means: "there is a
trailer with the same (<token>, <value>) pair above or below the line
where the replaced trailer will be put when ignoring trailers with a
different <token>".

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Paolo Bonzini]

On 06/10/2017 14:33, Christian Couder wrote:
> Ok. I think you might want something called for example
> "replaceIfIdenticalClose" where "IdenticalClose" means: "there is a
> trailer with the same (<token>, <value>) pair above or below the line
> where the replaced trailer will be put when ignoring trailers with a
> different <token>".

So basically "moveIfClosest" (move if last for where=end, move if first
for where=begin; for where=after and where=before it would just end up
doing nothing)?  It's not hard to implement, but I'm wondering if it's
too ad hoc.

Paolo

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Christian Couder]

On Fri, Oct 6, 2017 at 2:39 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 06/10/2017 14:33, Christian Couder wrote:
>> Ok. I think you might want something called for example
>> "replaceIfIdenticalClose" where "IdenticalClose" means: "there is a
>> trailer with the same (<token>, <value>) pair above or below the line
>> where the replaced trailer will be put when ignoring trailers with a
>> different <token>".
>
> So basically "moveIfClosest" (move if last for where=end, move if first
> for where=begin; for where=after and where=before it would just end up
> doing nothing)?

First yeah these would not make sense anyway if where=after or where=before.

Now it would be strange to have "moveIfClosest" without having "move"
first and I don't see how "move" would be different from the existing
"replace".
Or maybe "move" means "replaceIfIdentical", in this case I think it
would help users to just call it "replaceIfIdentical".

Also there is "addIfDifferentNeighbor" so we already have "Neighbor"
which means "just above or below". Then if we use "Closest" I think it
will be harder to distinguish it from "Neighbor" than if we use
"Close".

That's why I think "replaceIfIdenticalClose" is better. It could
enable us to eventually use a regexp like
"(add|replace)(If(Different|Identical)(Close|Neighbor)+)+"  to parse
the add* and replace* options.

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Paolo Bonzini]

On 06/10/2017 15:19, Christian Couder wrote:
> Now it would be strange to have "moveIfClosest" without having "move"
> first and I don't see how "move" would be different from the existing
> "replace".
> Or maybe "move" means "replaceIfIdentical", in this case I think it
> would help users to just call it "replaceIfIdentical".

Well, the effect of "replacing if identical" *is* to move the existing
identical trailer to the new position. :)

Paolo

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Junio C Hamano]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 06/10/2017 14:33, Christian Couder wrote:
>> Ok. I think you might want something called for example
>> "replaceIfIdenticalClose" where "IdenticalClose" means: "there is a
>> trailer with the same (<token>, <value>) pair above or below the line
>> where the replaced trailer will be put when ignoring trailers with a
>> different <token>".
>
> So basically "moveIfClosest" (move if last for where=end, move if first
> for where=begin; for where=after and where=before it would just end up
> doing nothing)?  It's not hard to implement, but I'm wondering if it's
> too ad hoc.

Yeah, it makes me wonder exactly that, too.