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

[Tobias Klauser <tklauser@distanz.ch>]

Thanks for your feedback Matthieu!

On 2016-01-06 at 15:19:45 +0100, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> 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.

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