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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 6, 2016, 19:02 UTC
Message-ID: <CAPig+cSvD-TAd6365wxycUcTBehL-81a0WWosz4mVr1-HKCqGQ@mail.gmail.com>
In-Reply-To: <1452087258-3893-1-git-send-email-tobias.klauser@zhinst.com>

On Wed, Jan 6, 2016 at 8:34 AM, Tobias Klauser <tobias.klauser@zhinst.com> wrote:

Show 10 quoted lines

> 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
>
> Also add the corresponding documentation and tests.

In addition to Matthieu's comments...

Show 12 quoted lines

> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> ---
> diff --git a/trailer.c b/trailer.c
> @@ -856,19 +858,28 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai
>
>         lines = read_input_file(file);
>
> +       if (in_place) {
> +               fp = fopen(file, "w");
> +               if (!fp)
> +                       die_errno(_("could not open file '%s' for writing"), file);
> +       }

The input file should be considered precious, but this implementation plays too loosely with it. If the user interrupts the program or a die() somewhere within the "trailers" code aborts the program before the output file is written, then the original file will be irrecoverably lost. Users won't be happy about that.

Instead, this code should go through the standard dance of writing the output to a new file (with some unique temporary name) and then, only once the output has been successfully written in full, rename the new file atop the old.

Show 22 quoted lines

>         /* Print the lines before the trailers */
> -       trailer_end = process_input_file(lines, &in_tok_first, &in_tok_last);
> +       trailer_end = process_input_file(fp, 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(fp, 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(fp, lines, trailer_end, INT_MAX);
> +
> +       if (in_place)
> +               fclose(fp);
>
>         strbuf_list_free(lines);
>  }