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

From: Pierre Habouzit <[email-masked]>
Date: Sun, 11 Nov 2007 20:42:28 +0100

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 <[email-masked]>
> ---
>  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`.

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

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

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

-- 
·O·  Pierre Habouzit
··O                                                [email-masked]
OOO                                                http://www.madism.org