{"thread":{"id":"41136","subject":"[PATCH] interpret-trailers: add option for in-place editing","startedAt":"2016-01-06T13:34:18Z","lastAt":"2016-01-07T12:42:17Z","messageCount":5,"participants":["Tobias Klauser","Matthieu Moy","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"275441","messageId":"1452087258-3893-1-git-send-email-tobias.klauser@zhinst.com","threadId":"41136","inReplyTo":null,"subject":"[PATCH] interpret-trailers: add option for in-place editing","fromName":"Tobias Klauser","fromEmail":"tobias.klauser@zhinst.com","sentAt":"2016-01-06T13:34:18Z","receivedAt":"2016-01-06T13:34:18Z","isPatch":true,"sender":{"key":"tobias.klauser@zhinst.com","avatar":null},"body":"From: Tobias Klauser <tklauser@distanz.ch>\n\nAdd a command line option --in-place to support in-place editing akin to\nsed -i.  This allows to write commands like the following:\n\n  git interpret-trailers --trailer \"X: Y\" a.txt > b.txt && mv b.txt a.txt\n\nin a more concise way:\n\n  git interpret-trailers --trailer \"X: Y\" --in-place a.txt\n\nAlso add the corresponding documentation and tests.\n\nSigned-off-by: Tobias Klauser <tklauser@distanz.ch>\n---\n Documentation/git-interpret-trailers.txt | 24 +++++++++++++++++++-\n builtin/interpret-trailers.c             | 13 +++++++----\n t/t7513-interpret-trailers.sh            | 32 ++++++++++++++++++++++++++\n trailer.c                                | 39 ++++++++++++++++++++------------\n trailer.h                                |  3 ++-\n 5 files changed, 91 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt\nindex 0ecd497c4de7..a77b901f1d7b 100644\n--- a/Documentation/git-interpret-trailers.txt\n+++ b/Documentation/git-interpret-trailers.txt\n@@ -8,7 +8,7 @@ git-interpret-trailers - help add structured information into commit messages\n SYNOPSIS\n --------\n [verse]\n-'git interpret-trailers' [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]\n+'git interpret-trailers' [--in-place] [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]\n \n DESCRIPTION\n -----------\n@@ -64,6 +64,9 @@ folding rules, the encoding rules and probably many other rules.\n \n OPTIONS\n -------\n+--in-place::\n+\tEdit the files in place.\n+\n --trim-empty::\n \tIf the <value> part of any trailer contains only whitespace,\n \tthe whole trailer will be removed from the resulting message.\n@@ -216,6 +219,25 @@ Signed-off-by: Alice <alice@example.com>\n Signed-off-by: Bob <bob@example.com>\n ------------\n \n+* Use the '--in-place' option to edit a message file in place:\n++\n+------------\n+$ cat msg.txt\n+subject\n+\n+message\n+\n+Signed-off-by: Bob <bob@example.com>\n+$ git interpret-trailers --trailer 'Acked-by: Alice <alice@example.com>' --in-place msg.txt\n+$ cat msg.txt\n+subject\n+\n+message\n+\n+Signed-off-by: Bob <bob@example.com>\n+Acked-by: Alice <alice@example.com>\n+------------\n+\n * Extract the last commit as a patch, and add a 'Cc' and a\n   'Reviewed-by' trailer to it:\n +\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex 46838d24a90a..b99ae4be8875 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -12,16 +12,18 @@\n #include \"trailer.h\"\n \n static const char * const git_interpret_trailers_usage[] = {\n-\tN_(\"git interpret-trailers [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]\"),\n+\tN_(\"git interpret-trailers [--in-place] [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]\"),\n \tNULL\n };\n \n int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n {\n+\tint in_place = 0;\n \tint trim_empty = 0;\n \tstruct string_list trailers = STRING_LIST_INIT_DUP;\n \n \tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"in-place\", &in_place, N_(\"edit files in place\")),\n \t\tOPT_BOOL(0, \"trim-empty\", &trim_empty, N_(\"trim empty trailers\")),\n \t\tOPT_STRING_LIST(0, \"trailer\", &trailers, N_(\"trailer\"),\n \t\t\t\tN_(\"trailer(s) to add\")),\n@@ -34,9 +36,12 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n \tif (argc) {\n \t\tint i;\n \t\tfor (i = 0; i < argc; i++)\n-\t\t\tprocess_trailers(argv[i], trim_empty, &trailers);\n-\t} else\n-\t\tprocess_trailers(NULL, trim_empty, &trailers);\n+\t\t\tprocess_trailers(argv[i], in_place, trim_empty, &trailers);\n+\t} else {\n+\t\tif (in_place)\n+\t\t\tdie(_(\"no input file given for in-place editing\"));\n+\t\tprocess_trailers(NULL, in_place, trim_empty, &trailers);\n+\t}\n \n \tstring_list_clear(&trailers, 0);\n \ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 322c436a494c..1103a4838b5c 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -326,6 +326,38 @@ test_expect_success 'with complex patch, args and --trim-empty' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'in-place editing with basic patch' '\n+\tcat basic_message >message &&\n+\tcat basic_patch >>message &&\n+\tcat basic_message >expected &&\n+\techo >>expected &&\n+\tcat basic_patch >>expected &&\n+\tgit interpret-trailers --in-place message &&\n+\ttest_cmp expected message\n+'\n+\n+test_expect_success 'in-place editing with additional trailer' '\n+\tcat basic_message >message &&\n+\tcat basic_patch >>message &&\n+\tcat basic_message >expected &&\n+\techo >>expected &&\n+\tcat >>expected <<-\\EOF &&\n+\t\tReviewed-by: Alice\n+\tEOF\n+\tcat basic_patch >>expected &&\n+\tgit interpret-trailers --trailer \"Reviewed-by: Alice\" --in-place message &&\n+\ttest_cmp expected message\n+'\n+\n+test_expect_success 'in-place editing on stdin' '\n+\ttest_must_fail git interpret-trailers --trailer \"Reviewed-by: Alice\" --in-place < basic_message\n+'\n+\n+test_expect_success 'in-place editing on non-existing file' '\n+\ttest_must_fail git interpret-trailers --trailer \"Reviewed-by: Alice\" --in-place nonexisting &&\n+\ttest_path_is_missing nonexisting\n+'\n+\n test_expect_success 'using \"where = before\"' '\n \tgit config trailer.bug.where \"before\" &&\n \tcat complex_message_body >expected &&\ndiff --git a/trailer.c b/trailer.c\nindex 6f3416febaba..e05fa0e9ce93 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -108,23 +108,23 @@ static char last_non_space_char(const char *s)\n \treturn '\\0';\n }\n \n-static void print_tok_val(const char *tok, const char *val)\n+static void print_tok_val(FILE *fp, const char *tok, const char *val)\n {\n \tchar c = last_non_space_char(tok);\n \tif (!c)\n \t\treturn;\n \tif (strchr(separators, c))\n-\t\tprintf(\"%s%s\\n\", tok, val);\n+\t\tfprintf(fp, \"%s%s\\n\", tok, val);\n \telse\n-\t\tprintf(\"%s%c %s\\n\", tok, separators[0], val);\n+\t\tfprintf(fp, \"%s%c %s\\n\", tok, separators[0], val);\n }\n \n-static void print_all(struct trailer_item *first, int trim_empty)\n+static void print_all(FILE *fp, struct trailer_item *first, int trim_empty)\n {\n \tstruct trailer_item *item;\n \tfor (item = first; item; item = item->next) {\n \t\tif (!trim_empty || strlen(item->value) > 0)\n-\t\t\tprint_tok_val(item->token, item->value);\n+\t\t\tprint_tok_val(fp, item->token, item->value);\n \t}\n }\n \n@@ -795,14 +795,15 @@ static int has_blank_line_before(struct strbuf **lines, int start)\n \treturn 0;\n }\n \n-static void print_lines(struct strbuf **lines, int start, int end)\n+static void print_lines(FILE *fp, struct strbuf **lines, int start, int end)\n {\n \tint i;\n \tfor (i = start; lines[i] && i < end; i++)\n-\t\tprintf(\"%s\", lines[i]->buf);\n+\t\tfprintf(fp, \"%s\", lines[i]->buf);\n }\n \n-static int process_input_file(struct strbuf **lines,\n+static int process_input_file(FILE *fp,\n+\t\t\t      struct strbuf **lines,\n \t\t\t      struct trailer_item **in_tok_first,\n \t\t\t      struct trailer_item **in_tok_last)\n {\n@@ -818,10 +819,10 @@ static int process_input_file(struct strbuf **lines,\n \ttrailer_start = find_trailer_start(lines, trailer_end);\n \n \t/* Print lines before the trailers as is */\n-\tprint_lines(lines, 0, trailer_start);\n+\tprint_lines(fp, lines, 0, trailer_start);\n \n \tif (!has_blank_line_before(lines, trailer_start - 1))\n-\t\tprintf(\"\\n\");\n+\t\tfprintf(fp, \"\\n\");\n \n \t/* Parse trailer lines */\n \tfor (i = trailer_start; i < trailer_end; i++) {\n@@ -842,13 +843,14 @@ static void free_all(struct trailer_item **first)\n \t}\n }\n \n-void process_trailers(const char *file, int trim_empty, struct string_list *trailers)\n+void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)\n {\n \tstruct trailer_item *in_tok_first = NULL;\n \tstruct trailer_item *in_tok_last = NULL;\n \tstruct trailer_item *arg_tok_first;\n \tstruct strbuf **lines;\n \tint trailer_end;\n+\tFILE *fp = stdout;\n \n \t/* Default config must be setup first */\n \tgit_config(git_trailer_default_config, NULL);\n@@ -856,19 +858,28 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n \n \tlines = read_input_file(file);\n \n+\tif (in_place) {\n+\t\tfp = fopen(file, \"w\");\n+\t\tif (!fp)\n+\t\t\tdie_errno(_(\"could not open file '%s' for writing\"), file);\n+\t}\n+\n \t/* Print the lines before the trailers */\n-\ttrailer_end = process_input_file(lines, &in_tok_first, &in_tok_last);\n+\ttrailer_end = process_input_file(fp, lines, &in_tok_first, &in_tok_last);\n \n \targ_tok_first = process_command_line_args(trailers);\n \n \tprocess_trailers_lists(&in_tok_first, &in_tok_last, &arg_tok_first);\n \n-\tprint_all(in_tok_first, trim_empty);\n+\tprint_all(fp, in_tok_first, trim_empty);\n \n \tfree_all(&in_tok_first);\n \n \t/* Print the lines after the trailers as is */\n-\tprint_lines(lines, trailer_end, INT_MAX);\n+\tprint_lines(fp, lines, trailer_end, INT_MAX);\n+\n+\tif (in_place)\n+\t\tfclose(fp);\n \n \tstrbuf_list_free(lines);\n }\ndiff --git a/trailer.h b/trailer.h\nindex 8eb25d565e28..36b40b81761f 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -1,6 +1,7 @@\n #ifndef TRAILER_H\n #define TRAILER_H\n \n-void process_trailers(const char *file, int trim_empty, struct string_list *trailers);\n+void process_trailers(const char *file, int in_place, int trim_empty,\n+\t\t      struct string_list *trailers);\n \n #endif /* TRAILER_H */\n-- \n2.6.3.368.gf34be46.dirty\n"},{"id":"275442","messageId":"vpqoacylri6.fsf@anie.imag.fr","threadId":"41136","inReplyTo":"1452087258-3893-1-git-send-email-tobias.klauser@zhinst.com","subject":"Re: [PATCH] interpret-trailers: add option for in-place editing","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-01-06T14:19:45Z","receivedAt":"2016-01-06T14:19:45Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tobias Klauser <tobias.klauser@zhinst.com> writes:\n\n> From: Tobias Klauser <tklauser@distanz.ch>\n>\n> Add a command line option --in-place to support in-place editing akin to\n> sed -i.  This allows to write commands like the following:\n\nSince -i is a common shortcut for --in-place (perl -i, sed -i), it\nprobably makes sense to have it here too. OTOH, this is meant for\nscripting and perhaps it's best to force script writters to be verbose.\n\n> Also add the corresponding documentation and tests.\n\nThis sentence does not harm, but I personnally don't think it's really\nhelpfull, as it's already clear by the diffstat right below, and the\npatch itself.\n\n> -static void print_tok_val(const char *tok, const char *val)\n> +static void print_tok_val(FILE *fp, const char *tok, const char *val)\n>  {\n>  \tchar c = last_non_space_char(tok);\n>  \tif (!c)\n>  \t\treturn;\n>  \tif (strchr(separators, c))\n> -\t\tprintf(\"%s%s\\n\", tok, val);\n> +\t\tfprintf(fp, \"%s%s\\n\", tok, val);\n>  \telse\n> -\t\tprintf(\"%s%c %s\\n\", tok, separators[0], val);\n> +\t\tfprintf(fp, \"%s%c %s\\n\", tok, separators[0], val);\n>  }\n\nThe patch would be even easier to read if split into a preparatory\nrefactoring patch \"turn printf into fprintf\" and then the actual one.\nBut it's already rather clear, so you can probably leave it as-is.\n\n> -static void print_lines(struct strbuf **lines, int start, int end)\n> +static void print_lines(FILE *fp, struct strbuf **lines, int start, int end)\n\nHere and below: it would probably be more readable with a more explicit\nname for fp, like \"outfile\". Especially here:\n\n> -static int process_input_file(struct strbuf **lines,\n> +static int process_input_file(FILE *fp,\n> +\t\t\t      struct strbuf **lines,\n\nWhere it's tempting to think that a parameter given to\nprocess_input_file is ... the input file!\n\nOther than these minor details, the patch looks good to me. Thanks for\nthe patch!\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"275443","messageId":"20160106143635.GI29125@distanz.ch","threadId":"41136","inReplyTo":"vpqoacylri6.fsf@anie.imag.fr","subject":"Re: [PATCH] interpret-trailers: add option for in-place editing","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-01-06T14:36:36Z","receivedAt":"2016-01-06T14:36:36Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"Thanks for your feedback Matthieu!\n\nOn 2016-01-06 at 15:19:45 +0100, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:\n> Tobias Klauser <tobias.klauser@zhinst.com> writes:\n> \n> > From: Tobias Klauser <tklauser@distanz.ch>\n> >\n> > Add a command line option --in-place to support in-place editing akin to\n> > sed -i.  This allows to write commands like the following:\n> \n> Since -i is a common shortcut for --in-place (perl -i, sed -i), it\n> probably makes sense to have it here too. OTOH, this is meant for\n> scripting and perhaps it's best to force script writters to be verbose.\n\nYes, I thought this would mainly be used in scripts and thus omitted the\nshort option.\n\n> > Also add the corresponding documentation and tests.\n> \n> This sentence does not harm, but I personnally don't think it's really\n> helpfull, as it's already clear by the diffstat right below, and the\n> patch itself.\n\nOk, I can omit it for v2.\n\n> > -static void print_tok_val(const char *tok, const char *val)\n> > +static void print_tok_val(FILE *fp, const char *tok, const char *val)\n> >  {\n> >  \tchar c = last_non_space_char(tok);\n> >  \tif (!c)\n> >  \t\treturn;\n> >  \tif (strchr(separators, c))\n> > -\t\tprintf(\"%s%s\\n\", tok, val);\n> > +\t\tfprintf(fp, \"%s%s\\n\", tok, val);\n> >  \telse\n> > -\t\tprintf(\"%s%c %s\\n\", tok, separators[0], val);\n> > +\t\tfprintf(fp, \"%s%c %s\\n\", tok, separators[0], val);\n> >  }\n> \n> The patch would be even easier to read if split into a preparatory\n> refactoring patch \"turn printf into fprintf\" and then the actual one.\n> But it's already rather clear, so you can probably leave it as-is.\n\nOk. I have also no problem with splitting it. I'll wait for a feedback\nfrom Junio on what he prefers.\n\n> > -static void print_lines(struct strbuf **lines, int start, int end)\n> > +static void print_lines(FILE *fp, struct strbuf **lines, int start, int end)\n> \n> Here and below: it would probably be more readable with a more explicit\n> name for fp, like \"outfile\". Especially here:\n> \n> > -static int process_input_file(struct strbuf **lines,\n> > +static int process_input_file(FILE *fp,\n> > +\t\t\t      struct strbuf **lines,\n> \n> Where it's tempting to think that a parameter given to\n> process_input_file is ... the input file!\n\nYes, makes sense. I'll change it to a more concise and readable name\nI'd also take \"outfile\" as you suggest, unless anyone objects.\n\nThanks\nTobias\n"},{"id":"275448","messageId":"CAPig+cSvD-TAd6365wxycUcTBehL-81a0WWosz4mVr1-HKCqGQ@mail.gmail.com","threadId":"41136","inReplyTo":"1452087258-3893-1-git-send-email-tobias.klauser@zhinst.com","subject":"Re: [PATCH] interpret-trailers: add option for in-place editing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-06T19:02:23Z","receivedAt":"2016-01-06T19:02:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 6, 2016 at 8:34 AM, Tobias Klauser\n<tobias.klauser@zhinst.com> wrote:\n> Add a command line option --in-place to support in-place editing akin to\n> sed -i.  This allows to write commands like the following:\n>\n>   git interpret-trailers --trailer \"X: Y\" a.txt > b.txt && mv b.txt a.txt\n>\n> in a more concise way:\n>\n>   git interpret-trailers --trailer \"X: Y\" --in-place a.txt\n>\n> Also add the corresponding documentation and tests.\n\nIn addition to Matthieu's comments...\n\n> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>\n> ---\n> diff --git a/trailer.c b/trailer.c\n> @@ -856,19 +858,28 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n>\n>         lines = read_input_file(file);\n>\n> +       if (in_place) {\n> +               fp = fopen(file, \"w\");\n> +               if (!fp)\n> +                       die_errno(_(\"could not open file '%s' for writing\"), file);\n> +       }\n\nThe input file should be considered precious, but this implementation\nplays too loosely with it. If the user interrupts the program or a\ndie() somewhere within the \"trailers\" code aborts the program before\nthe output file is written, then the original file will be\nirrecoverably lost. Users won't be happy about that.\n\nInstead, this code should go through the standard dance of writing the\noutput to a new file (with some unique temporary name) and then, only\nonce the output has been successfully written in full, rename the new\nfile atop the old.\n\n>         /* Print the lines before the trailers */\n> -       trailer_end = process_input_file(lines, &in_tok_first, &in_tok_last);\n> +       trailer_end = process_input_file(fp, lines, &in_tok_first, &in_tok_last);\n>\n>         arg_tok_first = process_command_line_args(trailers);\n>\n>         process_trailers_lists(&in_tok_first, &in_tok_last, &arg_tok_first);\n>\n> -       print_all(in_tok_first, trim_empty);\n> +       print_all(fp, in_tok_first, trim_empty);\n>\n>         free_all(&in_tok_first);\n>\n>         /* Print the lines after the trailers as is */\n> -       print_lines(lines, trailer_end, INT_MAX);\n> +       print_lines(fp, lines, trailer_end, INT_MAX);\n> +\n> +       if (in_place)\n> +               fclose(fp);\n>\n>         strbuf_list_free(lines);\n>  }\n"},{"id":"275474","messageId":"20160107124216.GK29125@distanz.ch","threadId":"41136","inReplyTo":"CAPig+cSvD-TAd6365wxycUcTBehL-81a0WWosz4mVr1-HKCqGQ@mail.gmail.com","subject":"Re: [PATCH] interpret-trailers: add option for in-place editing","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-01-07T12:42:17Z","receivedAt":"2016-01-07T12:42:17Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"On 2016-01-06 at 20:02:23 +0100, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Jan 6, 2016 at 8:34 AM, Tobias Klauser\n> <tobias.klauser@zhinst.com> wrote:\n> > Add a command line option --in-place to support in-place editing akin to\n> > sed -i.  This allows to write commands like the following:\n> >\n> >   git interpret-trailers --trailer \"X: Y\" a.txt > b.txt && mv b.txt a.txt\n> >\n> > in a more concise way:\n> >\n> >   git interpret-trailers --trailer \"X: Y\" --in-place a.txt\n> >\n> > Also add the corresponding documentation and tests.\n> \n> In addition to Matthieu's comments...\n> \n> > Signed-off-by: Tobias Klauser <tklauser@distanz.ch>\n> > ---\n> > diff --git a/trailer.c b/trailer.c\n> > @@ -856,19 +858,28 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n> >\n> >         lines = read_input_file(file);\n> >\n> > +       if (in_place) {\n> > +               fp = fopen(file, \"w\");\n> > +               if (!fp)\n> > +                       die_errno(_(\"could not open file '%s' for writing\"), file);\n> > +       }\n> \n> The input file should be considered precious, but this implementation\n> plays too loosely with it. If the user interrupts the program or a\n> die() somewhere within the \"trailers\" code aborts the program before\n> the output file is written, then the original file will be\n> irrecoverably lost. Users won't be happy about that.\n\nIndeed, I didn't consider this. Thanks a lot for pointing this out.\n\n> Instead, this code should go through the standard dance of writing the\n> output to a new file (with some unique temporary name) and then, only\n> once the output has been successfully written in full, rename the new\n> file atop the old.\n\nOk, will do this for v2. I guess with the help of the functions from\ntempfile.h it should be fairly easy to implement...\n\nThanks for your review!\n"}]}