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

Johannes Schindelin [off-list ref] writes:

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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