threads / patch / 29706

v2, 4 partsstrbuf: improve strbuf_get*line documentation

Subject: [PATCH v2 1/4] strbuf: improve strbuf_get*line documentation

## tl;dr

8 messages between Feb 23, 2012 and Feb 23, 2012. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Thomas Rast· Feb 23, 2012, 09:42 UTC · lore

[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

Show 6 quoted 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(-)
-- 
1.7.9.1.430.g4998543
Thomas Rast· Feb 23, 2012, 09:42 UTC · re: Thomas Rast · lore

strbuf_getline() was not documented very clearly, though a reader familiar with getline() would not have had any questions about it. strbuf_getwholeline() was not documented at all.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/technical/api-strbuf.txt |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
Show changes to Documentation/technical/api-strbuf.txt +10 −2
diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
index afe2759..cc6db5e 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -255,8 +255,16 @@ same behaviour as well.
 
 `strbuf_getline`::
 
-	Read a line from a FILE* pointer. The second argument specifies the line
-	terminator character, typically `'\n'`.
+	Read a line from a FILE*. The second argument specifies the
+	line terminator character, typically `'\n'`.  Reading stops
+	after the terminator or at EOF.  The terminator is removed
+	from the buffer before returning.  Returns 0 unless there was
+	nothing left before EOF, in which case it returns `EOF`.
+
+`strbuf_getwholeline`::
+	
+	Like `strbuf_getline`, but keeps the trailing terminator (if
+	any) in the buffer.
 
 `stripspace`::
 

-- 
1.7.9.1.430.g4998543
Jeff King· Feb 23, 2012, 10:08 UTC · re: Thomas Rast · lore

Re: [PATCH v2 1/4] strbuf: improve strbuf_get*line documentation

On Thu, Feb 23, 2012 at 10:42:21AM +0100, Thomas Rast wrote:
> strbuf_getline() was not documented very clearly, though a reader
> familiar with getline() would not have had any questions about it.
> strbuf_getwholeline() was not documented at all.
Thanks for improving the existing docs. One suggestion:
Show 9 quoted lines
>  `strbuf_getline`::
>  
> -	Read a line from a FILE* pointer. The second argument specifies the line
> -	terminator character, typically `'\n'`.
> +	Read a line from a FILE*. The second argument specifies the
> +	line terminator character, typically `'\n'`.  Reading stops
> +	after the terminator or at EOF.  The terminator is removed
> +	from the buffer before returning.  Returns 0 unless there was
> +	nothing left before EOF, in which case it returns `EOF`.

The get*line functions are unlike the rest of the strbuf API in that they overwrite, rather than append to, the strbuf argument. Maybe:

  s/from a FILE\*/&, overwriting the existing contents of the strbuf/
?
-Peff
Thomas Rast· Feb 23, 2012, 09:42 UTC · re: Thomas Rast · lore

[PATCH v2 2/4] bundle: put strbuf_readline_fd in strbuf.c with adjustments

The comment even said that it should eventually go there. While at it, match the calling convention and name of the function to the strbuf_get*line family. So it now is strbuf_getwholeline_fd.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/technical/api-strbuf.txt |    7 +++++++
 bundle.c                               |   21 ++-------------------
 strbuf.c                               |   16 ++++++++++++++++
 strbuf.h                               |    1 +
 4 files changed, 26 insertions(+), 19 deletions(-)
Show changes to 4 files +26 −19

Documentation/technical/api-strbuf.txt, bundle.c, strbuf.c, strbuf.h

diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
index cc6db5e..827c4bd 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -266,6 +266,13 @@ same behaviour as well.
 	Like `strbuf_getline`, but keeps the trailing terminator (if
 	any) in the buffer.
 
+`strbuf_getwholeline_fd`::
+
+	Like `strbuf_getwholeline`, but operates on a file descriptor.
+	It reads one character at a time, so it is very slow.  Do not
+	use it unless you need the correct position in the file
+	descriptor!
+
 `stripspace`::
 
 	Strip whitespace from a buffer. The second parameter controls if
diff --git a/bundle.c b/bundle.c
index b8acf3c..313de42 100644
--- a/bundle.c
+++ b/bundle.c
@@ -23,23 +23,6 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,
 	list->nr++;
 }
 
-/* Eventually this should go to strbuf.[ch] */
-static int strbuf_readline_fd(struct strbuf *sb, int fd)
-{
-	strbuf_reset(sb);
-
-	while (1) {
-		char ch;
-		ssize_t len = xread(fd, &ch, 1);
-		if (len <= 0)
-			return len;
-		strbuf_addch(sb, ch);
-		if (ch == '\n')
-			break;
-	}
-	return 0;
-}
-
 static int parse_bundle_header(int fd, struct bundle_header *header,
 			       const char *report_path)
 {
@@ -47,7 +30,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 	int status = 0;
 
 	/* The bundle header begins with the signature */
-	if (strbuf_readline_fd(&buf, fd) ||
+	if (strbuf_getwholeline_fd(&buf, fd, '\n') ||
 	    strcmp(buf.buf, bundle_signature)) {
 		if (report_path)
 			error("'%s' does not look like a v2 bundle file",
@@ -57,7 +40,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 	}
 
 	/* The bundle header ends with an empty line */
-	while (!strbuf_readline_fd(&buf, fd) &&
+	while (!strbuf_getwholeline_fd(&buf, fd, '\n') &&
 	       buf.len && buf.buf[0] != '\n') {
 		unsigned char sha1[20];
 		int is_prereq = 0;
diff --git a/strbuf.c b/strbuf.c
index ff0b96b..5135d59 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -383,6 +383,22 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term)
 	return 0;
 }
 
+int strbuf_getwholeline_fd(struct strbuf *sb, int fd, int term)
+{
+	strbuf_reset(sb);
+
+	while (1) {
+		char ch;
+		ssize_t len = xread(fd, &ch, 1);
+		if (len <= 0)
+			return EOF;
+		strbuf_addch(sb, ch);
+		if (ch == term)
+			break;
+	}
+	return 0;
+}
+
 int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
 {
 	int fd, len;
diff --git a/strbuf.h b/strbuf.h
index fbf059f..3effaa8 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -116,6 +116,7 @@ static inline void strbuf_complete_line(struct strbuf *sb)
 
 extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
 extern int strbuf_getline(struct strbuf *, FILE *, int);
+extern int strbuf_getwholeline_fd(struct strbuf *, int, int);
 
 extern void stripspace(struct strbuf *buf, int skip_comments);
 extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);

-- 
1.7.9.1.430.g4998543
Thomas Rast· Feb 23, 2012, 09:42 UTC · re: Thomas Rast · lore

[PATCH v2 3/4] t5704: match tests to modern style

The test did not adhere to the current style on several counts:
- empty lines around the test blocks, but within the test string
- ': > file' or even just '> file' with an extra space
- inconsistent indentation
- hand-rolled commits instead of using test_commit

Fix all of them. There's a catch to the last point: test_commit creates a tag. We still change it to test_commit, and explicitly delete the tags, so as to highlight that the test relies on not having them.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/t5704-bundle.sh |   33 ++++++++-------------------------
 1 file changed, 8 insertions(+), 25 deletions(-)
Show changes to t/t5704-bundle.sh +8 −25
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index 4ae127d..f35f559 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -4,59 +4,42 @@ test_description='some bundle related tests'
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-
-	: > file &&
-	git add file &&
-	test_tick &&
-	git commit -m initial &&
+	test_commit initial &&
 	test_tick &&
 	git tag -m tag tag &&
-	: > file2 &&
-	git add file2 &&
-	: > file3 &&
-	test_tick &&
-	git commit -m second &&
-	git add file3 &&
-	test_tick &&
-	git commit -m third
-
+	test_commit second &&
+	test_commit third &&
+	git tag -d initial &&
+	git tag -d second &&
+	git tag -d third
 '
 
 test_expect_success 'tags can be excluded by rev-list options' '
-
 	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
 	git ls-remote bundle > output &&
 	! grep tag output
-
 '
 
 test_expect_success 'die if bundle file cannot be created' '
-
 	mkdir adir &&
 	test_must_fail git bundle create adir --all
-
 '
 
 test_expect_failure 'bundle --stdin' '
-
 	echo master | git bundle create stdin-bundle.bdl --stdin &&
 	git ls-remote stdin-bundle.bdl >output &&
 	grep master output
-
 '
 
 test_expect_failure 'bundle --stdin <rev-list options>' '
-
 	echo master | git bundle create hybrid-bundle.bdl --stdin tag &&
 	git ls-remote hybrid-bundle.bdl >output &&
 	grep master output
-
 '
 
 test_expect_success 'empty bundle file is rejected' '
-
-    >empty-bundle && test_must_fail git fetch empty-bundle
-
+	: >empty-bundle &&
+	test_must_fail git fetch empty-bundle
 '
 
 test_done

-- 
1.7.9.1.430.g4998543
Junio C Hamano· Feb 23, 2012, 23:36 UTC · re: Thomas Rast · lore

Re: [PATCH v2 3/4] t5704: match tests to modern style

Thomas Rast <trast@student.ethz.ch> writes:
> Fix all of them.  There's a catch to the last point: test_commit
> creates a tag.  We still change it to test_commit, and explicitly
> delete the tags, so as to highlight that the test relies on not having
> them.

I do not have an objection to the use of these three test_commits, and I do not have an objection to delete the extra three tags they create so that the set of refs in the repository matches what the later test expects, either.

But I found the explanation a bit iffy.
Show 16 quoted lines
>  test_expect_success 'setup' '
> +	test_commit initial &&
>  	test_tick &&
>  	git tag -m tag tag &&
> +	test_commit second &&
> +	test_commit third &&
> +	git tag -d initial &&
> +	git tag -d second &&
> +	git tag -d third
>  '
>  
>  test_expect_success 'tags can be excluded by rev-list options' '
>  	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
>  	git ls-remote bundle > output &&
>  	! grep tag output
>  '

If you do not delete 'third' tag, which matches the tip of the current branch, the resulting bundle created with "create --all" would end up containing that tag, and you will see it in the ls-remote output.

But the funny thing is that you can leave initial and second in the repository and the resulting bundle still passes the test. 'initial', 'second' and 'tag' are excluded. Exclusion of 'tag' tag (sheesh, it makes this conversation more confusing than necessary) is what this test checks, and the date range given to the rev-list ensures that initial and second are not included.

But the tip of the current branch and the lightweight 'third' tag both point at the same commit object, and that, together with the fact that we ask for '--all', is the reason why it is included in the result, making the test fail.

So perhaps a better fix may be to do something like the attached on top, and rewrite everything after "Fix all of them".

	... Fix all of them.
        Also rename the manually created tag 'tag' that points at a commit
        that is older than the --since threshold a later test uses, to a
        more descriptive 'ancienttag', and update the check that reads
        from the resulting bundle with ls-remote to look for 'ancienttag'.
        The purpose of this test is to make sure that the tag that
        predates the date range is not in the resulting bundle, but
        because these test_commit also will create tags, one of which
        (namely, 'third') points at a commit that is newer than that
        threshold, ls-remote will list it in its output. Looking for 'tag'
        will match refs/tags/third, making the test incorrectly fail.
Show changes to t/t5704-bundle.sh +3 −6
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index a51c8b0..3c436e7 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -6,18 +6,15 @@ test_description='some bundle related tests'
 test_expect_success 'setup' '
 	test_commit initial &&
 	test_tick &&
-	git tag -m tag tag &&
+	git tag -m tag antienttag &&
 	test_commit second &&
-	test_commit third &&
-	git tag -d initial &&
-	git tag -d second &&
-	git tag -d third
+	test_commit third
 '
 
 test_expect_success 'tags can be excluded by rev-list options' '
 	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
 	git ls-remote bundle > output &&
-	! grep tag output
+	! grep antienttag output
 '
 
 test_expect_success 'die if bundle file cannot be created' '
Thomas Rast· Feb 23, 2012, 09:42 UTC · re: Thomas Rast · lore

[PATCH v2 4/4] bundle: use a strbuf to scan the log for boundary commits

The first part of the bundle header contains the boundary commits, and could be approximated by

  # v2 git bundle
  $(git rev-list --pretty=oneline --boundary <ARGS> | grep ^-)

git-bundle actually spawns exactly this rev-list invocation, and does the grepping internally.

There was a subtle bug in the latter step: it used fgets() with a 1024-byte buffer. If the user has sufficiently long subjects (e.g., by not adhering to the git oneline-subject convention in the first place), the 'oneline' format can easily overflow the buffer. fgets() then returns only the first part of the line _without a \n_, and it would be printed as such, turning the next line into trailing garbage. On top of that, fgets() returns the rest of the line in the next call(s). If one of these remaining parts starts with '-', git-bundle would mistakenly insert it into the boundary list.

Fix it by using strbuf_getwholeline() instead, which handles arbitrary line lengths correctly.

Note that on the receiving side in parse_bundle_header() we were already using strbuf_getwholeline_fd(), so that part is safe.

Thanks to Peff, Johannes Sixt and Junio for suggestions.
Reported-by: Jannis Pohlmann <jannis.pohlmann@codethink.co.uk>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 bundle.c          |   15 ++++++++-------
 t/t5704-bundle.sh |   16 ++++++++++++++++
 2 files changed, 24 insertions(+), 7 deletions(-)
Show changes to 2 files +24 −7

bundle.c, t/t5704-bundle.sh

diff --git a/bundle.c b/bundle.c
index 313de42..7a760db 100644
--- a/bundle.c
+++ b/bundle.c
@@ -234,7 +234,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 	const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
 	const char **argv_pack = xmalloc(6 * sizeof(const char *));
 	int i, ref_count = 0;
-	char buffer[1024];
+	struct strbuf buf = STRBUF_INIT;
 	struct rev_info revs;
 	struct child_process rls;
 	FILE *rls_fout;
@@ -266,20 +266,21 @@ int create_bundle(struct bundle_header *header, const char *path,
 	if (start_command(&rls))
 		return -1;
 	rls_fout = xfdopen(rls.out, "r");
-	while (fgets(buffer, sizeof(buffer), rls_fout)) {
+	while (strbuf_getwholeline(&buf, rls_fout, '\n') != EOF) {
 		unsigned char sha1[20];
-		if (buffer[0] == '-') {
-			write_or_die(bundle_fd, buffer, strlen(buffer));
-			if (!get_sha1_hex(buffer + 1, sha1)) {
+		if (buf.len > 0 && buf.buf[0] == '-') {
+			write_or_die(bundle_fd, buf.buf, buf.len);
+			if (!get_sha1_hex(buf.buf + 1, sha1)) {
 				struct object *object = parse_object(sha1);
 				object->flags |= UNINTERESTING;
-				add_pending_object(&revs, object, buffer);
+				add_pending_object(&revs, object, buf.buf);
 			}
-		} else if (!get_sha1_hex(buffer, sha1)) {
+		} else if (!get_sha1_hex(buf.buf, sha1)) {
 			struct object *object = parse_object(sha1);
 			object->flags |= SHOWN;
 		}
 	}
+	strbuf_release(&buf);
 	fclose(rls_fout);
 	if (finish_command(&rls))
 		return error("rev-list died");
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index f35f559..a51c8b0 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -42,4 +42,20 @@ test_expect_success 'empty bundle file is rejected' '
 	test_must_fail git fetch empty-bundle
 '
 
+# This triggers a bug in older versions where the resulting line (with
+# --pretty=oneline) was longer than a 1024-char buffer.
+test_expect_success 'ridiculously long subject in boundary' '
+	: >file4 &&
+	test_tick &&
+	git add file4 &&
+	printf "%01200d\n" 0 | git commit -F - &&
+	test_commit fifth &&
+	git bundle create long-subject-bundle.bdl HEAD^..HEAD &&
+	git bundle list-heads long-subject-bundle.bdl >heads &&
+	test -s heads &&
+	git fetch long-subject-bundle.bdl &&
+	sed -n "/^-/{p;q}" long-subject-bundle.bdl >boundary &&
+	grep "^-$_x40 " boundary
+'
+
 test_done

-- 
1.7.9.1.430.g4998543
Junio C Hamano· Feb 23, 2012, 19:04 UTC · re: Thomas Rast · lore

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

Thomas Rast <trast@student.ethz.ch> writes:
> So you all made very good points, and I don't want to repeat them.

I really wish you did not make an elephant out of this, or if you must, kept the minimum bugfix part that must apply to older codebase and making an elephant part clearly separable.

Anyway, I've reordered so that the bugfix part would apply to an older maintenance release, and then the remainder applied on top would merge cleanly to the bleeding edge, so there is no need to resend.

Thanks for working on this, everybody.

← back to recent threads