Tobias Klauser <tobias.klauser@xxxxxxxxxx> writes:
> From: Tobias Klauser <tklauser@xxxxxxxxxx>
>
> 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.
> -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/
--
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