{"thread":{"id":"46911","subject":"[RFC PATCH 0/4] interpret-trailers: introduce \"move\" action","startedAt":"2017-10-05T13:22:53Z","lastAt":"2017-10-07T00:51:10Z","messageCount":14,"participants":["Paolo Bonzini","Junio C Hamano","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"329765","messageId":"20171005132243.27058-1-pbonzini@redhat.com","threadId":"46911","inReplyTo":null,"subject":"[RFC PATCH 0/4] interpret-trailers: introduce \"move\" action","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2017-10-05T13:22:39Z","receivedAt":"2017-10-05T13:22:53Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"The purpose of this action is for scripts to be able to keep the\nuser's Signed-off-by at the end. For example say I have a script\nthat adds a Reviewed-by tag:\n\n #! /bin/sh\n them=$(git log -i -1 --pretty='format:%an <%ae>' --author=\"$*\")\n trailer=\"Reviewed-by: $them\"\n git log -1 --pretty=format:%B | \\\n git interpret-trailers --where end --if-exists doNothing --trailer \"$trailer\" | \\\n git commit --amend -F-\n\nNow, this script will leave my Signed-off-by line in a non-canonical\nplace, like\n\n Signed-off-by: Paolo Bonzini \n Reviewed-by: Junio C Hamano \n\nThis new option enables the following improvement:\n\n #! /bin/sh\n me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')\n them=$(git log -i -1 --pretty='format:%an <%ae>' --author=\"$*\")\n trailer=\"Reviewed-by: $them\"\n sob=\"Signed-off-by: $me\"\n git log -1 --pretty=format:%B | \\\n git interpret-trailers --where end --if-exists doNothing --trailer \"$trailer\" \\\n --where end --if-exists move --if-missing doNothing --trailer \"$sob\" | \\\n git commit --amend -F-\n\nwhich lets me keep the SoB line at the end, as it should be.\nPosting as RFC because it's possible that I'm missing a simpler\nway to achieve this...\n\nPaolo Bonzini (4):\n trailer: push free_arg_item up\n trailer: simplify check_if_different\n trailer: create a new function to handle adding trailers\n trailer: add \"move\" configuration for trailer.ifExists\n\n Documentation/git-interpret-trailers.txt | 13 ++-\n t/t7513-interpret-trailers.sh | 37 +++++++\n trailer.c | 169 ++++++++++++++++++-------------\n trailer.h | 1 +\n 4 files changed, 149 insertions(+), 71 deletions(-)\n\n-- \n2.14.2\n\n"},{"id":"329766","messageId":"20171005132243.27058-4-pbonzini@redhat.com","threadId":"46911","inReplyTo":"20171005132243.27058-1-pbonzini@redhat.com","subject":"[PATCH 3/4] trailer: create a new function to handle adding trailers","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2017-10-05T13:22:42Z","receivedAt":"2017-10-05T13:22:59Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"Create a new function apply_arg that takes care of computing the new\ntrailer's \"neighbor\", checking for duplicates through a pluggable\ncallback, and adding the new argument according to \"trailer.where\".\n\nRename after_or_end, and don't use it in apply_arg. It's a coincidence\nthat the conditions for \"scan backwards\" and \"add after\" are the same.\n\nThis simplifies find_same_and_apply_arg so that it does exactly what\nthe name says. apply_arg_if_missing can also use the new function;\nbefore, it was redoing add_arg_to_input_list's job in a slightly\ndifferent fashion.\n\nSigned-off-by: Paolo Bonzini \n---\n trailer.c | 125 +++++++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 75 insertions(+), 50 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 91f89db7f..ce0d94074 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -58,7 +58,7 @@ static const char *git_generated_prefixes[] = {\n \t\tpos != (head); \\\n \t\tpos = is_reverse ? pos->prev : pos->next)\n \n-static int after_or_end(enum trailer_where where)\n+static int scan_backwards(enum trailer_where where)\n {\n \treturn (where == WHERE_AFTER) || (where == WHERE_END);\n }\n@@ -181,18 +181,8 @@ static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)\n \treturn new;\n }\n \n-static void add_arg_to_input_list(struct trailer_item *on_tok,\n-\t\t\t\t struct arg_item *arg_tok)\n-{\n-\tint aoe = after_or_end(arg_tok->conf.where);\n-\tstruct trailer_item *to_add = trailer_from_arg(arg_tok);\n-\tif (aoe)\n-\t\tlist_add(&to_add->list, &on_tok->list);\n-\telse\n-\t\tlist_add_tail(&to_add->list, &on_tok->list);\n-}\n-\n static int check_if_different(struct trailer_item *in_tok,\n+\t\t\t struct trailer_item *neighbor,\n \t\t\t struct arg_item *arg_tok,\n \t\t\t struct list_head *head)\n {\n@@ -203,8 +193,8 @@ static int check_if_different(struct trailer_item *in_tok,\n \t\t * if we want to add a trailer after another one,\n \t\t * we have to check those before this one\n \t\t */\n-\t\tnext_head = after_or_end(where) ? in_tok->list.prev\n-\t\t\t\t\t\t: in_tok->list.next;\n+\t\tnext_head = scan_backwards(where) ? in_tok->list.prev\n+\t\t\t\t\t\t : in_tok->list.next;\n \t\tif (next_head == head)\n \t\t\treturn 1;\n \t\tin_tok = list_entry(next_head, struct trailer_item, list);\n@@ -212,6 +202,14 @@ static int check_if_different(struct trailer_item *in_tok,\n \treturn 0;\n }\n \n+static int check_if_different_neighbor(struct trailer_item *in_tok,\n+\t\t\t\t struct trailer_item *neighbor,\n+\t\t\t\t struct arg_item *arg_tok,\n+\t\t\t\t struct list_head *head)\n+{\n+\treturn !same_trailer(neighbor, arg_tok);\n+}\n+\n static char *apply_command(const char *command, const char *arg)\n {\n \tstruct strbuf cmd = STRBUF_INIT;\n@@ -260,33 +258,80 @@ static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg\n \t}\n }\n \n+static int apply_arg(struct trailer_item *in_tok,\n+\t\t struct arg_item *arg_tok,\n+\t\t struct list_head *head,\n+\t\t int (*check)(struct trailer_item *in_tok,\n+\t\t\t\t struct trailer_item *neighbor,\n+\t\t\t\t struct arg_item *arg_tok,\n+\t\t\t\t struct list_head *head),\n+\t\t int replace)\n+{\n+\tstruct trailer_item *to_add, *neighbor;\n+\tstruct list_head *place;\n+\tint add_after;\n+\n+\tenum trailer_where where = arg_tok->conf.where;\n+\tint middle = (where == WHERE_AFTER) || (where == WHERE_BEFORE);\n+\n+\t/*\n+\t * No other trailer to apply arg_tok one before/after. Put it\n+\t * before/after _all_ other trailers.\n+\t */\n+\tif (!in_tok && middle) {\n+\t\twhere = (where == WHERE_AFTER) ? WHERE_END : WHERE_START;\n+\t\tmiddle = 0;\n+\t}\n+\n+\tif (list_empty(head)) {\n+\t\tadd_after = 1;\n+\t\tplace = head;\n+\t\tneighbor = NULL;\n+\t} else if (middle) {\n+\t\tadd_after = (where == WHERE_AFTER);\n+\t\tplace = &in_tok->list;\n+\t\tneighbor = in_tok;\n+\t} else {\n+\t\tadd_after = (where == WHERE_END);\n+\t\tplace = (where == WHERE_END) ? head->prev : head->next;\n+\t\tneighbor = list_entry(place, struct trailer_item, list);\n+\t}\n+\n+\tapply_item_command(in_tok, arg_tok);\n+\tif (check && !check(in_tok, neighbor, arg_tok, head))\n+\t\treturn 0;\n+\n+\tto_add = trailer_from_arg(arg_tok);\n+\tif (add_after)\n+\t\tlist_add(&to_add->list, place);\n+\telse\n+\t\tlist_add_tail(&to_add->list, place);\n+\n+\tif (replace) {\n+\t\tlist_del(&in_tok->list);\n+\t\tfree_trailer_item(in_tok);\n+\t}\n+\treturn 1;\n+}\n+\n static void apply_arg_if_exists(struct trailer_item *in_tok,\n \t\t\t\tstruct arg_item *arg_tok,\n-\t\t\t\tstruct trailer_item *on_tok,\n \t\t\t\tstruct list_head *head)\n {\n \tswitch (arg_tok->conf.if_exists) {\n \tcase EXISTS_DO_NOTHING:\n \t\tbreak;\n \tcase EXISTS_REPLACE:\n-\t\tapply_item_command(in_tok, arg_tok);\n-\t\tadd_arg_to_input_list(on_tok, arg_tok);\n-\t\tlist_del(&in_tok->list);\n-\t\tfree_trailer_item(in_tok);\n+\t\tapply_arg(in_tok, arg_tok, head, NULL, 1);\n \t\tbreak;\n \tcase EXISTS_ADD:\n-\t\tapply_item_command(in_tok, arg_tok);\n-\t\tadd_arg_to_input_list(on_tok, arg_tok);\n+\t\tapply_arg(in_tok, arg_tok, head, NULL, 0);\n \t\tbreak;\n \tcase EXISTS_ADD_IF_DIFFERENT:\n-\t\tapply_item_command(in_tok, arg_tok);\n-\t\tif (check_if_different(in_tok, arg_tok, head))\n-\t\t\tadd_arg_to_input_list(on_tok, arg_tok);\n+\t\tapply_arg(in_tok, arg_tok, head, check_if_different, 0);\n \t\tbreak;\n \tcase EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:\n-\t\tapply_item_command(in_tok, arg_tok);\n-\t\tif (!same_trailer(on_tok, arg_tok))\n-\t\t\tadd_arg_to_input_list(on_tok, arg_tok);\n+\t\tapply_arg(in_tok, arg_tok, head, check_if_different_neighbor, 0);\n \t\tbreak;\n \tdefault:\n \t\tdie(\"BUG: trailer.c: unhandled value %d\",\n@@ -297,24 +342,12 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,\n static void apply_arg_if_missing(struct list_head *head,\n \t\t\t\t struct arg_item *arg_tok)\n {\n-\tenum trailer_where where;\n-\tstruct trailer_item *to_add;\n-\n \tswitch (arg_tok->conf.if_missing) {\n \tcase MISSING_DO_NOTHING:\n \t\tbreak;\n \tcase MISSING_ADD:\n-\t\twhere = arg_tok->conf.where;\n-\t\tapply_item_command(NULL, arg_tok);\n-\t\tto_add = trailer_from_arg(arg_tok);\n-\t\tif (after_or_end(where))\n-\t\t\tlist_add_tail(&to_add->list, head);\n-\t\telse\n-\t\t\tlist_add(&to_add->list, head);\n+\t\tapply_arg(NULL, arg_tok, head, NULL, 0);\n \t\tbreak;\n-\tdefault:\n-\t\tdie(\"BUG: trailer.c: unhandled value %d\",\n-\t\t arg_tok->conf.if_missing);\n \t}\n }\n \n@@ -323,26 +356,18 @@ static int find_same_and_apply_arg(struct list_head *head,\n {\n \tstruct list_head *pos;\n \tstruct trailer_item *in_tok;\n-\tstruct trailer_item *on_tok;\n \n \tenum trailer_where where = arg_tok->conf.where;\n-\tint middle = (where == WHERE_AFTER) || (where == WHERE_BEFORE);\n-\tint backwards = after_or_end(where);\n-\tstruct trailer_item *start_tok;\n+\tint backwards = scan_backwards(where);\n \n \tif (list_empty(head))\n \t\treturn 0;\n \n-\tstart_tok = list_entry(backwards ? head->prev : head->next,\n-\t\t\t struct trailer_item,\n-\t\t\t list);\n-\n \tlist_for_each_dir(pos, head, backwards) {\n \t\tin_tok = list_entry(pos, struct trailer_item, list);\n \t\tif (!same_token(in_tok, arg_tok))\n \t\t\tcontinue;\n-\t\ton_tok = middle ? in_tok : start_tok;\n-\t\tapply_arg_if_exists(in_tok, arg_tok, on_tok, head);\n+\t\tapply_arg_if_exists(in_tok, arg_tok, head);\n \t\treturn 1;\n \t}\n \treturn 0;\n-- \n2.14.2\n\n\n"},{"id":"329767","messageId":"20171005132243.27058-5-pbonzini@redhat.com","threadId":"46911","inReplyTo":"20171005132243.27058-1-pbonzini@redhat.com","subject":"[PATCH 4/4] trailer: add \"move\" configuration for trailer.ifExists","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2017-10-05T13:22:43Z","receivedAt":"2017-10-05T13:23:02Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"In some cases, people apply patches to a queue branch immediately\nwith \"git am -3 -s\", and later collect Reviewed-by or Acked-by\ntrailers as they come in from the mailing list.\n\nIn this case, \"where=after\" does not have the desired behavior,\nbecause it will add the trailer in an unorthodox position, after the\ncommitter's Signed-off-by line. The \"move\" configuration is intended\nto be applied in such a case to the Signed-off-by header, like\n\n git interpret-trailers \\\n\t--where end --if-missing doNothing --if-exists move\n\t\"Signed-off-by: A U Thor \"\n\nor perhaps with an automated configuration\n\n [trailer \"move-sob\"]\n command = \"'echo \\\"$(git config user.name) <$(git config user.email)>\\\"'\"\n key = Signed-off-by\n where = end\n ifMissing = doNothing\n ifExists = move\n\nThough this of course makes the most sense if the last Signed-off-by is\nfrom the committer itself, and thus is not necessarily a good idea for\neveryone.\n\nSigned-off-by: Paolo Bonzini \n---\n Documentation/git-interpret-trailers.txt | 13 ++++++++---\n t/t7513-interpret-trailers.sh | 37 ++++++++++++++++++++++++++++++++\n trailer.c | 26 +++++++++++++++++-----\n trailer.h | 1 +\n 4 files changed, 69 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt\nindex 9dd19a1dd..1cdde492c 100644\n--- a/Documentation/git-interpret-trailers.txt\n+++ b/Documentation/git-interpret-trailers.txt\n@@ -165,7 +165,7 @@ trailer.ifexists::\n \tsame in the message.\n +\n The valid values for this option are: `addIfDifferentNeighbor` (this\n-is the default), `addIfDifferent`, `add`, `replace` or `doNothing`.\n+is the default), `addIfDifferent`, `add`, `replace`, `move` or `doNothing`.\n +\n With `addIfDifferentNeighbor`, a new trailer will be added only if no\n trailer with the same (, ) pair is above or below the line\n@@ -182,13 +182,20 @@ deleted and the new trailer will be added. The deleted trailer will be\n the closest one (with the same ) to the place where the new one\n will be added.\n +\n+With `move`, an existing trailer with the same (, ) will be\n+deleted and the new trailer will be added. If more equal pairs exists,\n+the deleted trailer will be the closest one to the place where the new one\n+will be added.\n++\n With `doNothing`, nothing will be done; that is no new trailer will be\n added if there is already one with the same in the message.\n \n trailer.ifmissing::\n \tThis option makes it possible to choose what action will be\n-\tperformed when there is not yet any trailer with the same\n-\t in the message.\n+\tperformed when the `ifexists` action fails. This means that\n+\tthere is not yet any trailer with the same in the message\n+\tor, for the `move` action only, there is no identical trailer\n+\tin the message.\n +\n The valid values for this option are: `add` (this is the default) and\n `doNothing`.\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 164719d1c..a0f21fefd 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -1052,6 +1052,43 @@ test_expect_success 'using \"ifExists = doNothing\"' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'require token and value match when \"ifExists = move\"' '\n+\tgit config trailer.fix.ifExists \"move\" &&\n+\tgit config trailer.fix.where \"start\" &&\n+\tcat complex_message_body >expected &&\n+\tsed -e \"s/ Z\\$/ /\" >>expected <<-\\EOF &&\n+\t\tFixes: 22\n+\t\tFixes: Z\n+\t\tAcked-by= Z\n+\t\tReviewed-by:\n+\t\tSigned-off-by: Z\n+\t\tFixes: 53\n+\tEOF\n+\t(cat complex_message; echo Fixes: 53) | \\\n+\tgit interpret-trailers \\\n+\t\t--trailer \"fix=22\" \\\n+\t\t>actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'look for all trailers when \"ifExists = move\"' '\n+\tgit config trailer.fix.ifMissing \"doNothing\" &&\n+\tcat complex_message_body >expected &&\n+\tsed -e \"s/ Z\\$/ /\" >>expected <<-\\EOF &&\n+\t\tFixes: 22\n+\t\tFixes: Z\n+\t\tAcked-by= Z\n+\t\tReviewed-by:\n+\t\tSigned-off-by: Z\n+\t\tFixes: 53\n+\tEOF\n+\t(cat complex_message; echo Fixes: 22; echo Fixes: 53) | \\\n+\tgit interpret-trailers \\\n+\t\t--trailer \"fix=22\" \\\n+\t\t>actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'the default is \"ifMissing = add\"' '\n \tgit config trailer.cc.key \"Cc: \" &&\n \tgit config trailer.cc.where \"before\" &&\ndiff --git a/trailer.c b/trailer.c\nindex ce0d94074..4915bda8f 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -202,6 +202,14 @@ static int check_if_different(struct trailer_item *in_tok,\n \treturn 0;\n }\n \n+static int check_if_same(struct trailer_item *in_tok,\n+\t\t\t struct trailer_item *neighbor,\n+\t\t\t struct arg_item *arg_tok,\n+\t\t\t struct list_head *head)\n+{\n+\treturn same_trailer(in_tok, arg_tok);\n+}\n+\n static int check_if_different_neighbor(struct trailer_item *in_tok,\n \t\t\t\t struct trailer_item *neighbor,\n \t\t\t\t struct arg_item *arg_tok,\n@@ -255,6 +263,8 @@ static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg\n \t\t}\n \t\targ_tok->value = apply_command(arg_tok->conf.command, arg);\n \t\tfree((char *)arg);\n+\t\tfree(arg_tok->conf.command);\n+\t\targ_tok->conf.command = NULL;\n \t}\n }\n \n@@ -314,9 +324,9 @@ static int apply_arg(struct trailer_item *in_tok,\n \treturn 1;\n }\n \n-static void apply_arg_if_exists(struct trailer_item *in_tok,\n-\t\t\t\tstruct arg_item *arg_tok,\n-\t\t\t\tstruct list_head *head)\n+static int apply_arg_if_exists(struct trailer_item *in_tok,\n+\t\t\t struct arg_item *arg_tok,\n+\t\t\t struct list_head *head)\n {\n \tswitch (arg_tok->conf.if_exists) {\n \tcase EXISTS_DO_NOTHING:\n@@ -324,6 +334,9 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,\n \tcase EXISTS_REPLACE:\n \t\tapply_arg(in_tok, arg_tok, head, NULL, 1);\n \t\tbreak;\n+\tcase EXISTS_MOVE:\n+\t\t/* Look for another trailer if the match fails. */\n+\t\treturn apply_arg(in_tok, arg_tok, head, check_if_same, 1);\n \tcase EXISTS_ADD:\n \t\tapply_arg(in_tok, arg_tok, head, NULL, 0);\n \t\tbreak;\n@@ -337,6 +350,7 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,\n \t\tdie(\"BUG: trailer.c: unhandled value %d\",\n \t\t arg_tok->conf.if_exists);\n \t}\n+\treturn 1;\n }\n \n static void apply_arg_if_missing(struct list_head *head,\n@@ -367,8 +381,8 @@ static int find_same_and_apply_arg(struct list_head *head,\n \t\tin_tok = list_entry(pos, struct trailer_item, list);\n \t\tif (!same_token(in_tok, arg_tok))\n \t\t\tcontinue;\n-\t\tapply_arg_if_exists(in_tok, arg_tok, head);\n-\t\treturn 1;\n+\t\tif (apply_arg_if_exists(in_tok, arg_tok, head))\n+\t\t\treturn 1;\n \t}\n \treturn 0;\n }\n@@ -423,6 +437,8 @@ int trailer_set_if_exists(enum trailer_if_exists *item, const char *value)\n \t\t*item = EXISTS_ADD;\n \telse if (!strcasecmp(\"replace\", value))\n \t\t*item = EXISTS_REPLACE;\n+\telse if (!strcasecmp(\"move\", value))\n+\t\t*item = EXISTS_MOVE;\n \telse if (!strcasecmp(\"doNothing\", value))\n \t\t*item = EXISTS_DO_NOTHING;\n \telse\ndiff --git a/trailer.h b/trailer.h\nindex 6d7f8c2a5..0fbdab38d 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -16,6 +16,7 @@ enum trailer_if_exists {\n \tEXISTS_ADD_IF_DIFFERENT,\n \tEXISTS_ADD,\n \tEXISTS_REPLACE,\n+\tEXISTS_MOVE,\n \tEXISTS_DO_NOTHING\n };\n enum trailer_if_missing {\n-- \n2.14.2\n\n"},{"id":"329768","messageId":"20171005132243.27058-2-pbonzini@redhat.com","threadId":"46911","inReplyTo":"20171005132243.27058-1-pbonzini@redhat.com","subject":"[PATCH 1/4] trailer: push free_arg_item up","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2017-10-05T13:22:40Z","receivedAt":"2017-10-05T13:23:06Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"All callees of process_trailers_lists are calling free_arg_item.\nJust do it in process_trailers_lists itself.\n\nSigned-off-by: Paolo Bonzini \n---\n trailer.c | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 3ba157ed0..4ba28ae33 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -178,7 +178,6 @@ static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)\n \tnew->token = arg_tok->token;\n \tnew->value = arg_tok->value;\n \targ_tok->token = arg_tok->value = NULL;\n-\tfree_arg_item(arg_tok);\n \treturn new;\n }\n \n@@ -271,7 +270,6 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,\n {\n \tswitch (arg_tok->conf.if_exists) {\n \tcase EXISTS_DO_NOTHING:\n-\t\tfree_arg_item(arg_tok);\n \t\tbreak;\n \tcase EXISTS_REPLACE:\n \t\tapply_item_command(in_tok, arg_tok);\n@@ -287,15 +285,11 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,\n \t\tapply_item_command(in_tok, arg_tok);\n \t\tif (check_if_different(in_tok, arg_tok, 1, head))\n \t\t\tadd_arg_to_input_list(on_tok, arg_tok);\n-\t\telse\n-\t\t\tfree_arg_item(arg_tok);\n \t\tbreak;\n \tcase EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:\n \t\tapply_item_command(in_tok, arg_tok);\n \t\tif (check_if_different(on_tok, arg_tok, 0, head))\n \t\t\tadd_arg_to_input_list(on_tok, arg_tok);\n-\t\telse\n-\t\t\tfree_arg_item(arg_tok);\n \t\tbreak;\n \tdefault:\n \t\tdie(\"BUG: trailer.c: unhandled value %d\",\n@@ -311,7 +305,6 @@ static void apply_arg_if_missing(struct list_head *head,\n \n \tswitch (arg_tok->conf.if_missing) {\n \tcase MISSING_DO_NOTHING:\n-\t\tfree_arg_item(arg_tok);\n \t\tbreak;\n \tcase MISSING_ADD:\n \t\twhere = arg_tok->conf.where;\n@@ -374,6 +367,8 @@ static void process_trailers_lists(struct list_head *head,\n \n \t\tif (!applied)\n \t\t\tapply_arg_if_missing(head, arg_tok);\n+\n+\t\tfree_arg_item(arg_tok);\n \t}\n }\n \n-- \n2.14.2\n\n\n"},{"id":"329769","messageId":"20171005132243.27058-3-pbonzini@redhat.com","threadId":"46911","inReplyTo":"20171005132243.27058-1-pbonzini@redhat.com","subject":"[PATCH 2/4] trailer: simplify check_if_different","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2017-10-05T13:22:41Z","receivedAt":"2017-10-05T13:23:30Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"The check_all argument is pointless, because the function degenerates\nto !same_trailer when check_all==0 (if same_trailer fails, it always\nends up returning 1). Remove it, switching the check_all==0 caller\nto use same_trailer directly.\n\nSigned-off-by: Paolo Bonzini \n---\n trailer.c | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 4ba28ae33..91f89db7f 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -194,14 +194,11 @@ static void add_arg_to_input_list(struct trailer_item *on_tok,\n \n static int check_if_different(struct trailer_item *in_tok,\n \t\t\t struct arg_item *arg_tok,\n-\t\t\t int check_all,\n \t\t\t struct list_head *head)\n {\n \tenum trailer_where where = arg_tok->conf.where;\n \tstruct list_head *next_head;\n-\tdo {\n-\t\tif (same_trailer(in_tok, arg_tok))\n-\t\t\treturn 0;\n+\twhile (!same_trailer(in_tok, arg_tok)) {\n \t\t/*\n \t\t * if we want to add a trailer after another one,\n \t\t * we have to check those before this one\n@@ -209,10 +206,10 @@ static int check_if_different(struct trailer_item *in_tok,\n \t\tnext_head = after_or_end(where) ? in_tok->list.prev\n \t\t\t\t\t\t: in_tok->list.next;\n \t\tif (next_head == head)\n-\t\t\tbreak;\n+\t\t\treturn 1;\n \t\tin_tok = list_entry(next_head, struct trailer_item, list);\n-\t} while (check_all);\n-\treturn 1;\n+\t}\n+\treturn 0;\n }\n \n static char *apply_command(const char *command, const char *arg)\n@@ -283,12 +280,12 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,\n \t\tbreak;\n \tcase EXISTS_ADD_IF_DIFFERENT:\n \t\tapply_item_command(in_tok, arg_tok);\n-\t\tif (check_if_different(in_tok, arg_tok, 1, head))\n+\t\tif (check_if_different(in_tok, arg_tok, head))\n \t\t\tadd_arg_to_input_list(on_tok, arg_tok);\n \t\tbreak;\n \tcase EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:\n \t\tapply_item_command(in_tok, arg_tok);\n-\t\tif (check_if_different(on_tok, arg_tok, 0, head))\n+\t\tif (!same_trailer(on_tok, arg_tok))\n \t\t\tadd_arg_to_input_list(on_tok, arg_tok);\n \t\tbreak;\n \tdefault:\n-- \n2.14.2\n\n\n"},{"id":"329856","messageId":"xmqq8tgowzaa.fsf@gitster.mtv.corp.google.com","threadId":"46911","inReplyTo":"20171005132243.27058-1-pbonzini@redhat.com","subject":"Re: [RFC PATCH 0/4] interpret-trailers: introduce \"move\" action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T06:44:45Z","receivedAt":"2017-10-06T06:44:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paolo Bonzini writes:\n\n> The purpose of this action is for scripts to be able to keep the\n> user's Signed-off-by at the end. For example say I have a script\n> that adds a Reviewed-by tag:\n>\n> #! /bin/sh\n> them=$(git log -i -1 --pretty='format:%an <%ae>' --author=\"$*\")\n> trailer=\"Reviewed-by: $them\"\n> git log -1 --pretty=format:%B | \\\n> git interpret-trailers --where end --if-exists doNothing --trailer \"$trailer\" | \\\n> git commit --amend -F-\n>\n> Now, this script will leave my Signed-off-by line in a non-canonical\n> place, like\n>\n> Signed-off-by: Paolo Bonzini \n> Reviewed-by: Junio C Hamano \n>\n> This new option enables the following improvement:\n>\n> #! /bin/sh\n> me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')\n> them=$(git log -i -1 --pretty='format:%an <%ae>' --author=\"$*\")\n> trailer=\"Reviewed-by: $them\"\n> sob=\"Signed-off-by: $me\"\n> git log -1 --pretty=format:%B | \\\n> git interpret-trailers --where end --if-exists doNothing --trailer \"$trailer\" \\\n> --where end --if-exists move --if-missing doNothing --trailer \"$sob\" | \\\n> git commit --amend -F-\n>\n> which lets me keep the SoB line at the end, as it should be.\n> Posting as RFC because it's possible that I'm missing a simpler\n> way to achieve this...\n\nWhile I think \"move\" may turn out to be handy in some use case, an\nexample to move S-o-b does not sound convincing to me at all. \n\nIf anything, the above (assuming that you wrote a patch, sent out\nfor a review with or without signing it off, and then after getting\na review, you are adding reviewed-by to the commit) does not\ndemonstrate the need for \"move\". The use of \"move\" in the example\nlooks like a mere workaround that reviewed-by was added at the wrong\nplace (i.e. --where end) in the first place.\n\nBut that is not the primary reason why I find the example using\nS-o-b convincing. If the patch in your example originally did not\nhave just one S-o-b by you, but yours was at the end of the chain of\npatch passing, use of \"move\" may become even more problematic. Your\nfriend may write an original, sign it off and pass it to you, who\nthen signs it off and sends to the mailng list. It gets picked up\nby somebody else, who tweaks and adds her sign off, then you pick it\nup and relay it to the final destination (i.e. the first sign-off is\nby your friend, then you have two sign-offs of yours, one sign off\nfrom somebody else in between, and the chain records how the patch\n\"flowed\"). And then Linus says \"yeah, this is good, I throughly\nreviewed it.\" Where would you place that reviewed-by? Before your\nsecond (and last) sign-off? What makes that last one special?\nWould it more faithfully reflect the order of events if you added\nLinus's reviewed-by and then your own sign-off to conclude the\nchain?\n\nSo I am not opposed to the idea of \"move\", but I am not sure in what\nsituation it is useful and what use case it makes it easier to use.\nThe example makes me suspect that what we want is not a new\noperation, but a way to specify \"where\" in a richer way.\n"},{"id":"329857","messageId":"6ba01966-682a-6515-b1c2-3048da6158e0@redhat.com","threadId":"46911","inReplyTo":"xmqq8tgowzaa.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 0/4] interpret-trailers: introduce \"move\" action","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2017-10-06T07:26:02Z","receivedAt":"2017-10-06T07:26:10Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 06/10/2017 08:44, Junio C Hamano wrote:\n> Paolo Bonzini writes:\n>> The purpose of this action is for scripts to be able to keep the\n>> user's Signed-off-by at the end.\n>>\n>> #! /bin/sh\n>> me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')\n>> them=$(git log -i -1 --pretty='format:%an <%ae>' --author=\"$*\")\n>> trailer=\"Reviewed-by: $them\"\n>> sob=\"Signed-off-by: $me\"\n>> git log -1 --pretty=format:%B | \\\n>> git interpret-trailers --where end --if-exists doNothing --trailer \"$trailer\" \\\n>> --where end --if-exists move --if-missing doNothing --trailer \"$sob\" | \\\n>> git commit --amend -F-\n>>\n>> which lets me keep the SoB line at the end, as it should be.\n>> Posting as RFC because it's possible that I'm missing a simpler\n>> way to achieve this...\n>\n> If anything, the above (assuming that you wrote a patch, sent out\n> for a review with or without signing it off, and then after getting\n> a review, you are adding reviewed-by to the commit) does not\n> demonstrate the need for \"move\". The use of \"move\" in the example\n> looks like a mere workaround that reviewed-by was added at the wrong\n> place (i.e. --where end) in the first place.\n\nYes, I agree. Though I also tried implementing \"--where beforeLastSOB\", \nand in the end decided against it because there were more corner cases.\nI include the patch for reference at the end of this message, to be\napplied on top of these four.\n\n> But that is not the primary reason why I find the example using\n> S-o-b convincing. If the patch in your example originally did not\n> have just one S-o-b by you, but yours was at the end of the chain of\n> patch passing, use of \"move\" may become even more problematic. Your\n> friend may write an original, sign it off and pass it to you, who\n> then signs it off and sends to the mailng list. It gets picked up\n> by somebody else, who tweaks and adds her sign off, then you pick it\n> up and relay it to the final destination (i.e. the first sign-off is\n> by your friend, then you have two sign-offs of yours, one sign off\n> from somebody else in between, and the chain records how the patch\n> \"flowed\"). And then Linus says \"yeah, this is good, I throughly\n> reviewed it.\" Where would you place that reviewed-by? Before your\n> second (and last) sign-off? What makes that last one special?\n\nSo:\n\n Signed-off-by: Me\n Signed-off-by: Friend\n Signed-off-by: Me <<<\n Reviewed-by: Linus\n Signed-off-by: Me\n\nI do think the SoB line marked with \"<<<\" is a bit \"special\". SoB lines \nbefore it represent the path followed by the contribution, according to \nclause (c) of the Developer Certificate of Origin. Multiple \n*consecutive* SoB lines from the same person do not add much, while \nmultiple separate SoB lines from the same person must be kept.\n\nOf course, using \"--where start\" for Reviewed/Tested/Acked-by lines *is* \nan option. On the other hand, for \"Cc: stable@vger.kernel.org\" the \nplacement hints at *who* decided the patch to be worth of inclusion in a \nstable version. That person might be the right one to bug if the patch \ndoesn't apply and needs a manual backport. It's not science of course, \nbut in practice I found the \"always apply with -s, and use 'move' to \nkeep my SoB at the end\" workflow to be the least error-prone.\n\n> Would it more faithfully reflect the order of events if you added\n> Linus's reviewed-by and then your own sign-off to conclude the\n> chain?\n\nPossibly, but the DCO doesn't care and SoB lines are first and firemost \nabout the DCO.\n\nOn the other hand, \"move\" does not provide exactly what we want in the \ncase where the user's SoB is there, but is not the last. So, the above \nscript pretty much assumes that you apply the patch with \"-s\"; if you \ndidn't, you'd need something more like \"moveLast\". It is trivial to \nimplement \"moveLast\" on top of the first three patches in this series, \nbut things start getting a bit out of hand perhaps...\n\nPaolo\n\n> So I am not opposed to the idea of \"move\", but I am not sure in what\n> situation it is useful and what use case it makes it easier to use.\n> The example makes me suspect that what we want is not a new\n> operation, but a way to specify \"where\" in a richer way.\n\n----------- 8< ------------\nFrom a238b973821586eaaf26d608172cdc72f19d6071 Mon Sep 17 00:00:00 2001\nFrom: Paolo Bonzini \nDate: Wed, 12 Jul 2017 18:11:44 +0200\nSubject: [PATCH] trailer: add \"beforeLastSOB\" configuration for trailer.where\n\nIn some cases, people apply patches to a queue branch immediately\nwith \"git am -3 -s\", and later collect Reviewed-by or Acked-by\ntrailers as they come in from the mailing list.\n\nIn this case, \"where=after\" does not have the desired behavior,\nbecause it will add the trailer in an unorthodox position, after the\ncommitter's Signed-off-by line. Introduce a \"beforeLastSOB\" value\nfor trailer.where; this of course makes the most sense if the\nlast Signed-off-by is from the committer itself.\n\nSigned-off-by: Paolo Bonzini \n---\n Documentation/git-interpret-trailers.txt | 4 ++\n t/t7513-interpret-trailers.sh | 86 +++++++++++++++++++++++++++++++-\n trailer.c | 41 ++++++++++++++-\n trailer.h | 3 +-\n 4 files changed, 130 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt\nindex 1cdde492c..e012f11c1 100644\n--- a/Documentation/git-interpret-trailers.txt\n+++ b/Documentation/git-interpret-trailers.txt\n@@ -158,6 +158,10 @@ last trailer with the same .\n +\n If it is `before`, then each new trailer will appear just before the\n first trailer with the same .\n++\n+If it is `beforeLastSOB`, then each new trailer will appear just\n+before the last Signed-off-by line. If there is no such line, the\n+trailer will appear at the end of the existing trailers.\n \n trailer.ifexists::\n \tThis option makes it possible to choose what action will be\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex a0f21fefd..06d6226cd 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -727,6 +727,25 @@ test_expect_success 'using \"where = after\"' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'using \"where = beforeLastSOB\"' '\n+\tgit config trailer.review.key \"Reviewed-by\" &&\n+\tgit config trailer.review.where \"beforeLastSOB\" &&\n+\tcat complex_message_body >expected &&\n+\tsed -e \"s/ Z\\$/ /\" >>expected <<-\\EOF &&\n+\t\tFixes: Z\n+\t\tAcked-by= Z\n+\t\tReviewed-by: Z\n+\t\tSigned-off-by: Z\n+\t\tReviewed-by: Johannes\n+\t\tSigned-off-by: Junio\n+\tEOF\n+\tgit interpret-trailers --trailer \"Signed-off-by: Junio\" \\\n+\t\t--trailer \"Reviewed-by: Johannes\" \\\n+\t\tcomplex_message >actual &&\n+\tgit config trailer.ack.where \"after\" &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'using \"where = end\"' '\n \tgit config trailer.review.key \"Reviewed-by\" &&\n \tgit config trailer.review.where \"end\" &&\n@@ -833,6 +852,26 @@ test_expect_success 'default \"ifExists\" is now \"addIfDifferent\"' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'using \"ifExists = addIfDifferent\" with \"where = beforeLastSOB\"' '\n+\tgit config trailer.ack.ifExists \"addIfDifferent\" &&\n+\tgit config trailer.ack.where \"beforeLastSOB\" &&\n+\tcat complex_message_body >expected &&\n+\tsed -e \"s/ Z\\$/ /\" >>expected <<-\\EOF &&\n+\t\tFixes: Z\n+\t\tAcked-by= Z\n+\t\tReviewed-by:\n+\t\tSigned-off-by: Z\n+\t\tAcked-by= Johannes\n+\t\tAcked-by= Peff\n+\t\tSigned-off-by: Junio\n+\tEOF\n+\tgit interpret-trailers --trailer \"Signed-off-by: Junio\" \\\n+\t\t--trailer \"ack: Johannes\" --trailer \"ack: Johannes\" \\\n+\t\t--trailer \"ack: Peff\" --trailer \"ack: Johannes\" \\\n+\t\tcomplex_message >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'using \"ifExists = addIfDifferent\" with \"where = end\"' '\n \tgit config trailer.ack.ifExists \"addIfDifferent\" &&\n \tgit config trailer.ack.where \"end\" &&\n@@ -892,6 +931,27 @@ test_expect_success 'using \"ifExists = addIfDifferentNeighbor\" with \"where = end\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'using \"ifExists = addIfDifferentNeighbor\" with \"where = beforeLastSOB\"' '\n+\tgit config trailer.ack.ifExists \"addIfDifferentNeighbor\" &&\n+\tgit config trailer.ack.where \"beforeLastSOB\" &&\n+\tcat complex_message_body >expected &&\n+\tsed -e \"s/ Z\\$/ /\" >>expected <<-\\EOF &&\n+\t\tFixes: Z\n+\t\tAcked-by= Z\n+\t\tReviewed-by:\n+\t\tSigned-off-by: Z\n+\t\tAcked-by= Johannes\n+\t\tAcked-by= Peff\n+\t\tAcked-by= Johannes\n+\t\tSigned-off-by: Junio\n+\tEOF\n+\tgit interpret-trailers --trailer \"Signed-off-by: Junio\" \\\n+\t\t--trailer \"ack: Johannes\" --trailer \"ack: Johannes\" \\\n+\t\t--trailer \"ack: Peff\" --trailer \"ack: Johannes\" \\\n+\t\tcomplex_message >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'using \"ifExists = addIfDifferentNeighbor\" with \"where = after\"' '\n \tgit config trailer.ack.ifExists \"addIfDifferentNeighbor\" &&\n \tgit config trailer.ack.where \"after\" &&\n@@ -1211,6 +1271,30 @@ test_expect_success 'using \"ifMissing = doNothing\"' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'using \"where = beforeLastSOB\" to add Signed-off-by' '\n+\tgit config trailer.sign.key \"Signed-off-by: \" &&\n+\tgit config trailer.sign.ifExists \"addIfDifferentNeighbor\" &&\n+\tgit config trailer.sign.where \"beforeLastSOB\" &&\n+\tcat complex_message_body >expected &&\n+\tsed -e \"s/ Z\\$/ /\" >>expected <<-\\EOF &&\n+\t\tFixes: Z\n+\t\tAcked-by= Z\n+\t\tReviewed-by:\n+\t\tSigned-off-by: Junio\n+\t\tSigned-off-by: Johannes\n+\t\tSigned-off-by: Another\n+\t\tSigned-off-by: Junio\n+\t\tSigned-off-by: Z\n+\tEOF\n+\tgit interpret-trailers --trailer \"Signed-off-by: Junio\" |\n+\tgit interpret-trailers --trailer \"Signed-off-by: Junio\" \\\n+\t\t--trailer \"Signed-off-by: Johannes\" \\\n+\t\t--trailer \"Signed-off-by: Another\" \\\n+\t\t--trailer \"Signed-off-by: Junio\" \\\n+\t\tcomplex_message >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'default \"where\" is now \"after\"' '\n \tgit config trailer.where \"after\" &&\n \tgit config --unset trailer.ack.where &&\n@@ -1237,9 +1321,7 @@ test_expect_success 'default \"where\" is now \"after\"' '\n '\n \n test_expect_success 'with simple command' '\n-\tgit config trailer.sign.key \"Signed-off-by: \" &&\n \tgit config trailer.sign.where \"after\" &&\n-\tgit config trailer.sign.ifExists \"addIfDifferentNeighbor\" &&\n \tgit config trailer.sign.command \"echo \\\"A U Thor \\\"\" &&\n \tcat complex_message_body >expected &&\n \tsed -e \"s/ Z\\$/ /\" >>expected <<-\\EOF &&\ndiff --git a/trailer.c b/trailer.c\nindex 4915bda8f..c1ab69e58 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -60,7 +60,12 @@ static const char *git_generated_prefixes[] = {\n \n static int scan_backwards(enum trailer_where where)\n {\n-\treturn (where == WHERE_AFTER) || (where == WHERE_END);\n+\t/*\n+\t * beforeLastSOB is almost like \"end\", only the placement of the new\n+\t * trailer varies.\n+\t */\n+\treturn (where == WHERE_AFTER) || (where == WHERE_END) ||\n+\t\t(where == WHERE_BEFORE_LAST_SOB);\n }\n \n /*\n@@ -100,6 +105,24 @@ static int same_trailer(struct trailer_item *a, struct arg_item *b)\n \treturn same_token(a, b) && same_value(a, b);\n }\n \n+static int is_sob(struct trailer_item *in_tok)\n+{\n+\treturn !strncasecmp(in_tok->token, \"Signed-off-by\", 13);\n+}\n+\n+static struct list_head *find_last_sob(struct list_head *head)\n+{\n+\tstruct list_head *pos;\n+\tstruct trailer_item *in_tok;\n+\n+\tlist_for_each_prev(pos, head) {\n+\t\tin_tok = list_entry(pos, struct trailer_item, list);\n+\t\tif (is_sob(in_tok))\n+\t\t\treturn pos;\n+\t}\n+\treturn head;\n+}\n+\n static inline int is_blank_line(const char *str)\n {\n \tconst char *s = str;\n@@ -297,6 +320,20 @@ static int apply_arg(struct trailer_item *in_tok,\n \t\tadd_after = 1;\n \t\tplace = head;\n \t\tneighbor = NULL;\n+\t} else if (where == WHERE_BEFORE_LAST_SOB) {\n+\t\tadd_after = 0;\n+\t\tplace = find_last_sob(head);\n+\n+\t\t/*\n+\t\t * Trying to compare two trailers of the same kind provides the\n+\t\t * most sensible results for addIfDifferentNeighbor (see testsuite).\n+\t\t * So when *not* adding a SOB, beforeLastSOB compares to the element\n+\t\t * before.\n+\t\t */\n+\t\tif (strcasecmp(arg_tok->token, \"Signed-off-by\"))\n+\t\t\tneighbor = list_entry(place->prev, struct trailer_item, list);\n+\t\telse\n+\t\t\tneighbor = in_tok;\n \t} else if (middle) {\n \t\tadd_after = (where == WHERE_AFTER);\n \t\tplace = &in_tok->list;\n@@ -420,6 +457,8 @@ int trailer_set_where(enum trailer_where *item, const char *value)\n \t\t*item = WHERE_END;\n \telse if (!strcasecmp(\"start\", value))\n \t\t*item = WHERE_START;\n+\telse if (!strcasecmp(\"beforeLastSOB\", value))\n+\t\t*item = WHERE_BEFORE_LAST_SOB;\n \telse\n \t\treturn -1;\n \treturn 0;\ndiff --git a/trailer.h b/trailer.h\nindex 0fbdab38d..e30f59e47 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -8,7 +8,8 @@ enum trailer_where {\n \tWHERE_END,\n \tWHERE_AFTER,\n \tWHERE_BEFORE,\n-\tWHERE_START\n+\tWHERE_START,\n+\tWHERE_BEFORE_LAST_SOB\n };\n enum trailer_if_exists {\n \tEXISTS_DEFAULT,\n-- \n2.14.2\n"},{"id":"329868","messageId":"CAP8UFD1X-aRN5sAB5PQt04jL_92APK279bjNf=Zt_x8KOxyL+A@mail.gmail.com","threadId":"46911","inReplyTo":"20171005132243.27058-1-pbonzini@redhat.com","subject":"Re: [RFC PATCH 0/4] interpret-trailers: introduce \"move\" action","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-10-06T10:30:36Z","receivedAt":"2017-10-06T10:30:43Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Oct 5, 2017 at 3:22 PM, Paolo Bonzini wrote:\n> The purpose of this action is for scripts to be able to keep the\n> user's Signed-off-by at the end. For example say I have a script\n> that adds a Reviewed-by tag:\n>\n> #! /bin/sh\n> them=$(git log -i -1 --pretty='format:%an <%ae>' --author=\"$*\")\n> trailer=\"Reviewed-by: $them\"\n> git log -1 --pretty=format:%B | \\\n> git interpret-trailers --where end --if-exists doNothing --trailer \"$trailer\" | \\\n> git commit --amend -F-\n>\n> Now, this script will leave my Signed-off-by line in a non-canonical\n> place, like\n>\n> Signed-off-by: Paolo Bonzini \n> Reviewed-by: Junio C Hamano \n>\n> This new option enables the following improvement:\n>\n> #! /bin/sh\n> me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')\n> them=$(git log -i -1 --pretty='format:%an <%ae>' --author=\"$*\")\n> trailer=\"Reviewed-by: $them\"\n> sob=\"Signed-off-by: $me\"\n> git log -1 --pretty=format:%B | \\\n> git interpret-trailers --where end --if-exists doNothing --trailer \"$trailer\" \\\n> --where end --if-exists move --if-missing doNothing --trailer \"$sob\" | \\\n> git commit --amend -F-\n>\n> which lets me keep the SoB line at the end, as it should be.\n> Posting as RFC because it's possible that I'm missing a simpler\n> way to achieve this...\n\nDid you try using `--where end --if-exists replace --trailer \"$sob\"`?\n"},{"id":"329869","messageId":"748131b7-bddd-08c2-ff72-9fd1a63ef6a0@redhat.com","threadId":"46911","inReplyTo":"CAP8UFD1X-aRN5sAB5PQt04jL_92APK279bjNf=Zt_x8KOxyL+A@mail.gmail.com","subject":"Re: [RFC PATCH 0/4] interpret-trailers: introduce \"move\" action","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2017-10-06T10:40:36Z","receivedAt":"2017-10-06T10:40:43Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 06/10/2017 12:30, Christian Couder wrote:\n> On Thu, Oct 5, 2017 at 3:22 PM, Paolo Bonzini wrote:\n>> The purpose of this action is for scripts to be able to keep the\n>> user's Signed-off-by at the end. For example say I have a script\n>> that adds a Reviewed-by tag:\n>>\n>> #! /bin/sh\n>> them=$(git log -i -1 --pretty='format:%an <%ae>' --author=\"$*\")\n>> trailer=\"Reviewed-by: $them\"\n>> git log -1 --pretty=format:%B | \\\n>> git interpret-trailers --where end --if-exists doNothing --trailer \"$trailer\" | \\\n>> git commit --amend -F-\n>>\n>> Now, this script will leave my Signed-off-by line in a non-canonical\n>> place, like\n>>\n>> Signed-off-by: Paolo Bonzini \n>> Reviewed-by: Junio C Hamano \n>>\n>> This new option enables the following improvement:\n>>\n>> #! /bin/sh\n>> me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')\n>> them=$(git log -i -1 --pretty='format:%an <%ae>' --author=\"$*\")\n>> trailer=\"Reviewed-by: $them\"\n>> sob=\"Signed-off-by: $me\"\n>> git log -1 --pretty=format:%B | \\\n>> git interpret-trailers --where end --if-exists doNothing --trailer \"$trailer\" \\\n>> --where end --if-exists move --if-missing doNothing --trailer \"$sob\" | \\\n>> git commit --amend -F-\n>>\n>> which lets me keep the SoB line at the end, as it should be.\n>> Posting as RFC because it's possible that I'm missing a simpler\n>> way to achieve this...\n> \n> Did you try using `--where end --if-exists replace --trailer \"$sob\"`?\n\nYes, it's a different behavior; \"--if-exists replace\" matches on others'\nSoB as well, so it would eat the original author's SoB if I didn't have one.\n\nSo \"move\" does get it wrong for\n\n Signed-off-by: Me\n Signed-off-by: Friend\n\n(Me gets moved last, which may not be what you want) but \"replace\" gets\nit wrong in the arguably more common case of\n\n Signed-off-by: Friend\n\nwhich is damaged to just \"Signed-off-by: Me\".\n\nPaolo\n"},{"id":"329878","messageId":"CAP8UFD28vVx51xhDgQVesm356XAjfwb286baER-U6VOC+4NL4w@mail.gmail.com","threadId":"46911","inReplyTo":"748131b7-bddd-08c2-ff72-9fd1a63ef6a0@redhat.com","subject":"Re: [RFC PATCH 0/4] interpret-trailers: introduce \"move\" action","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-10-06T12:33:22Z","receivedAt":"2017-10-06T12:33:28Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Oct 6, 2017 at 12:40 PM, Paolo Bonzini wrote:\n> On 06/10/2017 12:30, Christian Couder wrote:\n\n>> Did you try using `--where end --if-exists replace --trailer \"$sob\"`?\n>\n> Yes, it's a different behavior; \"--if-exists replace\" matches on others'\n> SoB as well, so it would eat the original author's SoB if I didn't have one.\n>\n> So \"move\" does get it wrong for\n>\n> Signed-off-by: Me\n> Signed-off-by: Friend\n>\n> (Me gets moved last, which may not be what you want) but \"replace\" gets\n> it wrong in the arguably more common case of\n>\n> Signed-off-by: Friend\n>\n> which is damaged to just \"Signed-off-by: Me\".\n\nOk. I think you might want something called for example\n\"replaceIfIdenticalClose\" where \"IdenticalClose\" means: \"there is a\ntrailer with the same (, ) pair above or below the line\nwhere the replaced trailer will be put when ignoring trailers with a\ndifferent \".\n"},{"id":"329879","messageId":"fe023f38-01cc-2257-bbfe-3f4310193b41@redhat.com","threadId":"46911","inReplyTo":"CAP8UFD28vVx51xhDgQVesm356XAjfwb286baER-U6VOC+4NL4w@mail.gmail.com","subject":"Re: [RFC PATCH 0/4] interpret-trailers: introduce \"move\" action","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2017-10-06T12:39:46Z","receivedAt":"2017-10-06T12:39:59Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 06/10/2017 14:33, Christian Couder wrote:\n> Ok. I think you might want something called for example\n> \"replaceIfIdenticalClose\" where \"IdenticalClose\" means: \"there is a\n> trailer with the same (, ) pair above or below the line\n> where the replaced trailer will be put when ignoring trailers with a\n> different \".\n\nSo basically \"moveIfClosest\" (move if last for where=end, move if first\nfor where=begin; for where=after and where=before it would just end up\ndoing nothing)? It's not hard to implement, but I'm wondering if it's\ntoo ad hoc.\n\nPaolo\n"},{"id":"329880","messageId":"CAP8UFD2_ZC4J4eRxq04TJ6-xyK5oTqHM2qd+5HfPV7jcoShvqw@mail.gmail.com","threadId":"46911","inReplyTo":"fe023f38-01cc-2257-bbfe-3f4310193b41@redhat.com","subject":"Re: [RFC PATCH 0/4] interpret-trailers: introduce \"move\" action","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-10-06T13:19:48Z","receivedAt":"2017-10-06T13:19:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Oct 6, 2017 at 2:39 PM, Paolo Bonzini wrote:\n> On 06/10/2017 14:33, Christian Couder wrote:\n>> Ok. I think you might want something called for example\n>> \"replaceIfIdenticalClose\" where \"IdenticalClose\" means: \"there is a\n>> trailer with the same (, ) pair above or below the line\n>> where the replaced trailer will be put when ignoring trailers with a\n>> different \".\n>\n> So basically \"moveIfClosest\" (move if last for where=end, move if first\n> for where=begin; for where=after and where=before it would just end up\n> doing nothing)?\n\nFirst yeah these would not make sense anyway if where=after or where=before.\n\nNow it would be strange to have \"moveIfClosest\" without having \"move\"\nfirst and I don't see how \"move\" would be different from the existing\n\"replace\".\nOr maybe \"move\" means \"replaceIfIdentical\", in this case I think it\nwould help users to just call it \"replaceIfIdentical\".\n\nAlso there is \"addIfDifferentNeighbor\" so we already have \"Neighbor\"\nwhich means \"just above or below\". Then if we use \"Closest\" I think it\nwill be harder to distinguish it from \"Neighbor\" than if we use\n\"Close\".\n\nThat's why I think \"replaceIfIdenticalClose\" is better. It could\nenable us to eventually use a regexp like\n\"(add|replace)(If(Different|Identical)(Close|Neighbor)+)+\" to parse\nthe add* and replace* options.\n"},{"id":"329886","messageId":"5dc97064-7d6a-b32d-c25b-202f85041373@redhat.com","threadId":"46911","inReplyTo":"CAP8UFD2_ZC4J4eRxq04TJ6-xyK5oTqHM2qd+5HfPV7jcoShvqw@mail.gmail.com","subject":"Re: [RFC PATCH 0/4] interpret-trailers: introduce \"move\" action","fromName":"Paolo Bonzini","fromEmail":"pbonzini@redhat.com","sentAt":"2017-10-06T13:49:43Z","receivedAt":"2017-10-06T13:49:50Z","isPatch":true,"sender":{"key":"pbonzini@redhat.com","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 06/10/2017 15:19, Christian Couder wrote:\n> Now it would be strange to have \"moveIfClosest\" without having \"move\"\n> first and I don't see how \"move\" would be different from the existing\n> \"replace\".\n> Or maybe \"move\" means \"replaceIfIdentical\", in this case I think it\n> would help users to just call it \"replaceIfIdentical\".\n\nWell, the effect of \"replacing if identical\" *is* to move the existing\nidentical trailer to the new position. :)\n\nPaolo\n"},{"id":"329953","messageId":"xmqqinfrvkzu.fsf@gitster.mtv.corp.google.com","threadId":"46911","inReplyTo":"fe023f38-01cc-2257-bbfe-3f4310193b41@redhat.com","subject":"Re: [RFC PATCH 0/4] interpret-trailers: introduce \"move\" action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-07T00:51:01Z","receivedAt":"2017-10-07T00:51:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paolo Bonzini writes:\n\n> On 06/10/2017 14:33, Christian Couder wrote:\n>> Ok. I think you might want something called for example\n>> \"replaceIfIdenticalClose\" where \"IdenticalClose\" means: \"there is a\n>> trailer with the same (, ) pair above or below the line\n>> where the replaced trailer will be put when ignoring trailers with a\n>> different \".\n>\n> So basically \"moveIfClosest\" (move if last for where=end, move if first\n> for where=begin; for where=after and where=before it would just end up\n> doing nothing)? It's not hard to implement, but I'm wondering if it's\n> too ad hoc.\n\nYeah, it makes me wonder exactly that, too.\n\n\n"}]}