Re: [PATCH 5/6] builtin-commit: resurrect behavior for multiple -m options
On Sun, Nov 11, 2007 at 08:42:48PM +0000, Johannes Schindelin wrote:
Show 42 quoted lines
> 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.
Show 20 quoted lines
> > > +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.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
Pierre Habouzit <madcoder@debian.org>