Re: [PATCH] interpret-trailers: add option for in-place editing
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Jan 6, 2016, 14:19 UTC
- Message-ID
- <vpqoacylri6.fsf@anie.imag.fr>
- In-Reply-To
- <1452087258-3893-1-git-send-email-tobias.klauser@zhinst.com>
Tobias Klauser <tobias.klauser@zhinst.com> writes:
> From: Tobias Klauser <tklauser@distanz.ch> > > Add a command line option --in-place to support in-place editing akin to > sed -i. This allows to write commands like the following:
Since -i is a common shortcut for --in-place (perl -i, sed -i), it probably makes sense to have it here too. OTOH, this is meant for scripting and perhaps it's best to force script writters to be verbose.
> Also add the corresponding documentation and tests.
This sentence does not harm, but I personnally don't think it's really helpfull, as it's already clear by the diffstat right below, and the patch itself.
Show 13 quoted lines
> -static void print_tok_val(const char *tok, const char *val)
> +static void print_tok_val(FILE *fp, const char *tok, const char *val)
> {
> char c = last_non_space_char(tok);
> if (!c)
> return;
> if (strchr(separators, c))
> - printf("%s%s\n", tok, val);
> + fprintf(fp, "%s%s\n", tok, val);
> else
> - printf("%s%c %s\n", tok, separators[0], val);
> + fprintf(fp, "%s%c %s\n", tok, separators[0], val);
> }The patch would be even easier to read if split into a preparatory refactoring patch "turn printf into fprintf" and then the actual one. But it's already rather clear, so you can probably leave it as-is.
> -static void print_lines(struct strbuf **lines, int start, int end) > +static void print_lines(FILE *fp, struct strbuf **lines, int start, int end)
Here and below: it would probably be more readable with a more explicit name for fp, like "outfile". Especially here:
> -static int process_input_file(struct strbuf **lines, > +static int process_input_file(FILE *fp, > + struct strbuf **lines,
Where it's tempting to think that a parameter given to process_input_file is ... the input file!
Other than these minor details, the patch looks good to me. Thanks for the patch!
-- Matthieu Moy http://www-verimag.imag.fr/~moy/