[PATCH v4 0/2] Add in-place editing support to git interpret-trailers

[PATCH v4 0/2] Add in-place editing support to git interpret-trailers

[Tobias Klauser]

This patch series adds support for in-place editing to git
interpret-trailers akin to sed -i, perl -i.

v3->v4:
 - Reword a test title, as suggested by Eric Sunshine.
 - Add a test to verify that the original file is not clobbered/deleted
   on error, as suggested by Eric Sunshine.
 - Move code specific to in-place editing from process_trailers() into a
   separate function to keep the overall flow clean. Suggested by Eric
   Sunshine.
 - Drop unnecessary braces, as pointed out by Eric Sunshine.
 - Use a more meaningful title for patch 1/2. Suggested by Junio Hamano.

v2->v3:
 - Rephrase two error messages according to the suggestions by Matthieu
   Moy.

v1->v2:
 - Split patch to make review easier, as suggested by Matthieu Moy.
 - Rename FILE * function parameters to a more readable name, as
   suggested by Matthieu Moy.
 - Write output to temporary file and rename after successfully written
   in full to avoid losing the original file in case of an
   error/interrupt. Pointed out by Eric Sunshine.

Tobias Klauser (2):
  trailer: allow to write to files other than stdout
  interpret-trailers: add option for in-place editing

 Documentation/git-interpret-trailers.txt | 24 ++++++++++-
 builtin/interpret-trailers.c             | 13 ++++--
 t/t7513-interpret-trailers.sh            | 40 ++++++++++++++++++
 trailer.c                                | 69 +++++++++++++++++++++++++-------
 trailer.h                                |  3 +-
 5 files changed, 129 insertions(+), 20 deletions(-)

-- 
2.7.0.1.g5e091f5

[PATCH v4 1/2] trailer: allow to write to files other than stdout

[Tobias Klauser]

Use fprintf instead of printf in trailer.c in order to allow printing
to a file other than stdout. This will be needed to support in-place
editing in git interpret-trailers.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 trailer.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/trailer.c b/trailer.c
index 6f3416febaba..176fac213450 100644
--- a/trailer.c
+++ b/trailer.c
@@ -108,23 +108,23 @@ static char last_non_space_char(const char *s)
 	return '\0';
 }
 
-static void print_tok_val(const char *tok, const char *val)
+static void print_tok_val(FILE *outfile, const char *tok, const char *val)
 {
 	char c = last_non_space_char(tok);
 	if (!c)
 		return;
 	if (strchr(separators, c))
-		printf("%s%s\n", tok, val);
+		fprintf(outfile, "%s%s\n", tok, val);
 	else
-		printf("%s%c %s\n", tok, separators[0], val);
+		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
 }
 
-static void print_all(struct trailer_item *first, int trim_empty)
+static void print_all(FILE *outfile, struct trailer_item *first, int trim_empty)
 {
 	struct trailer_item *item;
 	for (item = first; item; item = item->next) {
 		if (!trim_empty || strlen(item->value) > 0)
-			print_tok_val(item->token, item->value);
+			print_tok_val(outfile, item->token, item->value);
 	}
 }
 
@@ -795,14 +795,15 @@ static int has_blank_line_before(struct strbuf **lines, int start)
 	return 0;
 }
 
-static void print_lines(struct strbuf **lines, int start, int end)
+static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end)
 {
 	int i;
 	for (i = start; lines[i] && i < end; i++)
-		printf("%s", lines[i]->buf);
+		fprintf(outfile, "%s", lines[i]->buf);
 }
 
-static int process_input_file(struct strbuf **lines,
+static int process_input_file(FILE *outfile,
+			      struct strbuf **lines,
 			      struct trailer_item **in_tok_first,
 			      struct trailer_item **in_tok_last)
 {
@@ -818,10 +819,10 @@ static int process_input_file(struct strbuf **lines,
 	trailer_start = find_trailer_start(lines, trailer_end);
 
 	/* Print lines before the trailers as is */
-	print_lines(lines, 0, trailer_start);
+	print_lines(outfile, lines, 0, trailer_start);
 
 	if (!has_blank_line_before(lines, trailer_start - 1))
-		printf("\n");
+		fprintf(outfile, "\n");
 
 	/* Parse trailer lines */
 	for (i = trailer_start; i < trailer_end; i++) {
@@ -849,6 +850,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai
 	struct trailer_item *arg_tok_first;
 	struct strbuf **lines;
 	int trailer_end;
+	FILE *outfile = stdout;
 
 	/* Default config must be setup first */
 	git_config(git_trailer_default_config, NULL);
@@ -857,18 +859,18 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai
 	lines = read_input_file(file);
 
 	/* Print the lines before the trailers */
-	trailer_end = process_input_file(lines, &in_tok_first, &in_tok_last);
+	trailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);
 
 	arg_tok_first = process_command_line_args(trailers);
 
 	process_trailers_lists(&in_tok_first, &in_tok_last, &arg_tok_first);
 
-	print_all(in_tok_first, trim_empty);
+	print_all(outfile, in_tok_first, trim_empty);
 
 	free_all(&in_tok_first);
 
 	/* Print the lines after the trailers as is */
-	print_lines(lines, trailer_end, INT_MAX);
+	print_lines(outfile, lines, trailer_end, INT_MAX);
 
 	strbuf_list_free(lines);
 }
-- 
2.7.0.1.g5e091f5

[PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Tobias Klauser]

Add a command line option --in-place to support in-place editing akin to
sed -i.  This allows to write commands like the following:

  git interpret-trailers --trailer "X: Y" a.txt > b.txt && mv b.txt a.txt

in a more concise way:

  git interpret-trailers --trailer "X: Y" --in-place a.txt

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 Documentation/git-interpret-trailers.txt | 24 ++++++++++++++++++-
 builtin/interpret-trailers.c             | 13 ++++++----
 t/t7513-interpret-trailers.sh            | 40 +++++++++++++++++++++++++++++++
 trailer.c                                | 41 +++++++++++++++++++++++++++++++-
 trailer.h                                |  3 ++-
 5 files changed, 114 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 0ecd497c4de7..a77b901f1d7b 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -8,7 +8,7 @@ git-interpret-trailers - help add structured information into commit messages
 SYNOPSIS
 --------
 [verse]
-'git interpret-trailers' [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]
+'git interpret-trailers' [--in-place] [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]
 
 DESCRIPTION
 -----------
@@ -64,6 +64,9 @@ folding rules, the encoding rules and probably many other rules.
 
 OPTIONS
 -------
+--in-place::
+	Edit the files in place.
+
 --trim-empty::
 	If the <value> part of any trailer contains only whitespace,
 	the whole trailer will be removed from the resulting message.
@@ -216,6 +219,25 @@ Signed-off-by: Alice <alice@example.com>
 Signed-off-by: Bob <bob@example.com>
 ------------
 
+* Use the '--in-place' option to edit a message file in place:
++
+------------
+$ cat msg.txt
+subject
+
+message
+
+Signed-off-by: Bob <bob@example.com>
+$ git interpret-trailers --trailer 'Acked-by: Alice <alice@example.com>' --in-place msg.txt
+$ cat msg.txt
+subject
+
+message
+
+Signed-off-by: Bob <bob@example.com>
+Acked-by: Alice <alice@example.com>
+------------
+
 * Extract the last commit as a patch, and add a 'Cc' and a
   'Reviewed-by' trailer to it:
 +
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 46838d24a90a..b99ae4be8875 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -12,16 +12,18 @@
 #include "trailer.h"
 
 static const char * const git_interpret_trailers_usage[] = {
-	N_("git interpret-trailers [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]"),
+	N_("git interpret-trailers [--in-place] [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]"),
 	NULL
 };
 
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
+	int in_place = 0;
 	int trim_empty = 0;
 	struct string_list trailers = STRING_LIST_INIT_DUP;
 
 	struct option options[] = {
+		OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
@@ -34,9 +36,12 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
-			process_trailers(argv[i], trim_empty, &trailers);
-	} else
-		process_trailers(NULL, trim_empty, &trailers);
+			process_trailers(argv[i], in_place, trim_empty, &trailers);
+	} else {
+		if (in_place)
+			die(_("no input file given for in-place editing"));
+		process_trailers(NULL, in_place, trim_empty, &trailers);
+	}
 
 	string_list_clear(&trailers, 0);
 
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 322c436a494c..aee785cffa8d 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -326,6 +326,46 @@ test_expect_success 'with complex patch, args and --trim-empty' '
 	test_cmp expected actual
 '
 
+test_expect_success 'in-place editing with basic patch' '
+	cat basic_message >message &&
+	cat basic_patch >>message &&
+	cat basic_message >expected &&
+	echo >>expected &&
+	cat basic_patch >>expected &&
+	git interpret-trailers --in-place message &&
+	test_cmp expected message
+'
+
+test_expect_success 'in-place editing with additional trailer' '
+	cat basic_message >message &&
+	cat basic_patch >>message &&
+	cat basic_message >expected &&
+	echo >>expected &&
+	cat >>expected <<-\EOF &&
+		Reviewed-by: Alice
+	EOF
+	cat basic_patch >>expected &&
+	git interpret-trailers --trailer "Reviewed-by: Alice" --in-place message &&
+	test_cmp expected message
+'
+
+test_expect_success 'in-place editing on stdin disallowed' '
+	test_must_fail git interpret-trailers --trailer "Reviewed-by: Alice" --in-place < basic_message
+'
+
+test_expect_success 'in-place editing on non-existing file' '
+	test_must_fail git interpret-trailers --trailer "Reviewed-by: Alice" --in-place nonexisting &&
+	test_path_is_missing nonexisting
+'
+
+test_expect_success POSIXPERM,SANITY "in-place editing doesn't clobber original file on error" '
+	cat basic_message >message &&
+	chmod -r message &&
+	test_must_fail git interpret-trailers --trailer "Reviewed-by: Alice" --in-place message &&
+	chmod +r message &&
+	test_cmp message basic_message
+'
+
 test_expect_success 'using "where = before"' '
 	git config trailer.bug.where "before" &&
 	cat complex_message_body >expected &&
diff --git a/trailer.c b/trailer.c
index 176fac213450..94b387b49971 100644
--- a/trailer.c
+++ b/trailer.c
@@ -2,6 +2,7 @@
 #include "string-list.h"
 #include "run-command.h"
 #include "commit.h"
+#include "tempfile.h"
 #include "trailer.h"
 /*
  * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
@@ -843,7 +844,38 @@ static void free_all(struct trailer_item **first)
 	}
 }
 
-void process_trailers(const char *file, int trim_empty, struct string_list *trailers)
+static struct tempfile trailers_tempfile;
+
+static FILE *create_in_place_tempfile(const char *file)
+{
+	struct stat st;
+	struct strbuf template = STRBUF_INIT;
+	const char *tail;
+	FILE *outfile;
+
+	if (stat(file, &st))
+		die_errno(_("could not stat %s"), file);
+	if (!S_ISREG(st.st_mode))
+		die(_("file %s is not a regular file"), file);
+	if (!(st.st_mode & S_IWUSR))
+		die(_("file %s is not writable by user"), file);
+
+	/* Create temporary file in the same directory as the original */
+	tail = strrchr(file, '/');
+	if (tail != NULL)
+		strbuf_add(&template, file, tail - file + 1);
+	strbuf_addstr(&template, "git-interpret-trailers-XXXXXX");
+
+	xmks_tempfile_m(&trailers_tempfile, template.buf, st.st_mode);
+	strbuf_release(&template);
+	outfile = fdopen_tempfile(&trailers_tempfile, "w");
+	if (!outfile)
+		die_errno(_("could not open temporary file"));
+
+	return outfile;
+}
+
+void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)
 {
 	struct trailer_item *in_tok_first = NULL;
 	struct trailer_item *in_tok_last = NULL;
@@ -858,6 +890,9 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai
 
 	lines = read_input_file(file);
 
+	if (in_place)
+		outfile = create_in_place_tempfile(file);
+
 	/* Print the lines before the trailers */
 	trailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);
 
@@ -872,5 +907,9 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai
 	/* Print the lines after the trailers as is */
 	print_lines(outfile, lines, trailer_end, INT_MAX);
 
+	if (in_place)
+		if (rename_tempfile(&trailers_tempfile, file))
+			die_errno(_("could not rename temporary file to %s"), file);
+
 	strbuf_list_free(lines);
 }
diff --git a/trailer.h b/trailer.h
index 8eb25d565e28..36b40b81761f 100644
--- a/trailer.h
+++ b/trailer.h
@@ -1,6 +1,7 @@
 #ifndef TRAILER_H
 #define TRAILER_H
 
-void process_trailers(const char *file, int trim_empty, struct string_list *trailers);
+void process_trailers(const char *file, int in_place, int trim_empty,
+		      struct string_list *trailers);
 
 #endif /* TRAILER_H */
-- 
2.7.0.1.g5e091f5

Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Junio C Hamano]

Tobias Klauser <tklauser@distanz.ch> writes:

> Add a command line option --in-place to support in-place editing akin to
> sed -i.  This allows to write commands like the following:
>
>   git interpret-trailers --trailer "X: Y" a.txt > b.txt && mv b.txt a.txt
>
> in a more concise way:
>
>   git interpret-trailers --trailer "X: Y" --in-place a.txt
>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> ---

Thanks, will replace.  I found some micronits, none of which I think
is big enough to require another reroll, but since I found them
already, I'll just point them out.

> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> index 322c436a494c..aee785cffa8d 100755
> --- a/t/t7513-interpret-trailers.sh
> +++ b/t/t7513-interpret-trailers.sh
> @@ -326,6 +326,46 @@ test_expect_success 'with complex patch, args and --trim-empty' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'in-place editing with basic patch' '
> +	cat basic_message >message &&
> +	cat basic_patch >>message &&
> +	cat basic_message >expected &&
> +	echo >>expected &&
> +	cat basic_patch >>expected &&
> +	git interpret-trailers --in-place message &&
> +	test_cmp expected message
> +'
> +
> +test_expect_success 'in-place editing with additional trailer' '
> +	cat basic_message >message &&
> +	cat basic_patch >>message &&
> +	cat basic_message >expected &&
> +	echo >>expected &&
> +	cat >>expected <<-\EOF &&
> +		Reviewed-by: Alice
> +	EOF

The "echo" is not needed, if you just include a leading blank line
in the here-document you use with this "cat".

> +test_expect_success POSIXPERM,SANITY "in-place editing doesn't clobber original file on error" '
> +	cat basic_message >message &&
> +	chmod -r message &&
> +	test_must_fail git interpret-trailers --trailer "Reviewed-by: Alice" --in-place message &&
> +	chmod +r message &&
> +	test_cmp message basic_message
> +'

If for some reason interpret-trailers fails to fail, this would
leave an unreadable 'message' in the trash directory.  Maybe no
other tests that come after this one want to be able to read the
contents of the file right now, but this is an accident waiting to
happen:

	cat basic_message >message &&
+       test_when_finished "chmod +r message" &&
        chmod -r message &&
        test_must_fail ... &&
	chmod +r message &&
        test_cmp ...

> +	if (!S_ISREG(st.st_mode))
> +		die(_("file %s is not a regular file"), file);
> +	if (!(st.st_mode & S_IWUSR))
> +		die(_("file %s is not writable by user"), file);

Hmph, are these two necessary, and do they make sense?

When doing an in-place thing, the primary thing you care about is
that you can read from the file and you can deposit the result of
the rewrite under the original name.  If for some reason a system
allowed you to read from a non-regular file and interpret-trailers
can do a sensible thing to the contents you read from there, do you
have to insist that original must be S_ISREG()?  Also, a funny file
(e.g. "interpret-trailers -i .") is likely to fail on the input
side.

For the latter,

    $ chmod a-w COPYING
    $ sed -i -e 's/a/b/' COPYING

seems to succeed _and_ leave the permission bits intact, i.e.
I get this before and after "sed -i"

    $ ls -l COPYING
    -r--r----- 1 jch eng 18765 Jan 14 12:34 COPYING

which hints at two points:

 - The users (of "sed -i") may have demanded that in-place update of
   read-only file must be allowed, and there may have been a good
   reason for wanting to do so.  That reason may apply equally to us
   here.

 - If we were to follow suit, then we should not forget to restore
   the permission bits on the new file.

In any case, these are something we could loosen after people gain
experience with the feature, so I think it is OK as-is, at least for
now.

> +	if (in_place)
> +		if (rename_tempfile(&trailers_tempfile, file))
> +			die_errno(_("could not rename temporary file to %s"), file);
> +

I briefly wondered if this should be

	if (in_place && rename_tempfile(...))
		die_errno(...);

to save one indentation level, but I think it is a bad idea,
i.e. the above code should stay as-is.

Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Tobias Klauser]

On 2016-01-14 at 21:45:01 +0100, Junio C Hamano <gitster@pobox.com> wrote:
> Tobias Klauser <tklauser@distanz.ch> writes:
> 
> > Add a command line option --in-place to support in-place editing akin to
> > sed -i.  This allows to write commands like the following:
> >
> >   git interpret-trailers --trailer "X: Y" a.txt > b.txt && mv b.txt a.txt
> >
> > in a more concise way:
> >
> >   git interpret-trailers --trailer "X: Y" --in-place a.txt
> >
> > Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> > ---
> 
> Thanks, will replace.  I found some micronits, none of which I think
> is big enough to require another reroll, but since I found them
> already, I'll just point them out.

Thanks a lot for your review!

> > diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> > index 322c436a494c..aee785cffa8d 100755
> > --- a/t/t7513-interpret-trailers.sh
> > +++ b/t/t7513-interpret-trailers.sh
> > @@ -326,6 +326,46 @@ test_expect_success 'with complex patch, args and --trim-empty' '
> >  	test_cmp expected actual
> >  '
> >  
> > +test_expect_success 'in-place editing with basic patch' '
> > +	cat basic_message >message &&
> > +	cat basic_patch >>message &&
> > +	cat basic_message >expected &&
> > +	echo >>expected &&
> > +	cat basic_patch >>expected &&
> > +	git interpret-trailers --in-place message &&
> > +	test_cmp expected message
> > +'
> > +
> > +test_expect_success 'in-place editing with additional trailer' '
> > +	cat basic_message >message &&
> > +	cat basic_patch >>message &&
> > +	cat basic_message >expected &&
> > +	echo >>expected &&
> > +	cat >>expected <<-\EOF &&
> > +		Reviewed-by: Alice
> > +	EOF
> 
> The "echo" is not needed, if you just include a leading blank line
> in the here-document you use with this "cat".

Classical case of copy&paste and me not thinking enough what could be
simplified ;)

> > +test_expect_success POSIXPERM,SANITY "in-place editing doesn't clobber original file on error" '
> > +	cat basic_message >message &&
> > +	chmod -r message &&
> > +	test_must_fail git interpret-trailers --trailer "Reviewed-by: Alice" --in-place message &&
> > +	chmod +r message &&
> > +	test_cmp message basic_message
> > +'
> 
> If for some reason interpret-trailers fails to fail, this would
> leave an unreadable 'message' in the trash directory.  Maybe no
> other tests that come after this one want to be able to read the
> contents of the file right now, but this is an accident waiting to
> happen:
> 
> 	cat basic_message >message &&
> +       test_when_finished "chmod +r message" &&
>         chmod -r message &&
>         test_must_fail ... &&
> 	chmod +r message &&
>         test_cmp ...

Indeed, I forgot about this. I saw you already folded in the missing
'chmod +r message' in your tree. Thanks for that!

> 
> > +	if (!S_ISREG(st.st_mode))
> > +		die(_("file %s is not a regular file"), file);
> > +	if (!(st.st_mode & S_IWUSR))
> > +		die(_("file %s is not writable by user"), file);
> 
> Hmph, are these two necessary, and do they make sense?

The check for S_ISREG I added because it's also done in sed:

	$ mkdir /tmp/foobar
	$ sed -i 's/foo/baz/' /tmp/foobar
	sed: couldn't edit foobar: not a regular file

I quickly checked their source and they do indeed a check for S_ISREG in
case of in-place editing (sed v4.2.2-98-g61c0a53ec997,
sed/execute.c:600)

But the writable check is probably too strict, I agree.

> When doing an in-place thing, the primary thing you care about is
> that you can read from the file and you can deposit the result of
> the rewrite under the original name.  If for some reason a system
> allowed you to read from a non-regular file and interpret-trailers
> can do a sensible thing to the contents you read from there, do you
> have to insist that original must be S_ISREG()?  Also, a funny file
> (e.g. "interpret-trailers -i .") is likely to fail on the input
> side.
> 
> For the latter,
> 
>     $ chmod a-w COPYING
>     $ sed -i -e 's/a/b/' COPYING
> 
> seems to succeed _and_ leave the permission bits intact, i.e.
> I get this before and after "sed -i"
> 
>     $ ls -l COPYING
>     -r--r----- 1 jch eng 18765 Jan 14 12:34 COPYING
> 
> which hints at two points:
> 
>  - The users (of "sed -i") may have demanded that in-place update of
>    read-only file must be allowed, and there may have been a good
>    reason for wanting to do so.  That reason may apply equally to us
>    here.

True. AFAIK rename(2) only need write permissions on the containing directory,
not the source/destination file itself. And rename_tempfile should barf
about that. So the S_IWUSR is unnecessarily strict...

>  - If we were to follow suit, then we should not forget to restore
>    the permission bits on the new file.

This should already be the case to to st.st_mode being passed to
xmks_tempfile_m, no?

> In any case, these are something we could loosen after people gain
> experience with the feature, so I think it is OK as-is, at least for
> now.

I agree.

> > +	if (in_place)
> > +		if (rename_tempfile(&trailers_tempfile, file))
> > +			die_errno(_("could not rename temporary file to %s"), file);
> > +
> 
> I briefly wondered if this should be
> 
> 	if (in_place && rename_tempfile(...))
> 		die_errno(...);
> 
> to save one indentation level, but I think it is a bad idea,
> i.e. the above code should stay as-is.

Thought about this for a while too, but I concluded that it would hurt
readability more than it would help.

Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Junio C Hamano]

Tobias Klauser <tklauser@distanz.ch> writes:

>> > +test_expect_success POSIXPERM,SANITY "in-place editing doesn't clobber original file on error" '
>> > +	cat basic_message >message &&
>> > +	chmod -r message &&
>> > +	test_must_fail git interpret-trailers --trailer "Reviewed-by: Alice" --in-place message &&
>> > +	chmod +r message &&
>> > +	test_cmp message basic_message
>> > +'
>> 
>> If for some reason interpret-trailers fails to fail, this would
>> leave an unreadable 'message' in the trash directory.  Maybe no
>> other tests that come after this one want to be able to read the
>> contents of the file right now, but this is an accident waiting to
>> happen:
>> 
>> 	cat basic_message >message &&
>> +       test_when_finished "chmod +r message" &&
>>         chmod -r message &&
>>         test_must_fail ... &&
>> 	chmod +r message &&
>>         test_cmp ...
>
> Indeed, I forgot about this. I saw you already folded in the missing
> 'chmod +r message' in your tree. Thanks for that!

I did no such thing, though.

Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Tobias Klauser]

On 2016-01-15 at 18:24:49 +0100, Junio C Hamano <gitster@pobox.com> wrote:
> Tobias Klauser <tklauser@distanz.ch> writes:
> 
> >> > +test_expect_success POSIXPERM,SANITY "in-place editing doesn't clobber original file on error" '
> >> > +	cat basic_message >message &&
> >> > +	chmod -r message &&
> >> > +	test_must_fail git interpret-trailers --trailer "Reviewed-by: Alice" --in-place message &&
> >> > +	chmod +r message &&
> >> > +	test_cmp message basic_message
> >> > +'
> >> 
> >> If for some reason interpret-trailers fails to fail, this would
> >> leave an unreadable 'message' in the trash directory.  Maybe no
> >> other tests that come after this one want to be able to read the
> >> contents of the file right now, but this is an accident waiting to
> >> happen:
> >> 
> >> 	cat basic_message >message &&
> >> +       test_when_finished "chmod +r message" &&
> >>         chmod -r message &&
> >>         test_must_fail ... &&
> >> 	chmod +r message &&
> >>         test_cmp ...
> >
> > Indeed, I forgot about this. I saw you already folded in the missing
> > 'chmod +r message' in your tree. Thanks for that!
> 
> I did no such thing, though.

Sorry, my misunderstanding. I thought about "chmod +r" but of course the
essential part is the

  +       test_when_finished "chmod +r message" &&

which isn't in your tree.

Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Eric Sunshine]

On Thu, Jan 14, 2016 at 3:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tobias Klauser <tklauser@distanz.ch> writes:
>> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
>> @@ -326,6 +326,46 @@ test_expect_success 'with complex patch, args and --trim-empty' '
>> +test_expect_success POSIXPERM,SANITY "in-place editing doesn't clobber original file on error" '

I think POSIXPERM is all you need for this case; SANITY doesn't buy
you anything, if I understand correctly.

>> +     cat basic_message >message &&
>> +     chmod -r message &&
>> +     test_must_fail git interpret-trailers --trailer "Reviewed-by: Alice" --in-place message &&
>> +     chmod +r message &&
>> +     test_cmp message basic_message
>> +'
>
> If for some reason interpret-trailers fails to fail, this would
> leave an unreadable 'message' in the trash directory.  Maybe no
> other tests that come after this one want to be able to read the
> contents of the file right now, but this is an accident waiting to
> happen:
>
>         cat basic_message >message &&
> +       test_when_finished "chmod +r message" &&
>         chmod -r message &&
>         test_must_fail ... &&
>         chmod +r message &&

Don't forget to remove this (now unnecessary) "chmod +r" once you've
added the 'test_when_finished "chmod +r"'.

>         test_cmp ...

Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Eric Sunshine]

On Mon, Jan 18, 2016 at 4:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> On Jan 18, 2016 13:11, "Eric Sunshine" <sunshine@sunshineco.com> wrote:
>> On Thu, Jan 14, 2016 at 3:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> If for some reason interpret-trailers fails to fail, this would
>>> leave an unreadable 'message' in the trash directory.  Maybe no
>>> other tests that come after this one want to be able to read the
>>> contents of the file right now, but this is an accident waiting to
>>> happen:
>>>
>>>         cat basic_message >message &&
>>> +       test_when_finished "chmod +r message" &&
>>>         chmod -r message &&
>>>         test_must_fail ... &&
>>>         chmod +r message &&
>>
>> Don't forget to remove this (now unnecessary) "chmod +r" once you've
>> added the 'test_when_finished "chmod +r"'.
>>
>>>         test_cmp ...
>
> It still is necessary for the test-cmp to work, no?

My bad. Ignore me.

By the way, isn't the:

    cat basic_message >message &&

in the above test just an unusual way to say:

    cp basic_message message &&

?

Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Tobias Klauser]

On 2016-01-18 at 23:13:22 +0100, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Jan 18, 2016 at 4:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > On Jan 18, 2016 13:11, "Eric Sunshine" <sunshine@sunshineco.com> wrote:
> >> On Thu, Jan 14, 2016 at 3:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >>> If for some reason interpret-trailers fails to fail, this would
> >>> leave an unreadable 'message' in the trash directory.  Maybe no
> >>> other tests that come after this one want to be able to read the
> >>> contents of the file right now, but this is an accident waiting to
> >>> happen:
> >>>
> >>>         cat basic_message >message &&
> >>> +       test_when_finished "chmod +r message" &&
> >>>         chmod -r message &&
> >>>         test_must_fail ... &&
> >>>         chmod +r message &&
> >>
> >> Don't forget to remove this (now unnecessary) "chmod +r" once you've
> >> added the 'test_when_finished "chmod +r"'.
> >>
> >>>         test_cmp ...
> >
> > It still is necessary for the test-cmp to work, no?
> 
> My bad. Ignore me.
> 
> By the way, isn't the:
> 
>     cat basic_message >message &&
> 
> in the above test just an unusual way to say:
> 
>     cp basic_message message &&
> 
> ?

Yes. I was following the other test cases which use cat to build more
complex messages.

I can change this as well along with the 'test_when_finished' fix.

Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> I think POSIXPERM is all you need for this case; SANITY doesn't buy
> you anything, if I understand correctly.
>
>>> +     cat basic_message >message &&
>>> +     chmod -r message &&
>>> +     test_must_fail git interpret-trailers --trailer "Reviewed-by: Alice" --in-place message &&

The purpose of "chmod -r message" is to force interpret-trailers to
fail due to its input being unreadable; without SANITY, i.e. running
this test as root, the command would happily read from message that
is marked as unreadable by anybody, and test_must_fail will not pass.

Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Eric Sunshine]

On Tue, Jan 19, 2016 at 12:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> I think POSIXPERM is all you need for this case; SANITY doesn't buy
>> you anything, if I understand correctly.
>>
>>>> +     cat basic_message >message &&
>>>> +     chmod -r message &&
>>>> +     test_must_fail git interpret-trailers --trailer "Reviewed-by: Alice" --in-place message &&
>
> The purpose of "chmod -r message" is to force interpret-trailers to
> fail due to its input being unreadable; without SANITY, i.e. running
> this test as root, the command would happily read from message that
> is marked as unreadable by anybody, and test_must_fail will not pass.

Makes sense. I never run tests as root, thus wasn't thinking along those lines.

Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Eric Sunshine]

On Tue, Jan 19, 2016 at 12:56 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Jan 19, 2016 at 12:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>> I think POSIXPERM is all you need for this case; SANITY doesn't buy
>>> you anything, if I understand correctly.
>>>
>>>>> +     cat basic_message >message &&
>>>>> +     chmod -r message &&
>>>>> +     test_must_fail git interpret-trailers --trailer "Reviewed-by: Alice" --in-place message &&
>>
>> The purpose of "chmod -r message" is to force interpret-trailers to
>> fail due to its input being unreadable; without SANITY, i.e. running
>> this test as root, the command would happily read from message that
>> is marked as unreadable by anybody, and test_must_fail will not pass.
>
> Makes sense. I never run tests as root, thus wasn't thinking along those lines.

On reflection, this doesn't make sense to me. Perhaps I'm missing
something obvious.

My understanding is that SANITY is an expectation that directory
permissions work in an expected POSIXy way: that is, a file can't be
deleted when its containing directory lacks 'write', and a file can't
be read/accessed when the directory has neither 'read' nor 'execute'.
This doesn't say anything about root not being allowed to read a file
when the file itself lacks 'read'.

As far as I can tell, as coded, this test will *always* fail as root
since root will always be able to read 'message'.

Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> My understanding is that SANITY is an expectation that directory
> permissions work in an expected POSIXy way: that is, a file can't be
> deleted when its containing directory lacks 'write', and a file can't
> be read/accessed when the directory has neither 'read' nor 'execute'.
> This doesn't say anything about root not being allowed to read a file
> when the file itself lacks 'read'.

In short, SANITY is "does looking at permission bits sufficient to
anticipate what the filesystem would do?" while POSIXPERM is "can
chmod be used to tweak permission bits of the filesystem" (a
filesystem that lacks permission bits support would qualify as
!POSIXPERM, as there is nothing to tweak in the first place).

I suspect the comment added by f400e51c and its patch description
stressed too much about permission of a directory affecting what we
can do to files inside the directory, and failed to describe another
criteria for a sane environment: "files whose permission bits say
you shouldn't be able to read or write cannot be read or written".
Traditionally, running tests as root was one major way to break
SANITY, but as f400e51c noticed, "can we write to '/'?", which was
an old-fashioned way to catch the only case where SANITY does not
hold on POSIX systems [*1*], cannot catch insanity on non-POSIX
system like Cygwin.

POSIXPERM is more about "if we do chmod, does filesystem remember it
so that ls -l reports the same?"  Output from "git grep POSIXPERM t"
shows that some users of it also assume that it requires "we can
make something executable by doing chmod +x and unexecutable by
doing chmod -x" (and that is fine--running tests as root would not
make an unexecutable file executable).  The tests that require
POSIXPERM but not SANITY can be run by root (I am not saying that
running tests as root is safe or sane, though) and are expected to
produce the same result as they were run by a non-root user.


[Footnote]

*1* This is an old-fashioned way back when everybody on UNIX was
    sane and / had 0755 permission bits everywhere.  Some people
    make their / owned by sysadmin group and give 0775 bits, and
    "test -w /" would incorrectly say that the environment lacks
    SANITY when run by non-root users in the sysadmin group, even
    though our tests like "chmod -r file && ! cat file" (drop
    readable bit, expect it to become unreadable) guarded by SANITY
    can correctly run by them.

    Back when f400e51c was written, checking `whoami` was suggested
    as an alternative as a workaround for this "/ may be writable by
    a non-root person and not a good SANITY check" issue, but that
    was rejected because it obviously would not work on Cygwin.

Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Eric Sunshine]

On Tue, Jan 19, 2016 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> My understanding is that SANITY is an expectation that directory
>> permissions work in an expected POSIXy way: that is, a file can't be
>> deleted when its containing directory lacks 'write', and a file can't
>> be read/accessed when the directory has neither 'read' nor 'execute'.
>> This doesn't say anything about root not being allowed to read a file
>> when the file itself lacks 'read'.
>
> In short, SANITY is "does looking at permission bits sufficient to
> anticipate what the filesystem would do?" while POSIXPERM is "can
> chmod be used to tweak permission bits of the filesystem" (a
> filesystem that lacks permission bits support would qualify as
> !POSIXPERM, as there is nothing to tweak in the first place).
>
> I suspect the comment added by f400e51c and its patch description
> stressed too much about permission of a directory affecting what we
> can do to files inside the directory, and failed to describe another
> criteria for a sane environment: "files whose permission bits say
> you shouldn't be able to read or write cannot be read or written".

You suspect correctly. It was exactly the comment added by f400e51c
that misled me. (t/README does, on the other hand, mention "root", as
I noticed after reading your previous response.)

Thanks for spelling all this out. Hopefully, others reading your reply
(now and later) will be less confused than I.

Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> You suspect correctly. It was exactly the comment added by f400e51c
> that misled me. (t/README does, on the other hand, mention "root", as
> I noticed after reading your previous response.)
>
> Thanks for spelling all this out. Hopefully, others reading your reply
> (now and later) will be less confused than I.

It is not too late to fix that, though.

-- >8 --
Subject: test-lib: clarify and tighten SANITY

f400e51c (test-lib.sh: set prerequisite SANITY by testing what we
really need, 2015-01-27) improved the way SANITY prerequisite was
determined, but made the resulting code (incorrectly) imply that
SANITY is all about effects of permission bits of the containing
directory has on the files contained in it by the comment it added,
its log message and the actual tests.

By the way, while we are on the subject, POSIXPERM is more about "if
we do chmod, does filesystem remember it so that ls -l reports the
same?"  Output from "git grep POSIXPERM t" shows that some users of
it also assume that it requires "we can make something executable by
doing chmod +x and unexecutable by doing chmod -x" (and that is
fine--running tests as root would not make an unexecutable file
executable).  The tests that require POSIXPERM but not SANITY can be
run by root (I am not saying that running tests as root is safe or
sane, though) and are expected to produce the same result as they
were run by a non-root user.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/test-lib.sh | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 446d8d5..68c31ae 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -997,20 +997,28 @@ test_lazy_prereq NOT_ROOT '
 	test "$uid" != 0
 '
 
-# On a filesystem that lacks SANITY, a file can be deleted even if
-# the containing directory doesn't have write permissions, or a file
-# can be accessed even if the containing directory doesn't have read
-# or execute permissions, causing our tests that validate that Git
-# works sensibly in such situations.
+# SANITY is about "can you correctly predict what the filesystem would
+# do by only looking at the permission bits of the files and
+# directories?"  A typical example of !SANITY is running the test
+# suite as root, where a test may expect "chmod -r file && cat file"
+# to fail because file is supposed to be unreadable after a successful
+# chmod.  In an environment (i.e. combination of what filesystem is
+# being used and who is running the tests) that lacks SANITY, you may
+# be able to delete or create a file when the containing directory
+# doesn't have write permissions, or access a file even if the
+# containing directory doesn't have read or execute permissions.
+
 test_lazy_prereq SANITY '
 	mkdir SANETESTD.1 SANETESTD.2 &&
 
 	chmod +w SANETESTD.1 SANETESTD.2 &&
 	>SANETESTD.1/x 2>SANETESTD.2/x &&
 	chmod -w SANETESTD.1 &&
+	chmod -r SANETESTD.1/x &&
 	chmod -rx SANETESTD.2 ||
 	error "bug in test sript: cannot prepare SANETESTD"
 
+	! test -r SANETESTD.1/x &&
 	! rm SANETESTD.1/x && ! test -f SANETESTD.2/x
 	status=$?
 

Re: [PATCH v4 2/2] interpret-trailers: add option for in-place editing

[Eric Sunshine]

On Tue, Jan 19, 2016 at 5:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Subject: test-lib: clarify and tighten SANITY
>
> f400e51c (test-lib.sh: set prerequisite SANITY by testing what we
> really need, 2015-01-27) improved the way SANITY prerequisite was
> determined, but made the resulting code (incorrectly) imply that
> SANITY is all about effects of permission bits of the containing
> directory has on the files contained in it by the comment it added,
> its log message and the actual tests.
>
> By the way, while we are on the subject, POSIXPERM is more about "if
> we do chmod, does filesystem remember it so that ls -l reports the
> same?"  Output from "git grep POSIXPERM t" shows that some users of
> it also assume that it requires "we can make something executable by
> doing chmod +x and unexecutable by doing chmod -x" (and that is
> fine--running tests as root would not make an unexecutable file
> executable).  The tests that require POSIXPERM but not SANITY can be
> run by root (I am not saying that running tests as root is safe or
> sane, though) and are expected to produce the same result as they
> were run by a non-root user.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> +# SANITY is about "can you correctly predict what the filesystem would
> +# do by only looking at the permission bits of the files and
> +# directories?"  A typical example of !SANITY is running the test
> +# suite as root, where a test may expect "chmod -r file && cat file"
> +# to fail because file is supposed to be unreadable after a successful
> +# chmod.  In an environment (i.e. combination of what filesystem is
> +# being used and who is running the tests) that lacks SANITY, you may
> +# be able to delete or create a file when the containing directory
> +# doesn't have write permissions, or access a file even if the
> +# containing directory doesn't have read or execute permissions.

This makes the intent much clearer. Thanks.

>  test_lazy_prereq SANITY '
>         mkdir SANETESTD.1 SANETESTD.2 &&
>
>         chmod +w SANETESTD.1 SANETESTD.2 &&
>         >SANETESTD.1/x 2>SANETESTD.2/x &&
>         chmod -w SANETESTD.1 &&
> +       chmod -r SANETESTD.1/x &&
>         chmod -rx SANETESTD.2 ||
>         error "bug in test sript: cannot prepare SANETESTD"
>
> +       ! test -r SANETESTD.1/x &&
>         ! rm SANETESTD.1/x && ! test -f SANETESTD.2/x
>         status=$?