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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 12, 2018, 16:20 UTC
Message-ID: <xmqqpnzszar0.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.1807121455130.75@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 19 quoted lines

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

I have no strong preference; anything is OK as long as the message is unique and points reading developer in the right direction, and "--no-file?!?" signals quite strongly that the code is not expected that it has to handle that option at this point (instead, it expects somebody else has dealt with it), so it sounds fine.

I think doing all of these inside parse_options callback means that you can have "merge -F file1 -F file2" and slurp contents from both files as separate paragraphs. I briefly wondered if --no-file is something the end user might want to be able to use to discard what has been read so far, but "merge -m msg -F file --no-file" would have to discard everything, not just what we read from the file, so it would not be useful with the structure of the message assembly we have today, which this code builds on.

Show 42 quoted lines

>> > +	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);

I had a slight suspicion that completing immediately before we append anything in a later step in the codepath would be safer. When we get a complaint:

    'merge -F file' when I am not using '--log' or adding sign-off,
    adds an extra newline at the end when I deliberately give a file
    that ends with an incomplete line for such and such reasons.

I do not think I would have a good argument why the then-current behaviour is not a bug but an intended behaviour.

And I do not think the fact that I am unable to fill "such and such" above means such a complaint is nonsense---it merely indicates that I lack imagination and that I am not thinking enough to accomodate other people's needs.