Thanks for your feedback Matthieu!
On 2016-01-06 at 15:19:45 +0100, Matthieu Moy <matthieu....@grenoble-inp.fr>
wrote:
> Tobias Klauser <tobias.klau...@zhinst.com> writes:
>
> > From: Tobias Klauser <tklau...@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.
Yes, I thought this would mainly be used in scripts and thus omitted the
short option.
> > 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.
Ok, I can omit it for v2.
> > -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.
Ok. I have also no problem with splitting it. I'll wait for a feedback
from Junio on what he prefers.
> > -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!
Yes, makes sense. I'll change it to a more concise and readable name
I'd also take "outfile" as you suggest, unless anyone objects.
Thanks
Tobias
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
