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

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