Brandon Casey <drafnel@xxxxxxxxx> writes:
> Before commit 33f2f9ab, 'commit -s' would populate the edit buffer with
> a blank line before the Signed-off-by line. This provided a nice
> hint to the user that something should be filled in. Let's restore that
> behavior, but now let's ensure that the Signed-off-by line is preceded
> by two blank lines to hint that something should be filled in, and that
> a blank line should separate it from the Signed-off-by line.
>
> Plus, add a test for this behavior.
>
> Reported-by: John Keeping <john@xxxxxxxxxxxxx>
> Signed-off-by: Brandon Casey <drafnel@xxxxxxxxx>
> ---
>
> Ok. Here's a patch on top of 959a2623 bc/append-signed-off-by. It
> implements the "2 blank lines preceding sob" behavior.
>
> -Brandon
>
> sequencer.c | 5 +++--
> t/t7502-commit.sh | 12 ++++++++++++
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 53ee49a..2dac106 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1127,9 +1127,10 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
> const char *append_newlines = NULL;
> size_t len = msgbuf->len - ignore_footer;
>
> - if (len && msgbuf->buf[len - 1] != '\n')
> + /* ensure a blank line precedes our signoff */
> + if (!len || msgbuf->buf[len - 1] != '\n')
> append_newlines = "\n\n";
> - else if (len > 1 && msgbuf->buf[len - 2] != '\n')
> + else if (len == 1 || msgbuf->buf[len - 2] != '\n')
> append_newlines = "\n";
Maybe I am getting slower with age, but it took me 5 minutes of
staring the above to convince me that it is doing the right thing.
The if/elseif cascade is dealing with three separate things and the
logic is a bit dense:
* Is the buffer completely empty? We need to add two LFs to give a
room for the title and body;
* Otherwise:
- Is the final line incomplete? We need to add one LF to make it a
complete line whatever we do.
- Is the final line an empty line? We need to add one more LF to
make sure we have a blank line before we add S-o-b.
I wondered if we can rewrite it to make the logic clearer (that is
where I spent most of the 5 minutes), but I did not think of a
better way; probably the above is the best we could do.
Thanks.
By the way, I think we would want to introduce a symbolic constants
for the possible return values from has_conforming_footer(). The
check that appears after this hunk
if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
sob.buf, sob.len);
is hard to grok without them.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html