fmt-merge-message: add empty line between tag and signature verification

fmt-merge-message: add empty line between tag and signature verification

[Linus Torvalds]

When adding the information from a tag, put an empty line between the 
message of the tag and the commented-out signature verification 
information.

At least for the kernel workflow, I often end up re-formatting the message 
that people send me in the tag data. In that situation, putting the tag 
message and the tag signature verification back-to-back then means that 
normal editor "reflow parapgraph" command will get confused and think that 
the signature is a continuation of the last message paragraph.

So I always end up having to first add an empty line, and then go back and 
reflow the last paragraph. Let's just do it in git directly.

The extra vertical space also makes the verification visually stand out 
more from the user-supplied message, so it looks a bit more readable to me 
too, but that may be just an odd personal preference.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
This is a throw-away patch - apply or not as you prefer. I thought I'd 
send it out and see what people thought.

I don't feel *that* strongly about it.

Btw, I'd also like to see the merge notes (notably the conflict file list) 
before the generated shortlog, but that seems to really not work with the 
current fmt-merge-message model.  Oh well.

And the strbuf_complete_line() change is entirely independent, but didn't 
seem worth an extra separate patch.  Feel free to take that out, or do it 
independently or whatever.

 builtin/fmt-merge-msg.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index a517f1794a1c..d42015d8672d 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -462,7 +462,10 @@ static void fmt_tag_signature(struct strbuf *tagbuf,
 		strbuf_add(tagbuf, tag_body, buf + len - tag_body);
 	}
 	strbuf_complete_line(tagbuf);
-	strbuf_add_lines(tagbuf, "# ", sig->buf, sig->len);
+	if (sig->len) {
+		strbuf_addch(tagbuf, '\n');
+		strbuf_add_lines(tagbuf, "# ", sig->buf, sig->len);
+	}
 }
 
 static void fmt_merge_msg_sigs(struct strbuf *out)
@@ -627,8 +630,7 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
 		rev.ignore_merges = 1;
 		rev.limited = 1;
 
-		if (suffixcmp(out->buf, "\n"))
-			strbuf_addch(out, '\n');
+		strbuf_complete_line(out);
 
 		for (i = 0; i < origins.nr; i++)
 			shortlog(origins.items[i].string,

Re: fmt-merge-message: add empty line between tag and signature verification

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> When adding the information from a tag, put an empty line between the 
> message of the tag and the commented-out signature verification 
> information.
>
> At least for the kernel workflow, I often end up re-formatting the message 
> that people send me in the tag data. In that situation, putting the tag 
> message and the tag signature verification back-to-back then means that 
> normal editor "reflow parapgraph" command will get confused and think that 
> the signature is a continuation of the last message paragraph.
>
> So I always end up having to first add an empty line, and then go back and 
> reflow the last paragraph. Let's just do it in git directly.
>
> The extra vertical space also makes the verification visually stand out 
> more from the user-supplied message, so it looks a bit more readable to me 
> too, but that may be just an odd personal preference.
>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
> This is a throw-away patch - apply or not as you prefer. I thought I'd 
> send it out and see what people thought.
>
> I don't feel *that* strongly about it.

Speaking as one of "people" (as opposed to "the maintainer to decide one
way or the other in bikeshedding"), I think this makes sense.

> Btw, I'd also like to see the merge notes (notably the conflict file list) 
> before the generated shortlog, but that seems to really not work with the 
> current fmt-merge-message model.  Oh well.

Postponing to assess if such a change is feasible, I am not sure if that
ordering makes more sense than the current one.  Is the objective to more
strongly motivate people to explain what happened to the conflicts?

Re: fmt-merge-message: add empty line between tag and signature verification

[Linus Torvalds]

On Fri, May 25, 2012 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Btw, I'd also like to see the merge notes (notably the conflict file list)
>> before the generated shortlog, but that seems to really not work with the
>> current fmt-merge-message model.  Oh well.
>
> Postponing to assess if such a change is feasible, I am not sure if that
> ordering makes more sense than the current one.  Is the objective to more
> strongly motivate people to explain what happened to the conflicts?

Yes. Right now, the conflict line is hidden way at the bottom, and
I've actually overlooked it several times (and I don't think I'm the
only one - do "git log --grep=Conflicts:" on the kernel tree and
you'll see a lot of people just leaving them be).

And even when I *don't* overlook it, it's actually annoying to go to
them and edit them. They're not at the very end (that's the big
comment about merging), and due to the shortlog the conflicts are
usually listed at least one screenful down.

And if you *don't* edit them, they end up pretty ugly and useless. I
like to at the very least make them a bit more compact and readable,
ie something like

    Fix up trivial conflicts in arch/arm/mach-ux500/{Makefile,board-mop500.c}

instead of listing them individually, but preferably actually talk
about what the conflicts were about, eg

    Fix up add-add onflicts in arch/x86/xen/enlighten.c due to addition of
    apic ipi interface next to the new apic_id functions.

which I think makes it more worthwhile to talk about the conflicts.

In fact, I think that if you *don't* edit them at all, you might as
well just leave the conflict data out entirely, and it might be worth
making the automatic conflict information be commented by default. But
then it *really* needs to be above the shortlog so that it's not
hidden away.

That said, this is all "minor tweaking". The current thing certainly
works. I just think it could be better. Clearly a lot of people do not
edit the conflict information even when it is right there (see the
same "git log --grep" above), and it looks like I'm sadly alone in
trying to make good merge messages.

                   Linus

Re: fmt-merge-message: add empty line between tag and signature verification

[Martin Fick]

On Friday, May 25, 2012 11:20:10 am Linus Torvalds wrote:
> On Fri, May 25, 2012 at 10:05 AM, Junio C Hamano 
<gitster@pobox.com> wrote:
> >> Btw, I'd also like to see the merge notes (notably the
> >> conflict file list) before the generated shortlog,
> >> but that seems to really not work with the current
> >> fmt-merge-message model.  Oh well.
> > 
> Yes. Right now, the conflict line is hidden way at the
> bottom, and I've actually overlooked it several times
> (and I don't think I'm the only one - do "git log
> --grep=Conflicts:" on the kernel tree and you'll see a
> lot of people just leaving them be).

Tangentially related, I think that in the cherry-pick case 
this can also be a problem. Not exactly the same, but in the 
cherry-pick case you are generally starting with an existing 
commit message, and the location of the conflicts at the 
bottom ends up actually causing functional problems for the 
interpretations of the old "footer-lines" by some tools.  

Notably, our users sometimes cherry-pick gerrit changes but 
then the old Change-Id footer line gets placed above the 
conflicts and if they forget to remove the conflicts, the 
Change-Id footer is not recognized by Gerrit.  

So, not only are users forgetting to remove the conflicts, 
it causes tooling problems in the cherry-pick case by 
messing with the footer layouts,

-Martin

-- 
Employee of Qualcomm Innovation Center, Inc. which is a 
member of Code Aurora Forum

Re: fmt-merge-message: add empty line between tag and signature verification

[Ralf Thielow]

On Fri, May 25, 2012 at 7:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Btw, I'd also like to see the merge notes (notably the conflict file list)
>> before the generated shortlog, but that seems to really not work with the
>> current fmt-merge-message model.  Oh well.
>
> Postponing to assess if such a change is feasible, I am not sure if that
> ordering makes more sense than the current one.  Is the objective to more
> strongly motivate people to explain what happened to the conflicts?

IMHO it only makes sense when you have a short list of conflicted files.
I mostly work on Java projects and when we merge a branch this list can
be very long. This is caused by several tools, different IDEs, different OSes
and so on and so forth. Anyway...
What I want to see on a merge like this is the shortlog. I only become
interested in
the conflicts if there is an issue in the software caused by a
conflict resolution.
Just my experience on Java projects I'm working on.