On 2016-01-06 at 20:02:23 +0100, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Wed, Jan 6, 2016 at 8:34 AM, Tobias Klauser
> <tobias.klauser@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html