Re: [PATCH 2/2] merge: make sure to terminate message with newline

From: Jeff King <peff@peff.net>
Date: Jul 17, 2021, 17:47 UTC
Message-ID: <YPMXqLESKApq2uPI@coredump.intra.peff.net>
In-Reply-To: <693954a7-af64-67c5-41b9-b648a9fe3ef2@gmail.com>

On Sat, Jul 17, 2021 at 02:40:55PM +0100, Phillip Wood wrote:

Show 10 quoted lines

> > which would cover that case. But Phillip mentioned that our own
> > fmt_merge_msg() does not include a newline.
> 
> I mentioned that we remove the newline that is added by fmt_merge_msg(), not
> that there is no newline added by fmt_merge_msg() - maybe I wasn't clear
> enough. In builtin/merge.c:prepare_merge_message() we do
> 
> 	fmt_merge_msg(merge_names, merge_msg, &opts);
> 	if (merge_msg->len)
> 		strbuf_setlen(merge_msg, merge_msg->len - 1);

Of maybe I didn't read carefully enough. :)

Either way, thanks for clarifying. Doing something like:

  cat >.git/hooks/commit-msg <<\EOF
  #!/bin/sh
  xxd "$1"
  EOF
  chmod +x .git/hooks/commit-msg

  git merge --no-edit ...

shows off the problem; the hook sees that intermediate state.

Likewise if we do:

  git merge -m "foo" ...

which similarly suppresses the editor. There are actually two interesting cases here:

  - if merge.log is not set, then we'd see "foo" with no newline

  - if it is set, we'll get a newline after "foo", but with no newline
    after the log data

Likewise for:

  printf foo >no-newline
  git merge -F no-newline ...

So I think we'd probably want to see a 3-patch series:

  1. Make interpret-trailers handle input missing the final newline.
     This isn't strictly necessary after patches 2 and 3, but it makes
     sense to be more robust with unexpected input.

  2. Drop the newline-stripping from prepare_merge_message(). The
     examples above show some ways we could cover this in the tests.
     This will help --no-edit case, but also using merge.log with "-m"
     or "-F".

  3. Teach prepare_to_commit() to add the extra newline before letting
     hooks see the message. This should probably be done only when
     cleanup_mode != COMMIT_MSG_CLEANUP_NONE.

Luca, do you want to try revising your series in that direction?

-Peff