Re: [PATCH] git-commit: populate the edit buffer with 2 blank lines before s-o-b

Brandon Casey [off-list ref] writes:

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.

On Fri, Feb 22, 2013 at 10:35 AM, Junio C Hamano [off-list ref] wrote:

Brandon Casey [off-list ref] writes:

quoted

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.

We could unroll the conditionals into individual blocks and add your
comments from above like:

   if (!len) {
      /* The buffer is completely empty.  Leave room for the title and body. */
      append_newlines = "\n\n";
   } else if (msgbuf->buf[len - 1] != '\n') {
      /* Incomplete line.  Complete the line and add a blank one */
      append_newlines = "\n\n";
   } else if (len == 1) {
      /*
       * Buffer contains a single newline.  Add another so that we leave
       * room for the title and body.
       */
      append_newlines = "\n";
   } ...

Not sure that it will reduce the amount of time needed to understand
what's going on, but at least it describes the expectations made by
each block.

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.

Yeah, Jonathan said the same thing and I agree.  I was hoping someone
else would beat me to it.

-Brandon

From: Brandon Casey <redacted>

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 <redacted>
Signed-off-by: Brandon Casey <redacted>
---

How about something like this?

-Brandon

 sequencer.c       | 27 +++++++++++++++++++++++++--
 t/t7502-commit.sh | 12 ++++++++++++
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 53ee49a..a07d2d0 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1127,10 +1127,33 @@ 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')
+		if (!len) {
+			/*
+			 * The buffer is completely empty.  Leave foom for
+			 * the title and body to be filled in by the user.
+			 */
 			append_newlines = "\n\n";
-		else if (len > 1 && msgbuf->buf[len - 2] != '\n')
+		} else if (msgbuf->buf[len - 1] != '\n') {
+			/*
+			 * Incomplete line.  Complete the line and add a
+			 * blank one so that there is an empty line between
+			 * the message body and the sob.
+			 */
+			append_newlines = "\n\n";
+		} else if (len == 1) {
+			/*
+			 * Buffer contains a single newline.  Add another
+			 * so that we leave room for the title and body.
+			 */
+			append_newlines = "\n";
+		} else if (msgbuf->buf[len - 2] != '\n') {
+			/*
+			 * Buffer ends with a single newline.  Add another
+			 * so that there is an empty line between the message
+			 * body and the sob.
+			 */
 			append_newlines = "\n";
+		} /* else, the buffer already ends with two newlines. */
 
 		if (append_newlines)
 			strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index deb187e..a53a1e0 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh

@@ -349,6 +349,18 @@ test_expect_success 'A single-liner subject with a token plus colon is not a foo
 
 '
 
+test_expect_success 'commit -s places sob on third line after two empty lines' '
+	git commit -s --allow-empty --allow-empty-message &&
+	cat <<-EOF >expect &&
+
+
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+
+	EOF
+	egrep -v '^#' .git/COMMIT_EDITMSG >actual &&
+	test_cmp expect actual
+'
+
 write_script .git/FAKE_EDITOR <<\EOF
 mv "$1" "$1.orig"
 (

-- 
1.8.1.3.566.gaa39828

On Fri, Feb 22, 2013 at 02:05:27PM -0800, Brandon Casey wrote:

From: Brandon Casey <redacted>

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 <redacted>
Signed-off-by: Brandon Casey <redacted>
---

How about something like this?

FWIW, as a casual reader of this series, I find this to be way easier
to follow than the previous round.

-Peff