{"thread":{"id":"41179","subject":"[PATCH v3 1/2] trailer: use fprintf instead of printf","startedAt":"2016-01-13T12:43:07Z","lastAt":"2016-01-14T09:33:45Z","messageCount":8,"participants":["Tobias Klauser","Matthieu Moy","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"275899","messageId":"1452688989-13746-1-git-send-email-tklauser@distanz.ch","threadId":"41179","inReplyTo":null,"subject":"[PATCH v3 0/2] Add in-place editing support to git interpret-trailers","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-01-13T12:43:07Z","receivedAt":"2016-01-13T12:43:07Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"This patch series adds support for in-place editing to git interpret-trailers\nakin to sed -i, perl -i.\n\nv2->v3:\n - Rephrase two error messages according to the suggestions by Matthieu Moy.\n\nv1->v2:\n - Split patch to make review easier, as suggested by Matthieu Moy.\n - Rename FILE * function parameters to a more readable name, as suggested by\n Matthieu Moy.\n - Write output to temporary file and rename after successfully written in full\n to avoid losing the original file in case of an error/interrupt. Pointed out\n by Eric Sunshine.\n\nTobias Klauser (2):\n trailer: use fprintf instead of printf\n interpret-trailers: add option for in-place editing\n\n Documentation/git-interpret-trailers.txt | 24 +++++++++++-\n builtin/interpret-trailers.c | 13 +++++--\n t/t7513-interpret-trailers.sh | 32 ++++++++++++++++\n trailer.c | 63 +++++++++++++++++++++++++-------\n trailer.h | 3 +-\n 5 files changed, 115 insertions(+), 20 deletions(-)\n\n-- \n2.7.0.1.g5e091f5\n"},{"id":"275898","messageId":"1452688989-13746-2-git-send-email-tklauser@distanz.ch","threadId":"41179","inReplyTo":"1452688989-13746-1-git-send-email-tklauser@distanz.ch","subject":"[PATCH v3 1/2] trailer: use fprintf instead of printf","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-01-13T12:43:08Z","receivedAt":"2016-01-13T12:43:08Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"Use fprintf instead of printf in trailer.c in order to allow printing\nto a file other than stdout. This will be needed to support in-place\nediting in git interpret-trailers.\n\nSigned-off-by: Tobias Klauser \n---\n trailer.c | 28 +++++++++++++++-------------\n 1 file changed, 15 insertions(+), 13 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 6f3416febaba..176fac213450 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 *outfile, 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(outfile, \"%s%s\\n\", tok, val);\n \telse\n-\t\tprintf(\"%s%c %s\\n\", tok, separators[0], val);\n+\t\tfprintf(outfile, \"%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 *outfile, 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(outfile, 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 *outfile, 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(outfile, \"%s\", lines[i]->buf);\n }\n \n-static int process_input_file(struct strbuf **lines,\n+static int process_input_file(FILE *outfile,\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(outfile, lines, 0, trailer_start);\n \n \tif (!has_blank_line_before(lines, trailer_start - 1))\n-\t\tprintf(\"\\n\");\n+\t\tfprintf(outfile, \"\\n\");\n \n \t/* Parse trailer lines */\n \tfor (i = trailer_start; i < trailer_end; i++) {\n@@ -849,6 +850,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n \tstruct trailer_item *arg_tok_first;\n \tstruct strbuf **lines;\n \tint trailer_end;\n+\tFILE *outfile = stdout;\n \n \t/* Default config must be setup first */\n \tgit_config(git_trailer_default_config, NULL);\n@@ -857,18 +859,18 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n \tlines = read_input_file(file);\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(outfile, 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(outfile, 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(outfile, lines, trailer_end, INT_MAX);\n \n \tstrbuf_list_free(lines);\n }\n-- \n2.7.0.1.g5e091f5\n"},{"id":"275900","messageId":"1452688989-13746-3-git-send-email-tklauser@distanz.ch","threadId":"41179","inReplyTo":"1452688989-13746-1-git-send-email-tklauser@distanz.ch","subject":"[PATCH v3 2/2] interpret-trailers: add option for in-place editing","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-01-13T12:43:09Z","receivedAt":"2016-01-13T12:43:09Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"Add 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\nSigned-off-by: Tobias Klauser \n---\n Documentation/git-interpret-trailers.txt | 24 +++++++++++++++++++++-\n builtin/interpret-trailers.c | 13 ++++++++----\n t/t7513-interpret-trailers.sh | 32 +++++++++++++++++++++++++++++\n trailer.c | 35 +++++++++++++++++++++++++++++++-\n trailer.h | 3 ++-\n 5 files changed, 100 insertions(+), 7 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 [(=|:)])...] [...]\n+'git interpret-trailers' [--in-place] [--trim-empty] [(--trailer [(=|:)])...] [...]\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 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 \n Signed-off-by: Bob \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 \n+$ git interpret-trailers --trailer 'Acked-by: Alice ' --in-place msg.txt\n+$ cat msg.txt\n+subject\n+\n+message\n+\n+Signed-off-by: Bob \n+Acked-by: Alice \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 [(=|:)])...] [...]\"),\n+\tN_(\"git interpret-trailers [--in-place] [--trim-empty] [(--trailer [(=|:)])...] [...]\"),\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 176fac213450..6834d1f8086a 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -2,6 +2,7 @@\n #include \"string-list.h\"\n #include \"run-command.h\"\n #include \"commit.h\"\n+#include \"tempfile.h\"\n #include \"trailer.h\"\n /*\n * Copyright (c) 2013, 2014 Christian Couder \n@@ -843,7 +844,9 @@ 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+static struct tempfile trailers_tempfile;\n+\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@@ -858,6 +861,31 @@ 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\tstruct stat st;\n+\t\tstruct strbuf template = STRBUF_INIT;\n+\t\tconst char *tail;\n+\n+\t\tif (stat(file, &st))\n+\t\t\tdie_errno(_(\"could not stat %s\"), file);\n+\t\tif (!S_ISREG(st.st_mode))\n+\t\t\tdie(_(\"file %s is not a regular file\"), file);\n+\t\tif (!(st.st_mode & S_IWUSR))\n+\t\t\tdie(_(\"file %s is not writable by user\"), file);\n+\n+\t\t/* Create temporary file in the same directory as the original */\n+\t\ttail = strrchr(file, '/');\n+\t\tif (tail != NULL)\n+\t\t\tstrbuf_add(&template, file, tail - file + 1);\n+\t\tstrbuf_addstr(&template, \"git-interpret-trailers-XXXXXX\");\n+\n+\t\txmks_tempfile_m(&trailers_tempfile, template.buf, st.st_mode);\n+\t\tstrbuf_release(&template);\n+\t\toutfile = fdopen_tempfile(&trailers_tempfile, \"w\");\n+\t\tif (!outfile)\n+\t\t\tdie_errno(_(\"could not open temporary file\"));\n+\t}\n+\n \t/* Print the lines before the trailers */\n \ttrailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);\n \n@@ -872,5 +900,10 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n \t/* Print the lines after the trailers as is */\n \tprint_lines(outfile, lines, trailer_end, INT_MAX);\n \n+\tif (in_place) {\n+\t\tif (rename_tempfile(&trailers_tempfile, file))\n+\t\t\tdie_errno(_(\"could not rename temporary file to %s\"), file);\n+\t}\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.7.0.1.g5e091f5\n"},{"id":"275901","messageId":"vpqmvs9mywq.fsf@anie.imag.fr","threadId":"41179","inReplyTo":"1452688989-13746-1-git-send-email-tklauser@distanz.ch","subject":"Re: [PATCH v3 0/2] Add in-place editing support to git interpret-trailers","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-01-13T12:44:53Z","receivedAt":"2016-01-13T12:44:53Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tobias Klauser writes:\n\n> This patch series adds support for in-place editing to git interpret-trailers\n> akin to sed -i, perl -i.\n>\n> v2->v3:\n> - Rephrase two error messages according to the suggestions by Matthieu Moy.\n\nPerfect, thanks.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"275948","messageId":"CAPig+cSX04OSLQVk-LHm2UQOAqThQUJuFhuhUfoNmUvHmZpPsA@mail.gmail.com","threadId":"41179","inReplyTo":"1452688989-13746-3-git-send-email-tklauser@distanz.ch","subject":"Re: [PATCH v3 2/2] interpret-trailers: add option for in-place editing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-13T18:15:54Z","receivedAt":"2016-01-13T18:15:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 13, 2016 at 7:43 AM, Tobias Klauser 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> Signed-off-by: Tobias Klauser \n> ---\n> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\n> @@ -326,6 +326,38 @@ test_expect_success 'with complex patch, args and --trim-empty' '\n> +test_expect_success 'in-place editing on stdin' '\n\nMaybe say:\n\n \"in-place editing on stdin disallowed\"\n\nor something?\n\n> + test_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> + test_must_fail git interpret-trailers --trailer \"Reviewed-by: Alice\" --in-place nonexisting &&\n> + test_path_is_missing nonexisting\n> +'\n\nAn significant shortcoming of the first version of this patch series\nwas that it did not treat the input file as precious, and would\nhappily delete it if trailer processing failed for any reason. This is\nbehavior we'd like to protect against. Toward that end, have you\nconsidered adding a test to verify that the input file is indeed\nconsidered precious, and not deleted upon failure? One way to do so\nwould be to write a test which triggers one of the failure conditions\nof the interpret-trailers framework. Looking at the source code, one\npossible way would be to make trailer.c:read_input_file() fail, for\ninstance, by making the file unreadable (with 'chmod -r', though you'd\nhave to protect the test with the POSIXPERM prerequisite).\n\nMore below.\n\n> diff --git a/trailer.c b/trailer.c\n> @@ -858,6 +861,31 @@ 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> + struct stat st;\n> + struct strbuf template = STRBUF_INIT;\n> + const char *tail;\n> +\n> + if (stat(file, &st))\n> + die_errno(_(\"could not stat %s\"), file);\n> + if (!S_ISREG(st.st_mode))\n> + die(_(\"file %s is not a regular file\"), file);\n> + if (!(st.st_mode & S_IWUSR))\n> + die(_(\"file %s is not writable by user\"), file);\n> +\n> + /* Create temporary file in the same directory as the original */\n> + tail = strrchr(file, '/');\n> + if (tail != NULL)\n> + strbuf_add(&template, file, tail - file + 1);\n> + strbuf_addstr(&template, \"git-interpret-trailers-XXXXXX\");\n> +\n> + xmks_tempfile_m(&trailers_tempfile, template.buf, st.st_mode);\n> + strbuf_release(&template);\n> + outfile = fdopen_tempfile(&trailers_tempfile, \"w\");\n> + if (!outfile)\n> + die_errno(_(\"could not open temporary file\"));\n> + }\n\nHmm, the current logic of process_trailers() is pretty easily\nunderstood at a glance, but this new (relatively huge) chunk of code\nobscures the overall operation. Have you considered factoring the new\ncode out into its own function in order to keep the overall flow of\nprocess_trailers() clean? (Genuine question; I don't necessarily feel\nso strongly about it to demand such a change.)\n\n> /* Print the lines before the trailers */\n> trailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);\n>\n> @@ -872,5 +900,10 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n> /* Print the lines after the trailers as is */\n> print_lines(outfile, lines, trailer_end, INT_MAX);\n>\n> + if (in_place) {\n> + if (rename_tempfile(&trailers_tempfile, file))\n> + die_errno(_(\"could not rename temporary file to %s\"), file);\n> + }\n\nYou could drop the unnecessary braces.\n\n> strbuf_list_free(lines);\n> }\n"},{"id":"275961","messageId":"xmqqd1t5nv6j.fsf@gitster.mtv.corp.google.com","threadId":"41179","inReplyTo":"1452688989-13746-2-git-send-email-tklauser@distanz.ch","subject":"Re: [PATCH v3 1/2] trailer: use fprintf instead of printf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-13T19:20:04Z","receivedAt":"2016-01-13T19:20:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tobias Klauser writes:\n\n> Use fprintf instead of printf in trailer.c in order to allow printing\n> to a file other than stdout. This will be needed to support in-place\n> editing in git interpret-trailers.\n>\n> Signed-off-by: Tobias Klauser \n\nThanks. I won't bother to amend it myself or request you to reroll\nonly for this, but that is a rather suboptimal title.\n\nThe distinction between fprintf/printf is an implementation detail;\nwhat you want to do in this change is to allow writing to a file\nother than the standard output, and that should be in the title.\n"},{"id":"276012","messageId":"20160114093157.GF26950@distanz.ch","threadId":"41179","inReplyTo":"CAPig+cSX04OSLQVk-LHm2UQOAqThQUJuFhuhUfoNmUvHmZpPsA@mail.gmail.com","subject":"Re: [PATCH v3 2/2] interpret-trailers: add option for in-place editing","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-01-14T09:31:57Z","receivedAt":"2016-01-14T09:31:57Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"On 2016-01-13 at 19:15:54 +0100, Eric Sunshine wrote:\n> On Wed, Jan 13, 2016 at 7:43 AM, Tobias Klauser 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> > Signed-off-by: Tobias Klauser \n> > ---\n> > diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\n> > @@ -326,6 +326,38 @@ test_expect_success 'with complex patch, args and --trim-empty' '\n> > +test_expect_success 'in-place editing on stdin' '\n> \n> Maybe say:\n> \n> \"in-place editing on stdin disallowed\"\n> \n> or something?\n\nAgree, will change in v4.\n\n> > + test_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> > + test_must_fail git interpret-trailers --trailer \"Reviewed-by: Alice\" --in-place nonexisting &&\n> > + test_path_is_missing nonexisting\n> > +'\n> \n> An significant shortcoming of the first version of this patch series\n> was that it did not treat the input file as precious, and would\n> happily delete it if trailer processing failed for any reason. This is\n> behavior we'd like to protect against. Toward that end, have you\n> considered adding a test to verify that the input file is indeed\n> considered precious, and not deleted upon failure? One way to do so\n> would be to write a test which triggers one of the failure conditions\n> of the interpret-trailers framework. Looking at the source code, one\n> possible way would be to make trailer.c:read_input_file() fail, for\n> instance, by making the file unreadable (with 'chmod -r', though you'd\n> have to protect the test with the POSIXPERM prerequisite).\n\nGood point. Yes, such a test should definitely be added, I forgot about\nit during the reroll. I'll look into your suggestion and add a test in\nv4.\n\n> More below.\n> \n> > diff --git a/trailer.c b/trailer.c\n> > @@ -858,6 +861,31 @@ 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> > + struct stat st;\n> > + struct strbuf template = STRBUF_INIT;\n> > + const char *tail;\n> > +\n> > + if (stat(file, &st))\n> > + die_errno(_(\"could not stat %s\"), file);\n> > + if (!S_ISREG(st.st_mode))\n> > + die(_(\"file %s is not a regular file\"), file);\n> > + if (!(st.st_mode & S_IWUSR))\n> > + die(_(\"file %s is not writable by user\"), file);\n> > +\n> > + /* Create temporary file in the same directory as the original */\n> > + tail = strrchr(file, '/');\n> > + if (tail != NULL)\n> > + strbuf_add(&template, file, tail - file + 1);\n> > + strbuf_addstr(&template, \"git-interpret-trailers-XXXXXX\");\n> > +\n> > + xmks_tempfile_m(&trailers_tempfile, template.buf, st.st_mode);\n> > + strbuf_release(&template);\n> > + outfile = fdopen_tempfile(&trailers_tempfile, \"w\");\n> > + if (!outfile)\n> > + die_errno(_(\"could not open temporary file\"));\n> > + }\n> \n> Hmm, the current logic of process_trailers() is pretty easily\n> understood at a glance, but this new (relatively huge) chunk of code\n> obscures the overall operation. Have you considered factoring the new\n> code out into its own function in order to keep the overall flow of\n> process_trailers() clean? (Genuine question; I don't necessarily feel\n> so strongly about it to demand such a change.)\n\nNo, I haven't considered this yet. But I agree that moving this code to\na separate function certainly will keep process_trailers() much more\nreadable. Also it would simplify future reusability if anyone else would\nneed similar functionality.\n\n> > /* Print the lines before the trailers */\n> > trailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);\n> >\n> > @@ -872,5 +900,10 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai\n> > /* Print the lines after the trailers as is */\n> > print_lines(outfile, lines, trailer_end, INT_MAX);\n> >\n> > + if (in_place) {\n> > + if (rename_tempfile(&trailers_tempfile, file))\n> > + die_errno(_(\"could not rename temporary file to %s\"), file);\n> > + }\n> \n> You could drop the unnecessary braces.\n\nOk, I'll drop the braces for v4.\n\nThanks a lot for your review!\n"},{"id":"276013","messageId":"20160114093345.GG26950@distanz.ch","threadId":"41179","inReplyTo":"xmqqd1t5nv6j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/2] trailer: use fprintf instead of printf","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-01-14T09:33:45Z","receivedAt":"2016-01-14T09:33:45Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"On 2016-01-13 at 20:20:04 +0100, Junio C Hamano wrote:\n> Tobias Klauser writes:\n> \n> > Use fprintf instead of printf in trailer.c in order to allow printing\n> > to a file other than stdout. This will be needed to support in-place\n> > editing in git interpret-trailers.\n> >\n> > Signed-off-by: Tobias Klauser \n> \n> Thanks. I won't bother to amend it myself or request you to reroll\n> only for this, but that is a rather suboptimal title.\n\nAgree.\n\n> The distinction between fprintf/printf is an implementation detail;\n> what you want to do in this change is to allow writing to a file\n> other than the standard output, and that should be in the title.\n\nSince I'll do a v4 anyhow to address Eric's review comments, I can\nchange the title in the reroll.\n\nThanks\n"}]}