Hi Junio,
On Wed, 11 Jul 2018, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgad...@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
