Re: [PATCH 5/6] builtin-commit: resurrect behavior for multiple -m options

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Sun, 11 Nov 2007, Pierre Habouzit wrote:

> On Sun, Nov 11, 2007 at 05:36:39PM +0000, Johannes Schindelin wrote:
> > 
> > When more than one -m option is given, the message does not replace
> > the previous, but is appended.
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin-commit.c |   26 ++++++++++++++++++++------
> >  1 files changed, 20 insertions(+), 6 deletions(-)
> > 
> > diff --git a/builtin-commit.c b/builtin-commit.c
> > index 66d7e5e..069d180 100644
> > --- a/builtin-commit.c
> > +++ b/builtin-commit.c
> > @@ -30,13 +30,27 @@ static char *use_message_buffer;
> >  static const char commit_editmsg[] = "COMMIT_EDITMSG";
> >  static struct lock_file lock_file;
> >  
> > -static char *logfile, *force_author, *message, *template_file;
> > +static char *logfile, *force_author, *template_file;
> >  static char *edit_message, *use_message;
> >  static int all, edit_flag, also, interactive, only, amend, signoff;
> >  static int quiet, verbose, untracked_files, no_verify;
> >  
> >  static int no_edit, initial_commit, in_merge;
> >  const char *only_include_assumed;
> > +struct strbuf message;
> 
>   Unless I'm mistaken `static` keywords are missign for`message` and
> `only_include_assumed`.

Oh yeah.  Will fix.

>   And you _have_ to initialize message with STRBUF_INIT (remember of the
> slop).

Not in this case, since I do not use message.buf as long as message.len == 
0.  But I agree it would be cleaner to just use STRBUF_INIT.

> > +static int opt_parse_m(const struct option *opt, const char *arg, int unset)
> > +{
> > +	struct strbuf *buf = opt->value;
> > +	if (unset)
> > +		strbuf_setlen(buf, 0);
> > +	else {
> > +		strbuf_addstr(buf, arg);
> > +		strbuf_addch(buf, '\n');
> > +		strbuf_addch(buf, '\n');
> > +	}
> > +	return 0;
> > +}
> 
>   I believe such a callback could live in parse-options.[hc]. The need
> to aggregate all string arguments into a strbuf looks generic enough to
> me. Why are you adding two '\n' btw ? Isn't one enough ?

Well, this empty line is needed to stay backwards compatible.  It was 
added to pass the test that Junio added to 'next'.  As such, this function 
is not really generic enough, right?

>   Oh and last nitpicking, strbuf_addstr(buf, "\n\n"); is more efficient
> than the two addchar (the strlen it generates is inlined).

Well, I meant to mention it in the cover letter.  My preference is to do 
away with the extra empty line.  But this might break existing setups 
depending on that behaviour.

Ciao,
Dscho