Re: [PATCH v3 2/2] interpret-trailers: add option for in-place editing
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jan 13, 2016, 18:15 UTC
- Message-ID
- <CAPig+cSX04OSLQVk-LHm2UQOAqThQUJuFhuhUfoNmUvHmZpPsA@mail.gmail.com>
- In-Reply-To
- <1452688989-13746-3-git-send-email-tklauser@distanz.ch>
On Wed, Jan 13, 2016 at 7:43 AM, Tobias Klauser <tklauser@distanz.ch> wrote:
Show 14 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 > > 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?
Show 7 quoted lines
> + 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.
Show 29 quoted lines
> 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.)
Show 11 quoted lines
> /* 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); > }