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

[Pierre Habouzit <madcoder@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 3190 bytes --]

On Sun, Nov 11, 2007 at 08:42:48PM +0000, Johannes Schindelin wrote:
> 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.

  I know that you don't use it, but who knows where this code will be
headed in its future right ? :) It's just good practice.

> > > +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?

  Well, you can make it generic enough using the defval to point to a
delimiter to use between lines :) But it's probably an overkill, and yes
if the "\n\n" is to be kept, then it's not generic "enough".

> 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.

  Right.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]