Re: [PATCH 1/3] merge: allow reading the merge commit message from a file

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

Hi Junio,

On Wed, 11 Jul 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > diff --git a/builtin/merge.c b/builtin/merge.c
> > index 4a4c09496..b0e907751 100644
> > --- a/builtin/merge.c
> > +++ b/builtin/merge.c
> > @@ -111,6 +111,35 @@ static int option_parse_message(const struct option *opt,
> >  	return 0;
> >  }
> >  
> > +static int option_read_message(struct parse_opt_ctx_t *ctx,
> > +			       const struct option *opt, int unset)
> > +{
> > +	struct strbuf *buf = opt->value;
> > +	const char *arg;
> > +
> > +	if (unset)
> > +		BUG("-F cannot be negated");
> 
> The message "-F cannot be negated" looks as if it is pointing out a
> mistake by the end user, and does not mesh well with the real reason
> why this is BUG() and is not die().
> 
> I understand that this is BUG() not die() because options[] array
> tells this callback not to be called with unset by having the
> PARSE_OPT_NONEG bit there.

Okay. I would have appreciated some sort of indication what you prefer
instead. I went with "--no-file?!?"

> > +	if (ctx->opt) {
> > +		arg = ctx->opt;
> > +		ctx->opt = NULL;
> > +	} else if (ctx->argc > 1) {
> > +		ctx->argc--;
> > +		arg = *++ctx->argv;
> > +	} else
> > +		return opterror(opt, "requires a value", 0);
> > +
> > +	if (buf->len)
> > +		strbuf_addch(buf, '\n');
> 
> Do we assume that buf, if it is not empty, is properly terminated
> with LF already?  I am wondering if the real reason we do these two
> lines is to make sure we have a separating blank line between what
> is already there (if there already is something) and what we add, in
> which case the above would want to say
> 
> 	if (buf->len) {
> 		strbuf_complete_line(buf);
> 		strbuf_addch(buf, '\n');
> 	}
> 
> instead.

True. Thanks for the suggestion!

> > +	if (ctx->prefix && !is_absolute_path(arg))
> > +		arg = prefix_filename(ctx->prefix, arg);
> > +	if (strbuf_read_file(buf, arg, 0) < 0)
> > +		return error(_("could not read file '%s'"), arg);
> > +	have_message = 1;
> 
> A similar question is what we would want to do when the file ends
> with an incomplete line.  With "--log", we would be appending more
> stuff to buf, and we'd want to complete such an incomplete line
> before that happens, either here or in the code immediately before
> "--log" is processed.

This is what I inserted here:

	strbuf_complete_line(buf);

> > +
> > +	return 0;
> > +}
> > +
> >  static struct strategy *get_strategy(const char *name)
> >  {
> >  	int i;

Thanks for the review, and especially for the suggestions how to improve
the code.

Ciao,
Dscho