Skip to content

Making an elephant out of a getline() bug

Thomas Rast git 1 version In review

Get series with b4
b4 am [email protected]

Versions newest first

  1. v2 Latest4 patches 3 repliesIn review

Patches in v2 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/4strbuf: improve strbuf_get*line documentationDocumentation/technical/api-strbuf.txt+10 -21 reply
2/4bundle: put strbuf_readline_fd in strbuf.c with adjustmentsDocumentation/technical/api-strbuf.txt bundle.c strbuf.c +1 more+26 -190 replies
3/4t5704: match tests to modern stylet/t5704-bundle.sh+8 -251 reply
4/4bundle: use a strbuf to scan the log for boundary commitsbundle.c t/t5704-bundle.sh+24 -70 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v2 yet.

Read the v2 thread

Cover letter [PATCH v2 0/4] Making an elephant out of a getline() bug

So you all made very good points, and I don't want to repeat them. Junio's analysis

Quoted 6 lines

Perhaps we would want to squash something like this to the test to avoid "seq", using J6t's idea. The issue is that we do not write the end of line for the long boundary (because it is hidden from us), keep reading and rejecting bogus letters, and then the tip is written without terminating the boundary line. So checking boundary line is not a very useful test, but "fetch" and "list-heads" are.

is of course also correct for the case where the overlong line is in a boundary commit. (The analysis about the accidentally-leading '-' is also correct and can create a boundary line where there was none, like in the perl case.)

The patches in this round are:

[1] new; following up on Peff's complaint that I should document strbuf_getwholeline_fd, I found that the strbuf_get*line documentation was also lacking.

[2] previously (1/2), now with documentation added

[3] new; Junio corrected me on the ': > file' spelling, which I added because the start of the test used it too. So let's just fix it outright to match the Git style all over.

[4] previously (2/2), now incorporating a strbuf_release and the suggestions for the tests as sent by Junio. I upped the %0982 a bit as it felt *too* tailored for the old bug, and this way everyone can see immediately that it will exceed the 1024 char limit (without thinking about the length of a sha and accounting for \0 and \n).

Thomas Rast (4):
strbuf: improve strbuf_get*line documentation
bundle: put strbuf_readline_fd in strbuf.c with adjustments
t5704: match tests to modern style
bundle: use a strbuf to scan the log for boundary commits

 Documentation/technical/api-strbuf.txt |   19 +++++++++++--
 bundle.c                               |   36 +++++++-----------------
 strbuf.c                               |   16 +++++++++++
 strbuf.h                               |    1 +
 t/t5704-bundle.sh                      |   47 ++++++++++++++++----------------
 5 files changed, 67 insertions(+), 52 deletions(-)
Signature · 1 line
1.7.9.1.430.g4998543