{"thread":{"id":"41192","subject":"[PATCH v4 2/2] interpret-trailers: add option for in-place editing","startedAt":"2016-01-14T16:57:53Z","lastAt":"2016-01-20T00:20:28Z","messageCount":17,"participants":["Tobias Klauser","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"276054","messageId":"1452790676-11937-1-git-send-email-tklauser@distanz.ch","threadId":"41192","inReplyTo":null,"subject":"[PATCH v4 0/2] Add in-place editing support to git interpret-trailers","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-01-14T16:57:53Z","receivedAt":"2016-01-14T16:57:53Z","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\ninterpret-trailers akin to sed -i, perl -i.\n\nv3->v4:\n - Reword a test title, as suggested by Eric Sunshine.\n - Add a test to verify that the original file is not clobbered/deleted\n on error, as suggested by Eric Sunshine.\n - Move code specific to in-place editing from process_trailers() into a\n separate function to keep the overall flow clean. Suggested by Eric\n Sunshine.\n - Drop unnecessary braces, as pointed out by Eric Sunshine.\n - Use a more meaningful title for patch 1/2. Suggested by Junio Hamano.\n\nv2->v3:\n - Rephrase two error messages according to the suggestions by Matthieu\n 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\n suggested by Matthieu Moy.\n - Write output to temporary file and rename after successfully written\n in full to avoid losing the original file in case of an\n error/interrupt. Pointed out by Eric Sunshine.\n\nTobias Klauser (2):\n trailer: allow to write to files other than stdout\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 | 40 ++++++++++++++++++\n trailer.c | 69 +++++++++++++++++++++++++-------\n trailer.h | 3 +-\n 5 files changed, 129 insertions(+), 20 deletions(-)\n\n-- \n2.7.0.1.g5e091f5\n"},{"id":"276055","messageId":"1452790676-11937-2-git-send-email-tklauser@distanz.ch","threadId":"41192","inReplyTo":"1452790676-11937-1-git-send-email-tklauser@distanz.ch","subject":"[PATCH v4 1/2] trailer: allow to write to files other than stdout","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-01-14T16:57:54Z","receivedAt":"2016-01-14T16:57:54Z","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":"276053","messageId":"1452790676-11937-3-git-send-email-tklauser@distanz.ch","threadId":"41192","inReplyTo":"1452790676-11937-1-git-send-email-tklauser@distanz.ch","subject":"[PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-01-14T16:57:55Z","receivedAt":"2016-01-14T16:57:55Z","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 | 40 +++++++++++++++++++++++++++++++\n trailer.c | 41 +++++++++++++++++++++++++++++++-\n trailer.h | 3 ++-\n 5 files changed, 114 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..aee785cffa8d 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -326,6 +326,46 @@ 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 disallowed' '\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 POSIXPERM,SANITY \"in-place editing doesn't clobber original file on error\" '\n+\tcat basic_message >message &&\n+\tchmod -r message &&\n+\ttest_must_fail git interpret-trailers --trailer \"Reviewed-by: Alice\" --in-place message &&\n+\tchmod +r message &&\n+\ttest_cmp message basic_message\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..94b387b49971 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,38 @@ 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+static FILE *create_in_place_tempfile(const char *file)\n+{\n+\tstruct stat st;\n+\tstruct strbuf template = STRBUF_INIT;\n+\tconst char *tail;\n+\tFILE *outfile;\n+\n+\tif (stat(file, &st))\n+\t\tdie_errno(_(\"could not stat %s\"), file);\n+\tif (!S_ISREG(st.st_mode))\n+\t\tdie(_(\"file %s is not a regular file\"), file);\n+\tif (!(st.st_mode & S_IWUSR))\n+\t\tdie(_(\"file %s is not writable by user\"), file);\n+\n+\t/* Create temporary file in the same directory as the original */\n+\ttail = strrchr(file, '/');\n+\tif (tail != NULL)\n+\t\tstrbuf_add(&template, file, tail - file + 1);\n+\tstrbuf_addstr(&template, \"git-interpret-trailers-XXXXXX\");\n+\n+\txmks_tempfile_m(&trailers_tempfile, template.buf, st.st_mode);\n+\tstrbuf_release(&template);\n+\toutfile = fdopen_tempfile(&trailers_tempfile, \"w\");\n+\tif (!outfile)\n+\t\tdie_errno(_(\"could not open temporary file\"));\n+\n+\treturn outfile;\n+}\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 +890,9 @@ 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\toutfile = create_in_place_tempfile(file);\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 +907,9 @@ 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+\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":"276076","messageId":"xmqqio2vki0i.fsf@gitster.mtv.corp.google.com","threadId":"41192","inReplyTo":"1452790676-11937-3-git-send-email-tklauser@distanz.ch","subject":"Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-14T20:45:01Z","receivedAt":"2016-01-14T20:45:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tobias Klauser writes:\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> 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\nThanks, will replace. I found some micronits, none of which I think\nis big enough to require another reroll, but since I found them\nalready, I'll just point them out.\n\n> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\n> index 322c436a494c..aee785cffa8d 100755\n> --- a/t/t7513-interpret-trailers.sh\n> +++ b/t/t7513-interpret-trailers.sh\n> @@ -326,6 +326,46 @@ 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\nThe \"echo\" is not needed, if you just include a leading blank line\nin the here-document you use with this \"cat\".\n\n> +test_expect_success POSIXPERM,SANITY \"in-place editing doesn't clobber original file on error\" '\n> +\tcat basic_message >message &&\n> +\tchmod -r message &&\n> +\ttest_must_fail git interpret-trailers --trailer \"Reviewed-by: Alice\" --in-place message &&\n> +\tchmod +r message &&\n> +\ttest_cmp message basic_message\n> +'\n\nIf for some reason interpret-trailers fails to fail, this would\nleave an unreadable 'message' in the trash directory. Maybe no\nother tests that come after this one want to be able to read the\ncontents of the file right now, but this is an accident waiting to\nhappen:\n\n\tcat basic_message >message &&\n+ test_when_finished \"chmod +r message\" &&\n chmod -r message &&\n test_must_fail ... &&\n\tchmod +r message &&\n test_cmp ...\n\n> +\tif (!S_ISREG(st.st_mode))\n> +\t\tdie(_(\"file %s is not a regular file\"), file);\n> +\tif (!(st.st_mode & S_IWUSR))\n> +\t\tdie(_(\"file %s is not writable by user\"), file);\n\nHmph, are these two necessary, and do they make sense?\n\nWhen doing an in-place thing, the primary thing you care about is\nthat you can read from the file and you can deposit the result of\nthe rewrite under the original name. If for some reason a system\nallowed you to read from a non-regular file and interpret-trailers\ncan do a sensible thing to the contents you read from there, do you\nhave to insist that original must be S_ISREG()? Also, a funny file\n(e.g. \"interpret-trailers -i .\") is likely to fail on the input\nside.\n\nFor the latter,\n\n $ chmod a-w COPYING\n $ sed -i -e 's/a/b/' COPYING\n\nseems to succeed _and_ leave the permission bits intact, i.e.\nI get this before and after \"sed -i\"\n\n $ ls -l COPYING\n -r--r----- 1 jch eng 18765 Jan 14 12:34 COPYING\n\nwhich hints at two points:\n\n - The users (of \"sed -i\") may have demanded that in-place update of\n read-only file must be allowed, and there may have been a good\n reason for wanting to do so. That reason may apply equally to us\n here.\n\n - If we were to follow suit, then we should not forget to restore\n the permission bits on the new file.\n\nIn any case, these are something we could loosen after people gain\nexperience with the feature, so I think it is OK as-is, at least for\nnow.\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> +\n\nI briefly wondered if this should be\n\n\tif (in_place && rename_tempfile(...))\n\t\tdie_errno(...);\n\nto save one indentation level, but I think it is a bad idea,\ni.e. the above code should stay as-is.\n"},{"id":"276144","messageId":"20160115103402.GC21205@distanz.ch","threadId":"41192","inReplyTo":"xmqqio2vki0i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-01-15T10:34:02Z","receivedAt":"2016-01-15T10:34:02Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"On 2016-01-14 at 21:45:01 +0100, Junio C Hamano wrote:\n> Tobias Klauser writes:\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> > 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> \n> Thanks, will replace. I found some micronits, none of which I think\n> is big enough to require another reroll, but since I found them\n> already, I'll just point them out.\n\nThanks a lot for your review!\n\n> > diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\n> > index 322c436a494c..aee785cffa8d 100755\n> > --- a/t/t7513-interpret-trailers.sh\n> > +++ b/t/t7513-interpret-trailers.sh\n> > @@ -326,6 +326,46 @@ 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> \n> The \"echo\" is not needed, if you just include a leading blank line\n> in the here-document you use with this \"cat\".\n\nClassical case of copy&paste and me not thinking enough what could be\nsimplified ;)\n\n> > +test_expect_success POSIXPERM,SANITY \"in-place editing doesn't clobber original file on error\" '\n> > +\tcat basic_message >message &&\n> > +\tchmod -r message &&\n> > +\ttest_must_fail git interpret-trailers --trailer \"Reviewed-by: Alice\" --in-place message &&\n> > +\tchmod +r message &&\n> > +\ttest_cmp message basic_message\n> > +'\n> \n> If for some reason interpret-trailers fails to fail, this would\n> leave an unreadable 'message' in the trash directory. Maybe no\n> other tests that come after this one want to be able to read the\n> contents of the file right now, but this is an accident waiting to\n> happen:\n> \n> \tcat basic_message >message &&\n> + test_when_finished \"chmod +r message\" &&\n> chmod -r message &&\n> test_must_fail ... &&\n> \tchmod +r message &&\n> test_cmp ...\n\nIndeed, I forgot about this. I saw you already folded in the missing\n'chmod +r message' in your tree. Thanks for that!\n\n> \n> > +\tif (!S_ISREG(st.st_mode))\n> > +\t\tdie(_(\"file %s is not a regular file\"), file);\n> > +\tif (!(st.st_mode & S_IWUSR))\n> > +\t\tdie(_(\"file %s is not writable by user\"), file);\n> \n> Hmph, are these two necessary, and do they make sense?\n\nThe check for S_ISREG I added because it's also done in sed:\n\n\t$ mkdir /tmp/foobar\n\t$ sed -i 's/foo/baz/' /tmp/foobar\n\tsed: couldn't edit foobar: not a regular file\n\nI quickly checked their source and they do indeed a check for S_ISREG in\ncase of in-place editing (sed v4.2.2-98-g61c0a53ec997,\nsed/execute.c:600)\n\nBut the writable check is probably too strict, I agree.\n\n> When doing an in-place thing, the primary thing you care about is\n> that you can read from the file and you can deposit the result of\n> the rewrite under the original name. If for some reason a system\n> allowed you to read from a non-regular file and interpret-trailers\n> can do a sensible thing to the contents you read from there, do you\n> have to insist that original must be S_ISREG()? Also, a funny file\n> (e.g. \"interpret-trailers -i .\") is likely to fail on the input\n> side.\n> \n> For the latter,\n> \n> $ chmod a-w COPYING\n> $ sed -i -e 's/a/b/' COPYING\n> \n> seems to succeed _and_ leave the permission bits intact, i.e.\n> I get this before and after \"sed -i\"\n> \n> $ ls -l COPYING\n> -r--r----- 1 jch eng 18765 Jan 14 12:34 COPYING\n> \n> which hints at two points:\n> \n> - The users (of \"sed -i\") may have demanded that in-place update of\n> read-only file must be allowed, and there may have been a good\n> reason for wanting to do so. That reason may apply equally to us\n> here.\n\nTrue. AFAIK rename(2) only need write permissions on the containing directory,\nnot the source/destination file itself. And rename_tempfile should barf\nabout that. So the S_IWUSR is unnecessarily strict...\n\n> - If we were to follow suit, then we should not forget to restore\n> the permission bits on the new file.\n\nThis should already be the case to to st.st_mode being passed to\nxmks_tempfile_m, no?\n\n> In any case, these are something we could loosen after people gain\n> experience with the feature, so I think it is OK as-is, at least for\n> now.\n\nI agree.\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> > +\n> \n> I briefly wondered if this should be\n> \n> \tif (in_place && rename_tempfile(...))\n> \t\tdie_errno(...);\n> \n> to save one indentation level, but I think it is a bad idea,\n> i.e. the above code should stay as-is.\n\nThought about this for a while too, but I concluded that it would hurt\nreadability more than it would help.\n"},{"id":"276174","messageId":"xmqqa8o6kb6m.fsf@gitster.mtv.corp.google.com","threadId":"41192","inReplyTo":"20160115103402.GC21205@distanz.ch","subject":"Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-15T17:24:49Z","receivedAt":"2016-01-15T17:24:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tobias Klauser writes:\n\n>> > +test_expect_success POSIXPERM,SANITY \"in-place editing doesn't clobber original file on error\" '\n>> > +\tcat basic_message >message &&\n>> > +\tchmod -r message &&\n>> > +\ttest_must_fail git interpret-trailers --trailer \"Reviewed-by: Alice\" --in-place message &&\n>> > +\tchmod +r message &&\n>> > +\ttest_cmp message basic_message\n>> > +'\n>> \n>> If for some reason interpret-trailers fails to fail, this would\n>> leave an unreadable 'message' in the trash directory. Maybe no\n>> other tests that come after this one want to be able to read the\n>> contents of the file right now, but this is an accident waiting to\n>> happen:\n>> \n>> \tcat basic_message >message &&\n>> + test_when_finished \"chmod +r message\" &&\n>> chmod -r message &&\n>> test_must_fail ... &&\n>> \tchmod +r message &&\n>> test_cmp ...\n>\n> Indeed, I forgot about this. I saw you already folded in the missing\n> 'chmod +r message' in your tree. Thanks for that!\n\nI did no such thing, though.\n"},{"id":"276178","messageId":"20160115174522.GD21205@distanz.ch","threadId":"41192","inReplyTo":"xmqqa8o6kb6m.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-01-15T17:45:23Z","receivedAt":"2016-01-15T17:45:23Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"On 2016-01-15 at 18:24:49 +0100, Junio C Hamano wrote:\n> Tobias Klauser writes:\n> \n> >> > +test_expect_success POSIXPERM,SANITY \"in-place editing doesn't clobber original file on error\" '\n> >> > +\tcat basic_message >message &&\n> >> > +\tchmod -r message &&\n> >> > +\ttest_must_fail git interpret-trailers --trailer \"Reviewed-by: Alice\" --in-place message &&\n> >> > +\tchmod +r message &&\n> >> > +\ttest_cmp message basic_message\n> >> > +'\n> >> \n> >> If for some reason interpret-trailers fails to fail, this would\n> >> leave an unreadable 'message' in the trash directory. Maybe no\n> >> other tests that come after this one want to be able to read the\n> >> contents of the file right now, but this is an accident waiting to\n> >> happen:\n> >> \n> >> \tcat basic_message >message &&\n> >> + test_when_finished \"chmod +r message\" &&\n> >> chmod -r message &&\n> >> test_must_fail ... &&\n> >> \tchmod +r message &&\n> >> test_cmp ...\n> >\n> > Indeed, I forgot about this. I saw you already folded in the missing\n> > 'chmod +r message' in your tree. Thanks for that!\n> \n> I did no such thing, though.\n\nSorry, my misunderstanding. I thought about \"chmod +r\" but of course the\nessential part is the\n\n + test_when_finished \"chmod +r message\" &&\n\nwhich isn't in your tree.\n"},{"id":"276308","messageId":"CAPig+cRRdca7PfkqppY2X7KSFpHX0yH19fxRL+w_=u9vg7NV9A@mail.gmail.com","threadId":"41192","inReplyTo":"xmqqio2vki0i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-18T21:11:11Z","receivedAt":"2016-01-18T21:11:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jan 14, 2016 at 3:45 PM, Junio C Hamano wrote:\n> Tobias Klauser writes:\n>> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\n>> @@ -326,6 +326,46 @@ test_expect_success 'with complex patch, args and --trim-empty' '\n>> +test_expect_success POSIXPERM,SANITY \"in-place editing doesn't clobber original file on error\" '\n\nI think POSIXPERM is all you need for this case; SANITY doesn't buy\nyou anything, if I understand correctly.\n\n>> + cat basic_message >message &&\n>> + chmod -r message &&\n>> + test_must_fail git interpret-trailers --trailer \"Reviewed-by: Alice\" --in-place message &&\n>> + chmod +r message &&\n>> + test_cmp message basic_message\n>> +'\n>\n> If for some reason interpret-trailers fails to fail, this would\n> leave an unreadable 'message' in the trash directory. Maybe no\n> other tests that come after this one want to be able to read the\n> contents of the file right now, but this is an accident waiting to\n> happen:\n>\n> cat basic_message >message &&\n> + test_when_finished \"chmod +r message\" &&\n> chmod -r message &&\n> test_must_fail ... &&\n> chmod +r message &&\n\nDon't forget to remove this (now unnecessary) \"chmod +r\" once you've\nadded the 'test_when_finished \"chmod +r\"'.\n\n> test_cmp ...\n"},{"id":"276318","messageId":"CAPig+cQ5X7r22pXyCs_n+-mXK3Lzh1CpAMQ_PbuhLT4C3S+v1Q@mail.gmail.com","threadId":"41192","inReplyTo":"CAPc5daWpnReWJzeTJjvZap78H0oZKG-YGEP19Neusyahu5A6cQ@mail.gmail.com","subject":"Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-18T22:13:22Z","receivedAt":"2016-01-18T22:13:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jan 18, 2016 at 4:21 PM, Junio C Hamano wrote:\n> On Jan 18, 2016 13:11, \"Eric Sunshine\" wrote:\n>> On Thu, Jan 14, 2016 at 3:45 PM, Junio C Hamano wrote:\n>>> If for some reason interpret-trailers fails to fail, this would\n>>> leave an unreadable 'message' in the trash directory. Maybe no\n>>> other tests that come after this one want to be able to read the\n>>> contents of the file right now, but this is an accident waiting to\n>>> happen:\n>>>\n>>> cat basic_message >message &&\n>>> + test_when_finished \"chmod +r message\" &&\n>>> chmod -r message &&\n>>> test_must_fail ... &&\n>>> chmod +r message &&\n>>\n>> Don't forget to remove this (now unnecessary) \"chmod +r\" once you've\n>> added the 'test_when_finished \"chmod +r\"'.\n>>\n>>> test_cmp ...\n>\n> It still is necessary for the test-cmp to work, no?\n\nMy bad. Ignore me.\n\nBy the way, isn't the:\n\n cat basic_message >message &&\n\nin the above test just an unusual way to say:\n\n cp basic_message message &&\n\n?\n"},{"id":"276337","messageId":"20160119082828.GE21205@distanz.ch","threadId":"41192","inReplyTo":"CAPig+cQ5X7r22pXyCs_n+-mXK3Lzh1CpAMQ_PbuhLT4C3S+v1Q@mail.gmail.com","subject":"Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-01-19T08:28:28Z","receivedAt":"2016-01-19T08:28:28Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"On 2016-01-18 at 23:13:22 +0100, Eric Sunshine wrote:\n> On Mon, Jan 18, 2016 at 4:21 PM, Junio C Hamano wrote:\n> > On Jan 18, 2016 13:11, \"Eric Sunshine\" wrote:\n> >> On Thu, Jan 14, 2016 at 3:45 PM, Junio C Hamano wrote:\n> >>> If for some reason interpret-trailers fails to fail, this would\n> >>> leave an unreadable 'message' in the trash directory. Maybe no\n> >>> other tests that come after this one want to be able to read the\n> >>> contents of the file right now, but this is an accident waiting to\n> >>> happen:\n> >>>\n> >>> cat basic_message >message &&\n> >>> + test_when_finished \"chmod +r message\" &&\n> >>> chmod -r message &&\n> >>> test_must_fail ... &&\n> >>> chmod +r message &&\n> >>\n> >> Don't forget to remove this (now unnecessary) \"chmod +r\" once you've\n> >> added the 'test_when_finished \"chmod +r\"'.\n> >>\n> >>> test_cmp ...\n> >\n> > It still is necessary for the test-cmp to work, no?\n> \n> My bad. Ignore me.\n> \n> By the way, isn't the:\n> \n> cat basic_message >message &&\n> \n> in the above test just an unusual way to say:\n> \n> cp basic_message message &&\n> \n> ?\n\nYes. I was following the other test cases which use cat to build more\ncomplex messages.\n\nI can change this as well along with the 'test_when_finished' fix.\n"},{"id":"276349","messageId":"xmqqio2pbgov.fsf@gitster.mtv.corp.google.com","threadId":"41192","inReplyTo":"CAPig+cRRdca7PfkqppY2X7KSFpHX0yH19fxRL+w_=u9vg7NV9A@mail.gmail.com","subject":"Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-19T17:52:00Z","receivedAt":"2016-01-19T17:52:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> I think POSIXPERM is all you need for this case; SANITY doesn't buy\n> you anything, if I understand correctly.\n>\n>>> + cat basic_message >message &&\n>>> + chmod -r message &&\n>>> + test_must_fail git interpret-trailers --trailer \"Reviewed-by: Alice\" --in-place message &&\n\nThe purpose of \"chmod -r message\" is to force interpret-trailers to\nfail due to its input being unreadable; without SANITY, i.e. running\nthis test as root, the command would happily read from message that\nis marked as unreadable by anybody, and test_must_fail will not pass.\n"},{"id":"276350","messageId":"CAPig+cRi2knygjeaMtojAr65BE71B-z7q+s8V5rcGrV9Qja6jw@mail.gmail.com","threadId":"41192","inReplyTo":"xmqqio2pbgov.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-19T17:56:18Z","receivedAt":"2016-01-19T17:56:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 19, 2016 at 12:52 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>> I think POSIXPERM is all you need for this case; SANITY doesn't buy\n>> you anything, if I understand correctly.\n>>\n>>>> + cat basic_message >message &&\n>>>> + chmod -r message &&\n>>>> + test_must_fail git interpret-trailers --trailer \"Reviewed-by: Alice\" --in-place message &&\n>\n> The purpose of \"chmod -r message\" is to force interpret-trailers to\n> fail due to its input being unreadable; without SANITY, i.e. running\n> this test as root, the command would happily read from message that\n> is marked as unreadable by anybody, and test_must_fail will not pass.\n\nMakes sense. I never run tests as root, thus wasn't thinking along those lines.\n"},{"id":"276351","messageId":"CAPig+cRozqCKdC2+nyG-UM6xFo_sSqa7OhGgcycyyDQujZHtHA@mail.gmail.com","threadId":"41192","inReplyTo":"CAPig+cRi2knygjeaMtojAr65BE71B-z7q+s8V5rcGrV9Qja6jw@mail.gmail.com","subject":"Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-19T18:10:49Z","receivedAt":"2016-01-19T18:10:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 19, 2016 at 12:56 PM, Eric Sunshine wrote:\n> On Tue, Jan 19, 2016 at 12:52 PM, Junio C Hamano wrote:\n>> Eric Sunshine writes:\n>>> I think POSIXPERM is all you need for this case; SANITY doesn't buy\n>>> you anything, if I understand correctly.\n>>>\n>>>>> + cat basic_message >message &&\n>>>>> + chmod -r message &&\n>>>>> + test_must_fail git interpret-trailers --trailer \"Reviewed-by: Alice\" --in-place message &&\n>>\n>> The purpose of \"chmod -r message\" is to force interpret-trailers to\n>> fail due to its input being unreadable; without SANITY, i.e. running\n>> this test as root, the command would happily read from message that\n>> is marked as unreadable by anybody, and test_must_fail will not pass.\n>\n> Makes sense. I never run tests as root, thus wasn't thinking along those lines.\n\nOn reflection, this doesn't make sense to me. Perhaps I'm missing\nsomething obvious.\n\nMy understanding is that SANITY is an expectation that directory\npermissions work in an expected POSIXy way: that is, a file can't be\ndeleted when its containing directory lacks 'write', and a file can't\nbe read/accessed when the directory has neither 'read' nor 'execute'.\nThis doesn't say anything about root not being allowed to read a file\nwhen the file itself lacks 'read'.\n\nAs far as I can tell, as coded, this test will *always* fail as root\nsince root will always be able to read 'message'.\n"},{"id":"276363","messageId":"xmqqfuxt9ti3.fsf@gitster.mtv.corp.google.com","threadId":"41192","inReplyTo":"CAPig+cRozqCKdC2+nyG-UM6xFo_sSqa7OhGgcycyyDQujZHtHA@mail.gmail.com","subject":"Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-19T20:58:12Z","receivedAt":"2016-01-19T20:58:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> My understanding is that SANITY is an expectation that directory\n> permissions work in an expected POSIXy way: that is, a file can't be\n> deleted when its containing directory lacks 'write', and a file can't\n> be read/accessed when the directory has neither 'read' nor 'execute'.\n> This doesn't say anything about root not being allowed to read a file\n> when the file itself lacks 'read'.\n\nIn short, SANITY is \"does looking at permission bits sufficient to\nanticipate what the filesystem would do?\" while POSIXPERM is \"can\nchmod be used to tweak permission bits of the filesystem\" (a\nfilesystem that lacks permission bits support would qualify as\n!POSIXPERM, as there is nothing to tweak in the first place).\n\nI suspect the comment added by f400e51c and its patch description\nstressed too much about permission of a directory affecting what we\ncan do to files inside the directory, and failed to describe another\ncriteria for a sane environment: \"files whose permission bits say\nyou shouldn't be able to read or write cannot be read or written\".\nTraditionally, running tests as root was one major way to break\nSANITY, but as f400e51c noticed, \"can we write to '/'?\", which was\nan old-fashioned way to catch the only case where SANITY does not\nhold on POSIX systems [*1*], cannot catch insanity on non-POSIX\nsystem like Cygwin.\n\nPOSIXPERM is more about \"if we do chmod, does filesystem remember it\nso that ls -l reports the same?\" Output from \"git grep POSIXPERM t\"\nshows that some users of it also assume that it requires \"we can\nmake something executable by doing chmod +x and unexecutable by\ndoing chmod -x\" (and that is fine--running tests as root would not\nmake an unexecutable file executable). The tests that require\nPOSIXPERM but not SANITY can be run by root (I am not saying that\nrunning tests as root is safe or sane, though) and are expected to\nproduce the same result as they were run by a non-root user.\n\n\n[Footnote]\n\n*1* This is an old-fashioned way back when everybody on UNIX was\n sane and / had 0755 permission bits everywhere. Some people\n make their / owned by sysadmin group and give 0775 bits, and\n \"test -w /\" would incorrectly say that the environment lacks\n SANITY when run by non-root users in the sysadmin group, even\n though our tests like \"chmod -r file && ! cat file\" (drop\n readable bit, expect it to become unreadable) guarded by SANITY\n can correctly run by them.\n\n Back when f400e51c was written, checking `whoami` was suggested\n as an alternative as a workaround for this \"/ may be writable by\n a non-root person and not a good SANITY check\" issue, but that\n was rejected because it obviously would not work on Cygwin.\n"},{"id":"276368","messageId":"CAPig+cRHTs9q4k=CqtY2j=ZtTYMU6_SPeCHkQe4m5AGXOjg_Ww@mail.gmail.com","threadId":"41192","inReplyTo":"xmqqfuxt9ti3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-19T21:45:15Z","receivedAt":"2016-01-19T21:45:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 19, 2016 at 3:58 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>> My understanding is that SANITY is an expectation that directory\n>> permissions work in an expected POSIXy way: that is, a file can't be\n>> deleted when its containing directory lacks 'write', and a file can't\n>> be read/accessed when the directory has neither 'read' nor 'execute'.\n>> This doesn't say anything about root not being allowed to read a file\n>> when the file itself lacks 'read'.\n>\n> In short, SANITY is \"does looking at permission bits sufficient to\n> anticipate what the filesystem would do?\" while POSIXPERM is \"can\n> chmod be used to tweak permission bits of the filesystem\" (a\n> filesystem that lacks permission bits support would qualify as\n> !POSIXPERM, as there is nothing to tweak in the first place).\n>\n> I suspect the comment added by f400e51c and its patch description\n> stressed too much about permission of a directory affecting what we\n> can do to files inside the directory, and failed to describe another\n> criteria for a sane environment: \"files whose permission bits say\n> you shouldn't be able to read or write cannot be read or written\".\n\nYou suspect correctly. It was exactly the comment added by f400e51c\nthat misled me. (t/README does, on the other hand, mention \"root\", as\nI noticed after reading your previous response.)\n\nThanks for spelling all this out. Hopefully, others reading your reply\n(now and later) will be less confused than I.\n"},{"id":"276373","messageId":"xmqqvb6p8bmm.fsf@gitster.mtv.corp.google.com","threadId":"41192","inReplyTo":"CAPig+cRHTs9q4k=CqtY2j=ZtTYMU6_SPeCHkQe4m5AGXOjg_Ww@mail.gmail.com","subject":"Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-19T22:09:37Z","receivedAt":"2016-01-19T22:09:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> You suspect correctly. It was exactly the comment added by f400e51c\n> that misled me. (t/README does, on the other hand, mention \"root\", as\n> I noticed after reading your previous response.)\n>\n> Thanks for spelling all this out. Hopefully, others reading your reply\n> (now and later) will be less confused than I.\n\nIt is not too late to fix that, though.\n\n-- >8 --\nSubject: test-lib: clarify and tighten SANITY\n\nf400e51c (test-lib.sh: set prerequisite SANITY by testing what we\nreally need, 2015-01-27) improved the way SANITY prerequisite was\ndetermined, but made the resulting code (incorrectly) imply that\nSANITY is all about effects of permission bits of the containing\ndirectory has on the files contained in it by the comment it added,\nits log message and the actual tests.\n\nBy the way, while we are on the subject, POSIXPERM is more about \"if\nwe do chmod, does filesystem remember it so that ls -l reports the\nsame?\" Output from \"git grep POSIXPERM t\" shows that some users of\nit also assume that it requires \"we can make something executable by\ndoing chmod +x and unexecutable by doing chmod -x\" (and that is\nfine--running tests as root would not make an unexecutable file\nexecutable). The tests that require POSIXPERM but not SANITY can be\nrun by root (I am not saying that running tests as root is safe or\nsane, though) and are expected to produce the same result as they\nwere run by a non-root user.\n\nSigned-off-by: Junio C Hamano \n---\n t/test-lib.sh | 18 +++++++++++++-----\n 1 file changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 446d8d5..68c31ae 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -997,20 +997,28 @@ test_lazy_prereq NOT_ROOT '\n \ttest \"$uid\" != 0\n '\n \n-# On a filesystem that lacks SANITY, a file can be deleted even if\n-# the containing directory doesn't have write permissions, or a file\n-# can be accessed even if the containing directory doesn't have read\n-# or execute permissions, causing our tests that validate that Git\n-# works sensibly in such situations.\n+# SANITY is about \"can you correctly predict what the filesystem would\n+# do by only looking at the permission bits of the files and\n+# directories?\" A typical example of !SANITY is running the test\n+# suite as root, where a test may expect \"chmod -r file && cat file\"\n+# to fail because file is supposed to be unreadable after a successful\n+# chmod. In an environment (i.e. combination of what filesystem is\n+# being used and who is running the tests) that lacks SANITY, you may\n+# be able to delete or create a file when the containing directory\n+# doesn't have write permissions, or access a file even if the\n+# containing directory doesn't have read or execute permissions.\n+\n test_lazy_prereq SANITY '\n \tmkdir SANETESTD.1 SANETESTD.2 &&\n \n \tchmod +w SANETESTD.1 SANETESTD.2 &&\n \t>SANETESTD.1/x 2>SANETESTD.2/x &&\n \tchmod -w SANETESTD.1 &&\n+\tchmod -r SANETESTD.1/x &&\n \tchmod -rx SANETESTD.2 ||\n \terror \"bug in test sript: cannot prepare SANETESTD\"\n \n+\t! test -r SANETESTD.1/x &&\n \t! rm SANETESTD.1/x && ! test -f SANETESTD.2/x\n \tstatus=$?\n \n"},{"id":"276387","messageId":"CAPig+cS_kOg6gJPW_VygzSYufTnw5Emsu88y8P=4_CTdnWCx-Q@mail.gmail.com","threadId":"41192","inReplyTo":"xmqqvb6p8bmm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-20T00:20:28Z","receivedAt":"2016-01-20T00:20:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 19, 2016 at 5:09 PM, Junio C Hamano wrote:\n> Subject: test-lib: clarify and tighten SANITY\n>\n> f400e51c (test-lib.sh: set prerequisite SANITY by testing what we\n> really need, 2015-01-27) improved the way SANITY prerequisite was\n> determined, but made the resulting code (incorrectly) imply that\n> SANITY is all about effects of permission bits of the containing\n> directory has on the files contained in it by the comment it added,\n> its log message and the actual tests.\n>\n> By the way, while we are on the subject, POSIXPERM is more about \"if\n> we do chmod, does filesystem remember it so that ls -l reports the\n> same?\" Output from \"git grep POSIXPERM t\" shows that some users of\n> it also assume that it requires \"we can make something executable by\n> doing chmod +x and unexecutable by doing chmod -x\" (and that is\n> fine--running tests as root would not make an unexecutable file\n> executable). The tests that require POSIXPERM but not SANITY can be\n> run by root (I am not saying that running tests as root is safe or\n> sane, though) and are expected to produce the same result as they\n> were run by a non-root user.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> +# SANITY is about \"can you correctly predict what the filesystem would\n> +# do by only looking at the permission bits of the files and\n> +# directories?\" A typical example of !SANITY is running the test\n> +# suite as root, where a test may expect \"chmod -r file && cat file\"\n> +# to fail because file is supposed to be unreadable after a successful\n> +# chmod. In an environment (i.e. combination of what filesystem is\n> +# being used and who is running the tests) that lacks SANITY, you may\n> +# be able to delete or create a file when the containing directory\n> +# doesn't have write permissions, or access a file even if the\n> +# containing directory doesn't have read or execute permissions.\n\nThis makes the intent much clearer. Thanks.\n\n> test_lazy_prereq SANITY '\n> mkdir SANETESTD.1 SANETESTD.2 &&\n>\n> chmod +w SANETESTD.1 SANETESTD.2 &&\n> >SANETESTD.1/x 2>SANETESTD.2/x &&\n> chmod -w SANETESTD.1 &&\n> + chmod -r SANETESTD.1/x &&\n> chmod -rx SANETESTD.2 ||\n> error \"bug in test sript: cannot prepare SANETESTD\"\n>\n> + ! test -r SANETESTD.1/x &&\n> ! rm SANETESTD.1/x && ! test -f SANETESTD.2/x\n> status=$?\n"}]}