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

[Tobias Klauser <tklauser@distanz.ch>]

On 2016-01-13 at 19:15:54 +0100, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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?

Agree, will change in v4.

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

Good point. Yes, such a test should definitely be added, I forgot about
it during the reroll. I'll look into your suggestion and add a test in
v4.

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

No, I haven't considered this yet. But I agree that moving this code to
a separate function certainly will keep process_trailers() much more
readable. Also it would simplify future reusability if anyone else would
need similar functionality.

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

Ok, I'll drop the braces for v4.

Thanks a lot for your review!