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

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Jan 13, 2016 at 7:43 AM, Tobias Klauser <tklauser@distanz.ch> wrote:
> 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>
> ---
> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> @@ -326,6 +326,38 @@ test_expect_success 'with complex patch, args and --trim-empty' '
> +test_expect_success 'in-place editing on stdin' '

Maybe say:

    "in-place editing on stdin disallowed"

or something?

> +       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
> +'

An significant shortcoming of the first version of this patch series
was that it did not treat the input file as precious, and would
happily delete it if trailer processing failed for any reason. This is
behavior we'd like to protect against. Toward that end, have you
considered adding a test to verify that the input file is indeed
considered precious, and not deleted upon failure? One way to do so
would be to write a test which triggers one of the failure conditions
of the interpret-trailers framework. Looking at the source code, one
possible way would be to make trailer.c:read_input_file() fail, for
instance, by making the file unreadable (with 'chmod -r', though you'd
have to protect the test with the POSIXPERM prerequisite).

More below.

> diff --git a/trailer.c b/trailer.c
> @@ -858,6 +861,31 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai
>
>         lines = read_input_file(file);
>
> +       if (in_place) {
> +               struct stat st;
> +               struct strbuf template = STRBUF_INIT;
> +               const char *tail;
> +
> +               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"));
> +       }

Hmm, the current logic of process_trailers() is pretty easily
understood at a glance, but this new (relatively huge) chunk of code
obscures the overall operation. Have you considered factoring the new
code out into its own function in order to keep the overall flow of
process_trailers() clean? (Genuine question; I don't necessarily feel
so strongly about it to demand such a change.)

>         /* Print the lines before the trailers */
>         trailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);
>
> @@ -872,5 +900,10 @@ 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);
> +       }

You could drop the unnecessary braces.

>         strbuf_list_free(lines);
>  }