for_each_reflog_ent_reverse: fix newlines on block boundaries

When we read a reflog file in reverse, we read whole chunks
of BUFSIZ bytes, then loop over the buffer, parsing any
lines we find. We find the beginning of each line by looking
for the newline from the previous line. If we don't find
one, we know that we are either at the beginning of
the file, or that we have to read another block.

In the latter case, we stuff away what we have into a
strbuf, read another block, and continue our parse. But we
missed one case here. If we did find a newline, and it is at
the beginning of the block, we must also stuff that newline
into the strbuf, as it belongs to the block we are about to
read.

The minimal fix here would be to add this special case to
the conditional that checks whether we found a newline.
But we can make the flow a little clearer by rearranging a
bit: we first handle lines that we are going to show, and
then at the end of each loop, stuff away any leftovers if
necessary. That lets us fold this special-case in with the
more common "we ended in the middle of a line" case.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/refs.c b/refs.c
index 5e5a3824b992132aaac8d86698feebcb773e13c6..8ed9aee939a2a5a3e1f67be91e8a5ba6049e2eb6 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -3089,24 +3089,49 @@ int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn, void
 
                        bp = find_beginning_of_line(buf, scanp);
 
-                       if (*bp != '\n') {
-                               strbuf_splice(&sb, 0, 0, buf, endp - buf);
-                               if (pos)
-                                       break; /* need to fill another block */
-                               scanp = buf - 1; /* leave loop */
-                       } else {
+                       if (*bp == '\n') {
                                /*
-                                * (bp + 1) thru endp is the beginning of the
-                                * current line we have in sb
+                                * The newline is the end of the previous line,
+                                * so we know we have complete line starting
+                                * at (bp + 1). Prefix it onto any prior data
+                                * we collected for the line and process it.
                                 */
                                strbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 1));
                                scanp = bp;
                                endp = bp + 1;
+                               ret = show_one_reflog_ent(&sb, fn, cb_data);
+                               strbuf_reset(&sb);
+                               if (ret)
+                                       break;
+                       } else if (!pos) {
+                               /*
+                                * We are at the start of the buffer, and the
+                                * start of the file; there is no previous
+                                * line, and we have everything for this one.
+                                * Process it, and we can end the loop.
+                                */
+                               strbuf_splice(&sb, 0, 0, buf, endp - buf);
+                               ret = show_one_reflog_ent(&sb, fn, cb_data);
+                               strbuf_reset(&sb);
+                               break;
                        }
-                       ret = show_one_reflog_ent(&sb, fn, cb_data);
-                       strbuf_reset(&sb);
-                       if (ret)
+
+                       if (bp == buf) {
+                               /*
+                                * We are at the start of the buffer, and there
+                                * is more file to read backwards. Which means
+                                * we are in the middle of a line. Note that we
+                                * may get here even if *bp was a newline; that
+                                * just means we are at the exact end of the
+                                * previous line, rather than some spot in the
+                                * middle.
+                                *
+                                * Save away what we have to be combined with
+                                * the data from the next read.
+                                */
+                               strbuf_splice(&sb, 0, 0, buf, endp - buf);
                                break;
+                       }
                }
 
        }

diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 236b13a3ab27f54808fa88574738747e3c3936b1..e582c0182bba81316352dc69e1cc7c6b664c68e9 100755 (executable)
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -245,4 +245,34 @@ test_expect_success 'gc.reflogexpire=false' '
 
 '
 
+# Triggering the bug detected by this test requires a newline to fall
+# exactly BUFSIZ-1 bytes from the end of the file. We don't know
+# what that value is, since it's platform dependent. However, if
+# we choose some value N, we also catch any D which divides N evenly
+# (since we will read backwards in chunks of D). So we choose 8K,
+# which catches glibc (with an 8K BUFSIZ) and *BSD (1K).
+#
+# Each line is 114 characters, so we need 75 to still have a few before the
+# last 8K. The 89-character padding on the final entry lines up our
+# newline exactly.
+test_expect_success 'parsing reverse reflogs at BUFSIZ boundaries' '
+       git checkout -b reflogskip &&
+       z38=00000000000000000000000000000000000000 &&
+       ident="abc <xyz> 0000000001 +0000" &&
+       for i in $(test_seq 1 75); do
+               printf "$z38%02d $z38%02d %s\t" $i $(($i+1)) "$ident" &&
+               if test $i = 75; then
+                       for j in $(test_seq 1 89); do
+                               printf X
+                       done
+               else
+                       printf X
+               fi &&
+               printf "\n"
+       done >.git/logs/refs/heads/reflogskip &&
+       git rev-parse reflogskip@{73} >actual &&
+       echo ${z38}03 >expect &&
+       test_cmp expect actual
+'
+
 test_done