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

[Tobias Klauser <tklauser@distanz.ch>]

On 2016-01-06 at 20:02:23 +0100, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jan 6, 2016 at 8:34 AM, Tobias Klauser
> <tobias.klauser@zhinst.com> 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
> >
> > Also add the corresponding documentation and tests.
> 
> In addition to Matthieu's comments...
> 
> > 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.

Indeed, I didn't consider this. Thanks a lot for pointing this out.

> 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.

Ok, will do this for v2. I guess with the help of the functions from
tempfile.h it should be fairly easy to implement...

Thanks for your review!