[PATCHv3 0/19] pkt-line cleanups and fixes

[PATCHv3 0/19] pkt-line cleanups and fixes

[Jeff King]

Here's another round of my pkt-line fixes. The more I dug, the more
interesting corners I found. :)

There are really several potentially independent topics rolled together
here. There are dependencies between some of them, so I tried to float
the most independent and non-controversial bits to the beginning. We may
want those as a separate topic to merge sooner, and have the rest as a
topic build on top.

Overall, the diffstat shows a reduction in lines (and I even added a few
dozen lines of comments), which is nice. The intent was to fix some bugs
and corner cases, but I found a lot of cleanup opportunities in the
middle.

 builtin/archive.c          |  17 ++--
 builtin/fetch-pack.c       |  11 +-
 builtin/receive-pack.c     |  10 +-
 builtin/send-pack.c        |   4 +-
 builtin/upload-archive.c   |  45 +++------
 cache.h                    |   4 +-
 connect.c                  |  13 +--
 daemon.c                   |   4 +-
 fetch-pack.c               |  18 ++--
 http-backend.c             |   8 +-
 http.c                     |   1 +
 pkt-line.c                 | 126 ++++++++++-------------
 pkt-line.h                 |  72 +++++++++++++-
 remote-curl.c              | 188 ++++++++++++++++-------------------
 send-pack.c                |  22 ++--
 sideband.c                 |  11 +-
 sideband.h                 |   3 -
 t/t5503-tagfollow.sh       |  38 ++++---
 t/t5700-clone-reference.sh |  10 +-
 transport.c                |   6 +-
 upload-pack.c              |  40 +++-----
 write_or_die.c             |  19 ++--
 22 files changed, 321 insertions(+), 349 deletions(-)

The patches are:

  [01/19]: upload-pack: use get_sha1_hex to parse "shallow" lines

    New in this round; fixes a potential interoperability problem.

  [02/19]: upload-pack: do not add duplicate objects to shallow list

    New. Fixes a potential memory-consumption denial-of-service.

  [03/19]: upload-pack: remove packet debugging harness

    New. Optional cleanup, but later patches textually depend on it.

  [04/19]: fetch-pack: fix out-of-bounds buffer offset in get_ack

    New. Fixes a potential interoperability problem.

  [05/19]: send-pack: prefer prefixcmp over memcmp in receive_status

    New. Optional cleanup.

  [06/19]: upload-archive: do not copy repo name
  [07/19]: upload-archive: use argv_array to store client arguments

    New. Optional cleanup.

  [08/19]: write_or_die: raise SIGPIPE when we get EPIPE
  [09/19]: pkt-line: move a misplaced comment
  [10/19]: pkt-line: drop safe_write function

    The latter two were in the last round; but it's 08/19 that makes
    doing 10/19 safe. I think it's also a sane thing to be doing in
    general for existing callers of write_or_die.

    These can really be pulled into a separate topic if we want, as
    there isn't even a lot of textual dependency.

  [11/19]: pkt-line: provide a generic reading function with options

    This is an alternative to the proliferation of different reading
    functions that round 2 had. I think it ends up cleaner.  It also
    addresses Jonathan's function-signature concerns.

  [12/19]: pkt-line: teach packet_read_line to chomp newlines

    New. A convenience cleanup that drops a lot of lines. Technically
    optional, but later patches depend heavily on it (textually, and for
    splitting line-readers from binary-readers).

  [13/19]: pkt-line: move LARGE_PACKET_MAX definition from sideband
  [14/19]: pkt-line: provide a LARGE_PACKET_MAX static buffer

    New. Another cleanup that makes packet_read_line callers a bit
    simpler, and bumps the packet size limits throughout git, as we
    discussed.

  [15/19]: pkt-line: share buffer/descriptor reading implementation
  [16/19]: teach get_remote_heads to read from a memory buffer
  [17/19]: remote-curl: pass buffer straight to get_remote_heads

    These are more or less ported from v2's patches 6-8, except that the
    earlier pkt-line changes make the first one way more pleasant.

  [18/19]: remote-curl: move ref-parsing code up in file
  [19/19]: remote-curl: always parse incoming refs

    ...and the yak is shaved. More or less a straight rebase of their v2
    counterparts, and the thing that actually started me on this topic.

I know it's a big series, but I tried hard to break it down into
bite-sized chunks. Thanks for your reviewing patience.

-Peff

[PATCH v3 01/19] upload-pack: use get_sha1_hex to parse "shallow" lines

[Jeff King]

When we receive a line like "shallow <sha1>" from the
client, we feed the <sha1> part to get_sha1. This is a
mistake, as the argument on a shallow line is defined by
Documentation/technical/pack-protocol.txt to contain an
"obj-id".  This is never defined in the BNF, but it is clear
from the text and from the other uses that it is meant to be
a hex sha1, not an arbitrary identifier (and that is what
fetch-pack has always sent).

We should be using get_sha1_hex instead, which doesn't allow
the client to request arbitrary junk like "HEAD@{yesterday}".
Because this is just marking shallow objects, the client
couldn't actually do anything interesting (like fetching
objects from unreachable reflog entries), but we should keep
our parsing tight to be on the safe side.

Because get_sha1 is for the most part a superset of
get_sha1_hex, in theory the only behavior change should be
disallowing non-hex object references. However, there is
one interesting exception: get_sha1 will only parse
a 40-character hex sha1 if the string has exactly 40
characters, whereas get_sha1_hex will just eat the first 40
characters, leaving the rest. That means that current
versions of git-upload-pack will not accept a "shallow"
packet that has a trailing newline, even though the protocol
documentation is clear that newlines are allowed (even
encouraged) in non-binary parts of the protocol.

This never mattered in practice, though, because fetch-pack,
contrary to the protocol documentation, does not include a
newline in its shallow lines. JGit follows its lead (though
it correctly is strict on the parsing end about wanting a
hex object id).

We do not adjust fetch-pack to send newlines here, as it
would break communication with older versions of git (and
there is no actual benefit to doing so, except for
consistency with other parts of the protocol).

Signed-off-by: Jeff King <peff@peff.net>
---
I couldn't trigger anything interestingly malicious from this, but I
didn't look very hard. Maybe somebody who knows the shallow protocol
better could think of something clever (not that it matters much).

 upload-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/upload-pack.c b/upload-pack.c
index 30146a0..b058e8d 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -596,7 +596,7 @@ static void receive_needs(void)
 		if (!prefixcmp(line, "shallow ")) {
 			unsigned char sha1[20];
 			struct object *object;
-			if (get_sha1(line + 8, sha1))
+			if (get_sha1_hex(line + 8, sha1))
 				die("invalid shallow line: %s", line);
 			object = parse_object(sha1);
 			if (!object)
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 02/19] upload-pack: do not add duplicate objects to shallow list

[Jeff King]

When the client tells us it has a shallow object via
"shallow <sha1>", we make sure we have the object, mark it
with a flag, then add it to a dynamic array of shallow
objects. This means that a client can get us to allocate
arbitrary amounts of memory just by flooding us with shallow
lines (whether they have the objects or not). You can
demonstrate it easily with:

  yes '0035shallow e83c5163316f89bfbde7d9ab23ca2e25604af290' |
  git-upload-pack git.git

We already protect against duplicates in want lines by
checking if our flag is already set; let's do the same thing
here. Note that a client can still get us to allocate some
amount of memory by marking every object in the repo as
"shallow" (or "want"). But this at least bounds it with the
number of objects in the repository, which is not under the
control of an upload-pack client.

Signed-off-by: Jeff King <peff@peff.net>
---
Looking over upload-pack, I think this is the only "consume arbitrary
memory" spot. Since you can convince git to go to quite a bit of work
just processing a big repo, the distinction may not be important, but
drawing the line between "large" and "arbitrarily large" seemed
reasonable to me (and it's a trivial fix).

 upload-pack.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index b058e8d..1aee407 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -603,8 +603,10 @@ static void receive_needs(void)
 				die("did not find object for %s", line);
 			if (object->type != OBJ_COMMIT)
 				die("invalid shallow object %s", sha1_to_hex(sha1));
-			object->flags |= CLIENT_SHALLOW;
-			add_object_array(object, NULL, &shallows);
+			if (!(object->flags & CLIENT_SHALLOW)) {
+			    object->flags |= CLIENT_SHALLOW;
+			    add_object_array(object, NULL, &shallows);
+			}
 			continue;
 		}
 		if (!prefixcmp(line, "deepen ")) {
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 03/19] upload-pack: remove packet debugging harness

[Jeff King]

If you set the GIT_DEBUG_SEND_PACK environment variable,
upload-pack will dump lines it receives in the receive_needs
phase to a descriptor. This debugging harness is a strict
subset of what GIT_TRACE_PACKET can do. Let's just drop it
in favor of that.

A few tests used GIT_DEBUG_SEND_PACK to confirm which
objects get sent; we have to adapt them to the new output
format.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5503-tagfollow.sh       | 38 +++++++++++++++++---------------------
 t/t5700-clone-reference.sh | 10 +++++-----
 upload-pack.c              |  9 ---------
 3 files changed, 22 insertions(+), 35 deletions(-)

diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 60de2d6..d181c96 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -5,7 +5,7 @@ if ! test_have_prereq NOT_MINGW; then
 . ./test-lib.sh
 
 if ! test_have_prereq NOT_MINGW; then
-	say "GIT_DEBUG_SEND_PACK not supported - skipping tests"
+	say "GIT_TRACE_PACKET not supported - skipping tests"
 fi
 
 # End state of the repository:
@@ -42,21 +42,26 @@ test_expect_success NOT_MINGW 'fetch A (new commit : 1 connection)' '
 
 test_expect_success NOT_MINGW 'setup expect' '
 cat - <<EOF >expect
-#S
 want $A
-#E
 EOF
 '
 
+get_needs () {
+	perl -alne '
+		next unless $F[1] eq "upload-pack<";
+		last if $F[2] eq "0000";
+		print $F[2], " ", $F[3];
+	' "$@"
+}
+
 test_expect_success NOT_MINGW 'fetch A (new commit : 1 connection)' '
 	rm -f $U &&
 	(
 		cd cloned &&
-		GIT_DEBUG_SEND_PACK=3 git fetch 3>../$U &&
+		GIT_TRACE_PACKET=3 git fetch 3>../$U &&
 		test $A = $(git rev-parse --verify origin/master)
 	) &&
-	test -s $U &&
-	cut -d" " -f1,2 $U >actual &&
+	get_needs $U >actual &&
 	test_cmp expect actual
 '
 
@@ -74,10 +79,8 @@ want $T
 
 test_expect_success NOT_MINGW 'setup expect' '
 cat - <<EOF >expect
-#S
 want $C
 want $T
-#E
 EOF
 '
 
@@ -85,13 +88,12 @@ test_expect_success NOT_MINGW 'fetch C, T (new branch, tag : 1 connection)' '
 	rm -f $U &&
 	(
 		cd cloned &&
-		GIT_DEBUG_SEND_PACK=3 git fetch 3>../$U &&
+		GIT_TRACE_PACKET=3 git fetch 3>../$U &&
 		test $C = $(git rev-parse --verify origin/cat) &&
 		test $T = $(git rev-parse --verify tag1) &&
 		test $A = $(git rev-parse --verify tag1^0)
 	) &&
-	test -s $U &&
-	cut -d" " -f1,2 $U >actual &&
+	get_needs $U >actual &&
 	test_cmp expect actual
 '
 
@@ -113,10 +115,8 @@ want $S
 
 test_expect_success NOT_MINGW 'setup expect' '
 cat - <<EOF >expect
-#S
 want $B
 want $S
-#E
 EOF
 '
 
@@ -124,22 +124,19 @@ want $S
 	rm -f $U &&
 	(
 		cd cloned &&
-		GIT_DEBUG_SEND_PACK=3 git fetch 3>../$U &&
+		GIT_TRACE_PACKET=3 git fetch 3>../$U &&
 		test $B = $(git rev-parse --verify origin/master) &&
 		test $B = $(git rev-parse --verify tag2^0) &&
 		test $S = $(git rev-parse --verify tag2)
 	) &&
-	test -s $U &&
-	cut -d" " -f1,2 $U >actual &&
+	get_needs $U >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success NOT_MINGW 'setup expect' '
 cat - <<EOF >expect
-#S
 want $B
 want $S
-#E
 EOF
 '
 
@@ -151,15 +148,14 @@ test_expect_success NOT_MINGW 'new clone fetch master and tags' '
 		cd clone2 &&
 		git init &&
 		git remote add origin .. &&
-		GIT_DEBUG_SEND_PACK=3 git fetch 3>../$U &&
+		GIT_TRACE_PACKET=3 git fetch 3>../$U &&
 		test $B = $(git rev-parse --verify origin/master) &&
 		test $S = $(git rev-parse --verify tag2) &&
 		test $B = $(git rev-parse --verify tag2^0) &&
 		test $T = $(git rev-parse --verify tag1) &&
 		test $A = $(git rev-parse --verify tag1^0)
 	) &&
-	test -s $U &&
-	cut -d" " -f1,2 $U >actual &&
+	get_needs $U >actual &&
 	test_cmp expect actual
 '
 
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index c47d450..9cd3b4d 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -55,10 +55,10 @@ test_expect_success 'fetched no objects' \
 rm -f "$U.D"
 
 test_expect_success 'cloning with reference (no -l -s)' \
-'GIT_DEBUG_SEND_PACK=3 git clone --reference B "file://$(pwd)/A" D 3>"$U.D"'
+'GIT_TRACE_PACKET=3 git clone --reference B "file://$(pwd)/A" D 3>"$U.D"'
 
 test_expect_success 'fetched no objects' \
-'! grep "^want" "$U.D"'
+'! grep " want" "$U.D"'
 
 cd "$base_dir"
 
@@ -173,12 +173,12 @@ test_expect_success 'fetch with incomplete alternates' '
 	(
 		cd K &&
 		git remote add J "file://$base_dir/J" &&
-		GIT_DEBUG_SEND_PACK=3 git fetch J 3>"$U.K"
+		GIT_TRACE_PACKET=3 git fetch J 3>"$U.K"
 	) &&
 	master_object=$(cd A && git for-each-ref --format="%(objectname)" refs/heads/master) &&
-	! grep "^want $master_object" "$U.K" &&
+	! grep " want $master_object" "$U.K" &&
 	tag_object=$(cd A && git for-each-ref --format="%(objectname)" refs/tags/HEAD) &&
-	! grep "^want $tag_object" "$U.K"
+	! grep " want $tag_object" "$U.K"
 '
 
 test_done
diff --git a/upload-pack.c b/upload-pack.c
index 1aee407..63cea91 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -42,7 +42,6 @@ static int use_sideband;
  * otherwise maximum packet size (up to 65520 bytes).
  */
 static int use_sideband;
-static int debug_fd;
 static int advertise_refs;
 static int stateless_rpc;
 
@@ -580,8 +579,6 @@ static void receive_needs(void)
 	int has_non_tip = 0;
 
 	shallow_nr = 0;
-	if (debug_fd)
-		write_str_in_full(debug_fd, "#S\n");
 	for (;;) {
 		struct object *o;
 		const char *features;
@@ -590,8 +587,6 @@ static void receive_needs(void)
 		reset_timeout();
 		if (!len)
 			break;
-		if (debug_fd)
-			write_in_full(debug_fd, line, len);
 
 		if (!prefixcmp(line, "shallow ")) {
 			unsigned char sha1[20];
@@ -653,8 +648,6 @@ static void receive_needs(void)
 			add_object_array(o, NULL, &want_obj);
 		}
 	}
-	if (debug_fd)
-		write_str_in_full(debug_fd, "#E\n");
 
 	/*
 	 * We have sent all our refs already, and the other end
@@ -845,8 +838,6 @@ int main(int argc, char **argv)
 	if (is_repository_shallow())
 		die("attempt to fetch/clone from a shallow repository");
 	git_config(upload_pack_config, NULL);
-	if (getenv("GIT_DEBUG_SEND_PACK"))
-		debug_fd = atoi(getenv("GIT_DEBUG_SEND_PACK"));
 	upload_pack();
 	return 0;
 }
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 04/19] fetch-pack: fix out-of-bounds buffer offset in get_ack

[Jeff King]

When we read acks from the remote, we expect either:

  ACK <sha1>

or

  ACK <sha1> <multi-ack-flag>

We parse the "ACK <sha1>" bit from the line, and then start
looking for the flag strings at "line+45"; if we don't have
them, we assume it's of the first type.  But if we do have
the first type, then line+45 is not necessarily inside our
string at all!

It turns out that this works most of the time due to the way
we parse the packets. They should come in with a newline,
and packet_read puts an extra NUL into the buffer, so we end
up with:

  ACK <sha1>\n\0

with the newline at offset 44 and the NUL at offset 45. We
then strip the newline, putting a NUL at offset 44. So
when we look at "line+45", we are looking past the end of
our string; but it's OK, because we hit the terminator from
the original string.

This breaks down, however, if the other side does not
terminate their packets with a newline. In that case, our
packet is one character shorter, and we start looking
through uninitialized memory for the flag. No known
implementation sends such a packet, so it has never come up
in practice.

This patch tightens the check by looking for a short,
flagless ACK before trying to parse the flag.

Signed-off-by: Jeff King <peff@peff.net>
---
This is the absolute minimal fix, which just checks for the no-flag case
early; we still treat arbitrary crud in the flag field as just an ACK.
From my understanding of the protocol, a saner parsing scheme would be:

  const char *flag = line + 44; /* we already parsed "ACK <sha1>" */
  if (!*flag)
          return ACK;
  if (!strcmp(flag, " continue"))
          return ACK_continue;
  if (!strcmp(flag, " common"))
          return ACK_continue;
  if (!strcmp(flag, " ready"))
          return ACK_ready;
  die("fetch-pack expected multi-ack flag, got: %s", line);

But that is much tighter, and I wasn't sure if the looseness was there
to facilitate future expansion or something (though I'd think we would
need a new capability for that).

 fetch-pack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fetch-pack.c b/fetch-pack.c
index 6d8926a..27a3e80 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -226,6 +226,8 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 		return NAK;
 	if (!prefixcmp(line, "ACK ")) {
 		if (!get_sha1_hex(line+4, result_sha1)) {
+			if (len < 45)
+				return ACK;
 			if (strstr(line+45, "continue"))
 				return ACK_continue;
 			if (strstr(line+45, "common"))
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 05/19] send-pack: prefer prefixcmp over memcmp in receive_status

[Jeff King]

This code predates prefixcmp, so it used memcmp along with
static sizes. Replacing these memcmps with prefixcmp makes
the code much more readable, and the lack of static sizes
will make refactoring it in future patches simpler.

Note that we used to be unnecessarily liberal in parsing the
"unpack" status line, and would accept "unpack ok\njunk". No
version of git has ever produced that, and it violates the
BNF in Documentation/technical/pack-protocol.txt. Let's take
this opportunity to tighten the check by converting the
prefix comparison into a strcmp.

While we're in the area, let's also fix a vague error
message that does not follow our usual conventions (it
writes directly to stderr and does not use the "error:"
prefix).

Signed-off-by: Jeff King <peff@peff.net>
---
 send-pack.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 97ab336..e91cbe2 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -109,9 +109,9 @@ static int receive_status(int in, struct ref *refs)
 	char line[1000];
 	int ret = 0;
 	int len = packet_read_line(in, line, sizeof(line));
-	if (len < 10 || memcmp(line, "unpack ", 7))
+	if (prefixcmp(line, "unpack "))
 		return error("did not receive remote status");
-	if (memcmp(line, "unpack ok\n", 10)) {
+	if (strcmp(line, "unpack ok\n")) {
 		char *p = line + strlen(line) - 1;
 		if (*p == '\n')
 			*p = '\0';
@@ -125,9 +125,8 @@ static int receive_status(int in, struct ref *refs)
 		len = packet_read_line(in, line, sizeof(line));
 		if (!len)
 			break;
-		if (len < 3 ||
-		    (memcmp(line, "ok ", 3) && memcmp(line, "ng ", 3))) {
-			fprintf(stderr, "protocol error: %s\n", line);
+		if (prefixcmp(line, "ok ") && prefixcmp(line, "ng ")) {
+			error("invalid ref status from remote: %s", line);
 			ret = -1;
 			break;
 		}
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 06/19] upload-archive: do not copy repo name

[Jeff King]

According to the comment, enter_repo will modify its input.
However, this has not been the case since 1c64b48
(enter_repo: do not modify input, 2011-10-04). Drop the
now-useless copy.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/upload-archive.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index b928beb..c3d134e 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -27,13 +27,8 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 	if (argc != 2)
 		usage(upload_archive_usage);
 
-	if (strlen(argv[1]) + 1 > sizeof(buf))
-		die("insanely long repository name");
-
-	strcpy(buf, argv[1]); /* enter-repo smudges its argument */
-
-	if (!enter_repo(buf, 0))
-		die("'%s' does not appear to be a git repository", buf);
+	if (!enter_repo(argv[1], 0))
+		die("'%s' does not appear to be a git repository", argv[1]);
 
 	/* put received options in sent_argv[] */
 	sent_argc = 1;
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 07/19] upload-archive: use argv_array to store client arguments

[Jeff King]

The current parsing scheme for upload-archive is to pack
arguments into a fixed-size buffer, separated by NULs, and
put a pointer to each argument in the buffer into a
fixed-size argv array.

This works fine, and the limits are high enough that nobody
reasonable is going to hit them, but it makes the code hard
to follow.  Instead, let's just stuff the arguments into an
argv_array, which is much simpler. That lifts the "all
arguments must fit inside 4K together" limit.

We could also trivially lift the MAX_ARGS limitation (in
fact, we have to keep extra code to enforce it). But that
would mean a client could force us to allocate an arbitrary
amount of memory simply by sending us "argument" lines. By
limiting the MAX_ARGS, we limit an attacker to about 4
megabytes (64 times a maximum 64K packet buffer). That may
sound like a lot compared to the 4K limit, but it's not a
big deal compared to what git-archive will actually allocate
while working (e.g., to load blobs into memory). The
important thing is that it is bounded.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/upload-archive.c | 35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index c3d134e..3393cef 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -7,6 +7,7 @@
 #include "pkt-line.h"
 #include "sideband.h"
 #include "run-command.h"
+#include "argv-array.h"
 
 static const char upload_archive_usage[] =
 	"git upload-archive <repo>";
@@ -18,10 +19,9 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 
 int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 {
-	const char *sent_argv[MAX_ARGS];
+	struct argv_array sent_argv = ARGV_ARRAY_INIT;
 	const char *arg_cmd = "argument ";
-	char *p, buf[4096];
-	int sent_argc;
+	char buf[4096];
 	int len;
 
 	if (argc != 2)
@@ -31,33 +31,26 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 		die("'%s' does not appear to be a git repository", argv[1]);
 
 	/* put received options in sent_argv[] */
-	sent_argc = 1;
-	sent_argv[0] = "git-upload-archive";
-	for (p = buf;;) {
+	argv_array_push(&sent_argv, "git-upload-archive");
+	for (;;) {
 		/* This will die if not enough free space in buf */
-		len = packet_read_line(0, p, (buf + sizeof buf) - p);
+		len = packet_read_line(0, buf, sizeof(buf));
 		if (len == 0)
 			break;	/* got a flush */
-		if (sent_argc > MAX_ARGS - 2)
-			die("Too many options (>%d)", MAX_ARGS - 2);
+		if (sent_argv.argc > MAX_ARGS)
+		    die("Too many options (>%d)", MAX_ARGS - 1);
 
-		if (p[len-1] == '\n') {
-			p[--len] = 0;
+		if (buf[len-1] == '\n') {
+			buf[--len] = 0;
 		}
-		if (len < strlen(arg_cmd) ||
-		    strncmp(arg_cmd, p, strlen(arg_cmd)))
-			die("'argument' token or flush expected");
 
-		len -= strlen(arg_cmd);
-		memmove(p, p + strlen(arg_cmd), len);
-		sent_argv[sent_argc++] = p;
-		p += len;
-		*p++ = 0;
+		if (prefixcmp(buf, arg_cmd))
+			die("'argument' token or flush expected");
+		argv_array_push(&sent_argv, buf + strlen(arg_cmd));
 	}
-	sent_argv[sent_argc] = NULL;
 
 	/* parse all options sent by the client */
-	return write_archive(sent_argc, sent_argv, prefix, 0, NULL, 1);
+	return write_archive(sent_argv.argc, sent_argv.argv, prefix, 0, NULL, 1);
 }
 
 __attribute__((format (printf, 1, 2)))
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE

[Jeff King]

The write_or_die function will always die on an error,
including EPIPE. However, it currently treats EPIPE
specially by suppressing any error message, and by exiting
with exit code 0.

Suppressing the error message makes some sense; a pipe death
may just be a sign that the other side is not interested in
what we have to say. However, exiting with a successful
error code is not a good idea, as write_or_die is frequently
used in cases where we want to be careful about having
written all of the output, and we may need to signal to our
caller that we have done so (e.g., you would not want a push
whose other end has hung up to report success).

This distinction doesn't typically matter in git, because we
do not ignore SIGPIPE in the first place. Which means that
we will not get EPIPE, but instead will just die when we get
a SIGPIPE. But it's possible for a default handler to be set
by a parent process, or for us to add a callsite inside one
of our few SIGPIPE-ignoring blocks of code.

This patch converts write_or_die to actually raise SIGPIPE
when we see EPIPE, rather than exiting with zero. This
brings the behavior in line with the "normal" case that we
die from SIGPIPE (and any callers who want to check why we
died will see the same thing). We also give the same
treatment to other related functions, including
write_or_whine_pipe and maybe_flush_or_die.

Signed-off-by: Jeff King <peff@peff.net>
---
 write_or_die.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/write_or_die.c b/write_or_die.c
index 960f448..b50f99a 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -1,5 +1,15 @@
 #include "cache.h"
 
+static void check_pipe(int err)
+{
+	if (err == EPIPE) {
+		signal(SIGPIPE, SIG_DFL);
+		raise(SIGPIPE);
+		/* Should never happen, but just in case... */
+		exit(141);
+	}
+}
+
 /*
  * Some cases use stdio, but want to flush after the write
  * to get error handling (and to get better interactive
@@ -34,8 +44,7 @@ void maybe_flush_or_die(FILE *f, const char *desc)
 			return;
 	}
 	if (fflush(f)) {
-		if (errno == EPIPE)
-			exit(0);
+		check_pipe(errno);
 		die_errno("write failure on '%s'", desc);
 	}
 }
@@ -50,8 +59,7 @@ void write_or_die(int fd, const void *buf, size_t count)
 void write_or_die(int fd, const void *buf, size_t count)
 {
 	if (write_in_full(fd, buf, count) < 0) {
-		if (errno == EPIPE)
-			exit(0);
+		check_pipe(errno);
 		die_errno("write error");
 	}
 }
@@ -59,8 +67,7 @@ int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
 int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
 {
 	if (write_in_full(fd, buf, count) < 0) {
-		if (errno == EPIPE)
-			exit(0);
+		check_pipe(errno);
 		fprintf(stderr, "%s: write error (%s)\n",
 			msg, strerror(errno));
 		return 0;
-- 
1.8.2.rc0.9.g352092c

Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE

[Jonathan Nieder]

Jeff King wrote:

> The write_or_die function will always die on an error,
> including EPIPE. However, it currently treats EPIPE
> specially by suppressing any error message, and by exiting
> with exit code 0.
>
> Suppressing the error message makes some sense; a pipe death
> may just be a sign that the other side is not interested in
> what we have to say. However, exiting with a successful
> error code is not a good idea, as write_or_die is frequently
> used in cases where we want to be careful about having
> written all of the output, and we may need to signal to our
> caller that we have done so (e.g., you would not want a push
> whose other end has hung up to report success).
>
> This distinction doesn't typically matter in git, because we
> do not ignore SIGPIPE in the first place. Which means that
> we will not get EPIPE, but instead will just die when we get
> a SIGPIPE. But it's possible for a default handler to be set
> by a parent process,

Not so much "default" as "insane inherited", as in the example
of old versions of Python's subprocess.Popen.

I suspect this used exit(0) instead of raise(SIGPIPE) in the first
place to work around a bash bug (too much verbosity about SIGPIPE).
If any programs still have that kind of bug, I'd rather put pressure
on them to fix it by *not* working around it.  So the basic idea here
looks good to me.

[...]
> --- a/write_or_die.c
> +++ b/write_or_die.c
> @@ -1,5 +1,15 @@
>  #include "cache.h"
>  
> +static void check_pipe(int err)
> +{
> +	if (err == EPIPE) {
> +		signal(SIGPIPE, SIG_DFL);
> +		raise(SIGPIPE);
> +		/* Should never happen, but just in case... */
> +		exit(141);

How about

		die("BUG: another thread changed SIGPIPE handling behind my back!");

to make it easier to find and fix such problems?

Thanks,
Jonathan

Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE

[Jeff King]

On Wed, Feb 20, 2013 at 01:51:11PM -0800, Jonathan Nieder wrote:

> > This distinction doesn't typically matter in git, because we
> > do not ignore SIGPIPE in the first place. Which means that
> > we will not get EPIPE, but instead will just die when we get
> > a SIGPIPE. But it's possible for a default handler to be set
> > by a parent process,
> 
> Not so much "default" as "insane inherited", as in the example
> of old versions of Python's subprocess.Popen.

It's possible that somebody could have a legitimate reason for doing so.
I just can't think of one. :)

> I suspect this used exit(0) instead of raise(SIGPIPE) in the first
> place to work around a bash bug (too much verbosity about SIGPIPE).
> If any programs still have that kind of bug, I'd rather put pressure
> on them to fix it by *not* working around it.  So the basic idea here
> looks good to me.

Yeah, if you look for old discussions on SIGPIPE in the git list, it is
mostly Linus complaining about the bash behavior, and this code does
date back to that era. The bash bug is long since fixed.

> > +	if (err == EPIPE) {
> > +		signal(SIGPIPE, SIG_DFL);
> > +		raise(SIGPIPE);
> > +		/* Should never happen, but just in case... */
> > +		exit(141);
> 
> How about
> 
> 		die("BUG: another thread changed SIGPIPE handling behind my back!");
> 
> to make it easier to find and fix such problems?

You mean for the "should never happen" bit, not the first part, right? I
actually wonder if we should simply exit(141) in the first place. That
is shell exit-code for SIGPIPE death already (so it's what our
run_command would show us, and what anybody running us through shell
would see).

-Peff

Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE

[Jonathan Nieder]

Jeff King wrote:
> On Wed, Feb 20, 2013 at 01:51:11PM -0800, Jonathan Nieder wrote:

>>> +	if (err == EPIPE) {
>>> +		signal(SIGPIPE, SIG_DFL);
>>> +		raise(SIGPIPE);
>>> +		/* Should never happen, but just in case... */
>>> +		exit(141);
>>
>> How about
>>
>> 		die("BUG: another thread changed SIGPIPE handling behind my back!");
>>
>> to make it easier to find and fix such problems?
>
> You mean for the "should never happen" bit, not the first part, right? I
> actually wonder if we should simply exit(141) in the first place. That
> is shell exit-code for SIGPIPE death already (so it's what our
> run_command would show us, and what anybody running us through shell
> would see).

Yes, for the "should never happen" part.  Raising a signal is nice
because it means the wait()-ing process can see what happened by
checking WIFSIGNALED(status).

Jonathan

Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE

[Jeff King]

On Wed, Feb 20, 2013 at 02:01:14PM -0800, Jonathan Nieder wrote:

> >> How about
> >>
> >> 		die("BUG: another thread changed SIGPIPE handling behind my back!");
> >>
> >> to make it easier to find and fix such problems?
> >
> > You mean for the "should never happen" bit, not the first part, right? I
> > actually wonder if we should simply exit(141) in the first place. That
> > is shell exit-code for SIGPIPE death already (so it's what our
> > run_command would show us, and what anybody running us through shell
> > would see).
> 
> Yes, for the "should never happen" part.  Raising a signal is nice
> because it means the wait()-ing process can see what happened by
> checking WIFSIGNALED(status).

Right. My point is that only happens if there's no shell in the way. But
I guess it doesn't hurt to make the attempt to help the people using
wait() directly.

I don't mind adding a "BUG: " message like you described, but we should
still try to exit(141) as the backup, since that is the shell-equivalent
code to the SIGPIPE signal death.

-Peff

Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE

[Jonathan Nieder]

Jeff King wrote:
> On Wed, Feb 20, 2013 at 02:01:14PM -0800, Jonathan Nieder wrote:

>>>> How about
>>>>
>>>> 		die("BUG: another thread changed SIGPIPE handling behind my back!");
>>>>
>>>> to make it easier to find and fix such problems?
>>>
>>> You mean for the "should never happen" bit, not the first part, right? I
>>> actually wonder if we should simply exit(141) in the first place. That
>>> is shell exit-code for SIGPIPE death already (so it's what our
>>> run_command would show us, and what anybody running us through shell
>>> would see).
>>
>> Yes, for the "should never happen" part.
[...]
> I don't mind adding a "BUG: " message like you described, but we should
> still try to exit(141) as the backup, since that is the shell-equivalent
> code to the SIGPIPE signal death.

If you want. :)

I think caring about graceful degradation of behavior in the case of
an assertion failure is overengineering, but it's mostly harmless.

Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE

[Jeff King]

On Wed, Feb 20, 2013 at 02:06:37PM -0800, Jonathan Nieder wrote:

> > I don't mind adding a "BUG: " message like you described, but we should
> > still try to exit(141) as the backup, since that is the shell-equivalent
> > code to the SIGPIPE signal death.
> 
> If you want. :)
> 
> I think caring about graceful degradation of behavior in the case of
> an assertion failure is overengineering, but it's mostly harmless.

I am more concerned that the assertion is not "oops, another thread is
doing something crazy, and it is a bug", but rather that there is some
weird platform where SIG_DFL does not kill the program under SIGPIPE.
That seems pretty crazy, though. I think I'd squash in something like
this:

diff --git a/write_or_die.c b/write_or_die.c
index b50f99a..abb64db 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -5,7 +5,9 @@ static void check_pipe(int err)
 	if (err == EPIPE) {
 		signal(SIGPIPE, SIG_DFL);
 		raise(SIGPIPE);
+
 		/* Should never happen, but just in case... */
+		error("BUG: SIGPIPE on SIG_DFL handler did not kill us.");
 		exit(141);
 	}
 }

which more directly reports the assertion that failed, and degrades
reasonably gracefully. Yeah, it's probably overengineering, but it's
easy enough to do.

-Peff

Re: [PATCH v3 08/19] write_or_die: raise SIGPIPE when we get EPIPE

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I am more concerned that the assertion is not "oops, another thread is
> doing something crazy, and it is a bug", but rather that there is some
> weird platform where SIG_DFL does not kill the program under SIGPIPE.
> That seems pretty crazy, though. I think I'd squash in something like
> this:
>
> diff --git a/write_or_die.c b/write_or_die.c
> index b50f99a..abb64db 100644
> --- a/write_or_die.c
> +++ b/write_or_die.c
> @@ -5,7 +5,9 @@ static void check_pipe(int err)
>  	if (err == EPIPE) {
>  		signal(SIGPIPE, SIG_DFL);
>  		raise(SIGPIPE);
> +
>  		/* Should never happen, but just in case... */
> +		error("BUG: SIGPIPE on SIG_DFL handler did not kill us.");
>  		exit(141);
>  	}
>  }
>
> which more directly reports the assertion that failed, and degrades
> reasonably gracefully. Yeah, it's probably overengineering, but it's
> easy enough to do.

Yeah, that sounds like a sensible thing to do, as it is cheap even
though we do not expect it to trigger.

[BUG] MSVC: error box when interrupting `gitlog` by quitting less

[Marat Radchenko]

Jeff King <peff <at> peff.net> writes:

> 
> The write_or_die function will always die on an error,
> including EPIPE. However, it currently treats EPIPE
> specially by suppressing any error message, and by exiting
> with exit code 0.

This causes error box on Windows in MSVC=1 build:

git.exe!_invoke_watson(...) Line 132	C++
git.exe!_invalid_parameter(...) Line 85	C++
git.exe!_invalid_parameter_noinfo() Line 97	C++
git.exe!raise(int signum) Line 499	C
git.exe!mingw_raise(int sig) Line 1745	C
git.exe!check_pipe(int err) Line 9	C
git.exe!maybe_flush_or_die(_iobuf * f, const char * desc) Line 48	C
git.exe!log_tree_commit(rev_info * opt, commit * commit) Line 820	C
git.exe!cmd_log_walk(rev_info * rev) Line 344	C
git.exe!cmd_log(int argc, const char * * argv, const char * prefix) Line 637	C
git.exe!run_builtin(cmd_struct * p, int argc, const char * * argv) Line 314	C
git.exe!handle_builtin(int argc, const char * * argv) Line 487	C
git.exe!run_argv(int * argcp, const char * * * argv) Line 536	C
git.exe!mingw_main(int argc, char * * av) Line 616	C
git.exe!main(int argc, char * * argv) Line 551	C

"Should never happen", ha-ha.

Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less

[Marat Radchenko]

Marat Radchenko <marat <at> slonopotamus.org> writes:

> 
> Jeff King <peff <at> peff.net> writes:
> 
> > 
> > The write_or_die function will always die on an error,
> > including EPIPE. However, it currently treats EPIPE
> > specially by suppressing any error message, and by exiting
> > with exit code 0.
> 
> This causes error box on Windows in MSVC=1 build:

After deeper investigation it turned out that Windows supports
much less signals [1] than POSIX and "If the argument is not a valid signal 
as specified above, the invalid parameter handler is invoked".

The question is - what is the proper way to fix this?
Patch mingw_raise in compat/mingw.c to map unsupported signals into
supported ones like SIGPIPE -> SIGTERM?

[1]: http://msdn.microsoft.com/en-us/library/dwwzkt4c.aspx

Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less

[Jeff King]

On Fri, Mar 28, 2014 at 09:14:07AM +0000, Marat Radchenko wrote:

> > Jeff King <peff <at> peff.net> writes:
> > 
> > > 
> > > The write_or_die function will always die on an error,
> > > including EPIPE. However, it currently treats EPIPE
> > > specially by suppressing any error message, and by exiting
> > > with exit code 0.
> > 
> > This causes error box on Windows in MSVC=1 build:
> 
> After deeper investigation it turned out that Windows supports
> much less signals [1] than POSIX and "If the argument is not a valid signal 
> as specified above, the invalid parameter handler is invoked".
> 
> The question is - what is the proper way to fix this?
> Patch mingw_raise in compat/mingw.c to map unsupported signals into
> supported ones like SIGPIPE -> SIGTERM?
> 
> [1]: http://msdn.microsoft.com/en-us/library/dwwzkt4c.aspx

I'm not sure what an actual SIGPIPE death looks like on Windows. What
happens if git is still writing data to the pager and the pager exits?
Does it receive a signal of some sort?

The point of the code in check_pipe is to simulate that death. So
whatever happens to git in that case is what we would want to happen
when we call raise(SIGPIPE).

A possibly simpler option would be to just have the MSVC build skip the
raise() call, and do the exit(141) that comes just after. That is
probably close enough simulation of SIGPIPE death.

-Peff

Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less

[Marat Radchenko]

Jeff King <peff <at> peff.net> writes:

> 
> I'm not sure what an actual SIGPIPE death looks like on Windows.

There is no SIGPIPE death on Windows due to total absence of SIGPIPE.
raise(unsupported int) just causes ugly "git.exe has stopped working"
window and possibly ends up as SIGABT (I don't know how to check this).

> What
> happens if git is still writing data to the pager and the pager exits?
> Does it receive a signal of some sort?

I'm not sure what you mean, sorry. check_pipe properly detects pager exit.
The problem is with the way it tries to die.

> The point of the code in check_pipe is to simulate that death. So
> whatever happens to git in that case is what we would want to happen
> when we call raise(SIGPIPE).

That's what I'm talking about. On Windows, you can't raise(SIGPIPE).
You can only raise(Windows_supported_signal) where signal is one of:
SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM as MSDN tells us.

> A possibly simpler option would be to just have the MSVC build skip the
> raise() call, and do the exit(141) that comes just after. That is
> probably close enough simulation of SIGPIPE death.

Isn't raise(SIGTERM/SIGINT) good enough?

Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less

[Jeff King]

On Fri, Mar 28, 2014 at 10:07:22AM +0000, Marat Radchenko wrote:

> > What
> > happens if git is still writing data to the pager and the pager exits?
> > Does it receive a signal of some sort?
> 
> I'm not sure what you mean, sorry. check_pipe properly detects pager exit.
> The problem is with the way it tries to die.

Right, but check_pipe shouldn't trigger in most cases on Unix because
the process will be killed by SIGPIPE automatically. It's only there to
catch the case where we have disabled SIGPIPE.

On Windows, what happens to "yes" if you run:

  yes | (exit 0)

On Unix, "yes" receives SIGPIPE and dies. Does it run forever on
Windows? If it dies, what does the death look like (does it have a
signal death, or exit with a specific code?).

> > The point of the code in check_pipe is to simulate that death. So
> > whatever happens to git in that case is what we would want to happen
> > when we call raise(SIGPIPE).
> 
> That's what I'm talking about. On Windows, you can't raise(SIGPIPE).
> You can only raise(Windows_supported_signal) where signal is one of:
> SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM as MSDN tells us.

Right, I understand that you don't have SIGPIPE. But we want to emulate
whatever happens in the case I described above.

> > A possibly simpler option would be to just have the MSVC build skip the
> > raise() call, and do the exit(141) that comes just after. That is
> > probably close enough simulation of SIGPIPE death.
> 
> Isn't raise(SIGTERM/SIGINT) good enough?

Perhaps. It is a slight lie. We _didn't_ get a SIGTERM, and anybody
looking at our exit code to find out why we died would be misled. But
the most important thing is that we die and that the exit status is
non-zero.

-Peff

Re: [BUG] MSVC: error box when interrupting `gitlog` by quitting less

[Johannes Sixt]

Please do not cull the Cc list.

Am 3/28/2014 11:07, schrieb Marat Radchenko:
> Jeff King <peff <at> peff.net> writes:
> 
>>
>> I'm not sure what an actual SIGPIPE death looks like on Windows.
> 
> There is no SIGPIPE death on Windows due to total absence of SIGPIPE.
> raise(unsupported int) just causes ugly "git.exe has stopped working"
> window and possibly ends up as SIGABT (I don't know how to check this).

This happens "only" with newer Microsoft C runtime libraries. They do not
return EINVAL (because that usually indicates a bug caused by insufficient
checks before the function call), but crash the program by default in the
way that you observed.

>> What
>> happens if git is still writing data to the pager and the pager exits?
>> Does it receive a signal of some sort?

No; the write attempt returns with EPIPE.

> 
> I'm not sure what you mean, sorry. check_pipe properly detects pager exit.
> The problem is with the way it tries to die.
> 
>> The point of the code in check_pipe is to simulate that death. So
>> whatever happens to git in that case is what we would want to happen
>> when we call raise(SIGPIPE).
> 
> That's what I'm talking about. On Windows, you can't raise(SIGPIPE).
> You can only raise(Windows_supported_signal) where signal is one of:
> SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, SIGTERM as MSDN tells us.

Correct. All other signal number should return EINVAL. But, as I said,
that does not happen by default.

The correct solution is to link against invalidcontinue.obj in the MSVC
build. This is a compiler-provided object file that changes the default
behavior to the "expected" kind, i.e., C runtime functions return EINVAL
when appropriate instead of crashing the application.

>> A possibly simpler option would be to just have the MSVC build skip the
>> raise() call, and do the exit(141) that comes just after. That is
>> probably close enough simulation of SIGPIPE death.

Correct. The MinGW build uses an older C runtime library, which does not
have the strange default behavior, and we do use that exit(141). And with
the fix to the MSVC build suggested above, that version would do likewise.

-- Hannes

[PATCH] MSVC: link in invalidcontinue.obj for better POSIX compatibility

[Marat Radchenko]

This patch fixes crashes caused by quitting from PAGER.

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---

> Please do not cull the Cc list.

That was gmane web interface.

> The correct solution is to link against invalidcontinue.obj in the MSVC
> build. This is a compiler-provided object file that changes the default
> behavior to the "expected" kind, i.e., C runtime functions return EINVAL
> when appropriate instead of crashing the application.

Thanks for a hint.

 config.mak.uname | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.mak.uname b/config.mak.uname
index 38c60af..8e7ec6e 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -366,7 +366,7 @@ ifeq ($(uname_S),Windows)
 		compat/win32/dirent.o
 	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
-	EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib
+	EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib invalidcontinue.obj
 	PTHREAD_LIBS =
 	lib =
 ifndef DEBUG
-- 
1.9.1

Re: [PATCH] MSVC: link in invalidcontinue.obj for better POSIX compatibility

[Junio C Hamano]

Marat Radchenko <marat@slonopotamus.org> writes:

> This patch fixes crashes caused by quitting from PAGER.

Can you elaborate a bit more on the underlying cause, summarizing
what you learned from this discussion, so that those who read "git
log" output two weeks from now do not have to come back to this
thread in the mail archive in order to figure out why we suddenly
needs to link with yet another library?

Thanks.

> Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
> ---
>
>> Please do not cull the Cc list.
>
> That was gmane web interface.
>
>> The correct solution is to link against invalidcontinue.obj in the MSVC
>> build. This is a compiler-provided object file that changes the default
>> behavior to the "expected" kind, i.e., C runtime functions return EINVAL
>> when appropriate instead of crashing the application.
>
> Thanks for a hint.
>
>  config.mak.uname | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index 38c60af..8e7ec6e 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -366,7 +366,7 @@ ifeq ($(uname_S),Windows)
>  		compat/win32/dirent.o
>  	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
>  	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
> -	EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib
> +	EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib invalidcontinue.obj
>  	PTHREAD_LIBS =
>  	lib =
>  ifndef DEBUG

Re: [PATCH] MSVC: link in invalidcontinue.obj for better POSIX compatibility

[Marat Radchenko]

Junio C Hamano <gitster <at> pobox.com> writes:

> > This patch fixes crashes caused by quitting from PAGER.
> 
> Can you elaborate a bit more on the underlying cause, summarizing
> what you learned from this discussion, so that those who read "git
> log" output two weeks from now do not have to come back to this
> thread in the mail archive in order to figure out why we suddenly
> needs to link with yet another library?

Without linking to that obj, Windows abort()'s instead of setting
errno=EINVAL when invalid arguments are passed to standard functions.
In this particular case, when PAGER quits and git detects it with
errno=EPIPE on write(), git tries raise(SIGPIPE) but since there is no
SIGPIPE on Windows, it is treated as invalid argument, causing abort()
and crash report window.

Linking in invalidcontinue.obj (provided along with MS compiler) allows
raise(SIGPIPE) to return with errno=EINVAL. While testing MSVC=1 git,
I found several more cases with same sympthoms (and also fixed by
given patch).

Re: [PATCH] MSVC: link in invalidcontinue.obj for better POSIX compatibility

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Marat Radchenko <marat@slonopotamus.org> writes:
>
>> This patch fixes crashes caused by quitting from PAGER.
>
> Can you elaborate a bit more on the underlying cause, summarizing
> what you learned from this discussion, so that those who read "git
> log" output two weeks from now do not have to come back to this
> thread in the mail archive in order to figure out why we suddenly
> needs to link with yet another library?
>
> Thanks.

Just to avoid getting misunderstood, I am not asking it to be
explained to me in an e-mail.  I want to see a patch with its
proposed commit log message to explain it to readers of "git log".

Thanks.

[PATCH v2] MSVC: link in invalidcontinue.obj for better POSIX compatibility

[Marat Radchenko]

By default, Windows abort()'s instead of setting
errno=EINVAL when invalid arguments are passed to standard functions.

For example, when PAGER quits and git detects it with
errno=EPIPE on write(), check_pipe() in write_or_die.c tries raise(SIGPIPE)
but since there is no SIGPIPE on Windows, it is treated as invalid argument,
causing abort() and crash report window.

Linking in invalidcontinue.obj (provided along with MS compiler) allows
raise(SIGPIPE) to return with errno=EINVAL.

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
---
 config.mak.uname | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.mak.uname b/config.mak.uname
index 38c60af..8e7ec6e 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -366,7 +366,7 @@ ifeq ($(uname_S),Windows)
 		compat/win32/dirent.o
 	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
 	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
-	EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib
+	EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib invalidcontinue.obj
 	PTHREAD_LIBS =
 	lib =
 ifndef DEBUG
-- 
1.8.3.2

Re: [PATCH v2] MSVC: link in invalidcontinue.obj for better POSIX compatibility

[Junio C Hamano]

Marat Radchenko <marat@slonopotamus.org> writes:

> By default, Windows abort()'s instead of setting
> errno=EINVAL when invalid arguments are passed to standard functions.
>
> For example, when PAGER quits and git detects it with
> errno=EPIPE on write(), check_pipe() in write_or_die.c tries raise(SIGPIPE)
> but since there is no SIGPIPE on Windows, it is treated as invalid argument,
> causing abort() and crash report window.
>
> Linking in invalidcontinue.obj (provided along with MS compiler) allows
> raise(SIGPIPE) to return with errno=EINVAL.
>
> Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
> ---

Thanks; will queue.

>  config.mak.uname | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index 38c60af..8e7ec6e 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -366,7 +366,7 @@ ifeq ($(uname_S),Windows)
>  		compat/win32/dirent.o
>  	COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\"
>  	BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib
> -	EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib
> +	EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib invalidcontinue.obj
>  	PTHREAD_LIBS =
>  	lib =
>  ifndef DEBUG

[PATCH v3 09/19] pkt-line: move a misplaced comment

[Jeff King]

The comment describing the packet writing interface was
originally written above packet_write, but migrated to be
above safe_write in f3a3214, probably because it is meant to
generally describe the packet writing interface and not a
single function. Let's move it into the header file, where
users of the interface are more likely to see it.

Signed-off-by: Jeff King <peff@peff.net>
---
 pkt-line.c | 15 ---------------
 pkt-line.h | 14 +++++++++++++-
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index eaba15f..5138f47 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -46,21 +46,6 @@ static void packet_trace(const char *buf, unsigned int len, int write)
 	strbuf_release(&out);
 }
 
-/*
- * Write a packetized stream, where each line is preceded by
- * its length (including the header) as a 4-byte hex number.
- * A length of 'zero' means end of stream (and a length of 1-3
- * would be an error).
- *
- * This is all pretty stupid, but we use this packetized line
- * format to make a streaming format possible without ever
- * over-running the read buffers. That way we'll never read
- * into what might be the pack data (which should go to another
- * process entirely).
- *
- * The writing side could use stdio, but since the reading
- * side can't, we stay with pure read/write interfaces.
- */
 ssize_t safe_write(int fd, const void *buf, ssize_t n)
 {
 	ssize_t nn = n;
diff --git a/pkt-line.h b/pkt-line.h
index 8cfeb0c..7a67e9c 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -5,7 +5,19 @@
 #include "strbuf.h"
 
 /*
- * Silly packetized line writing interface
+ * Write a packetized stream, where each line is preceded by
+ * its length (including the header) as a 4-byte hex number.
+ * A length of 'zero' means end of stream (and a length of 1-3
+ * would be an error).
+ *
+ * This is all pretty stupid, but we use this packetized line
+ * format to make a streaming format possible without ever
+ * over-running the read buffers. That way we'll never read
+ * into what might be the pack data (which should go to another
+ * process entirely).
+ *
+ * The writing side could use stdio, but since the reading
+ * side can't, we stay with pure read/write interfaces.
  */
 void packet_flush(int fd);
 void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 10/19] pkt-line: drop safe_write function

[Jeff King]

This is just write_or_die by another name. The one
distinction is that write_or_die will treat EPIPE specially
by suppressing error messages. That's fine, as we die by
SIGPIPE anyway (and in the off chance that it is disabled,
write_or_die will simulate it).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/receive-pack.c |  2 +-
 builtin/send-pack.c    |  2 +-
 fetch-pack.c           |  2 +-
 http-backend.c         |  8 ++++----
 pkt-line.c             | 21 ++-------------------
 pkt-line.h             |  1 -
 remote-curl.c          |  4 ++--
 send-pack.c            |  2 +-
 sideband.c             |  9 +++++----
 upload-pack.c          |  3 ++-
 10 files changed, 19 insertions(+), 35 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 62ba6e7..9129563 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -932,7 +932,7 @@ static void report(struct command *commands, const char *unpack_status)
 	if (use_sideband)
 		send_sideband(1, 1, buf.buf, buf.len, use_sideband);
 	else
-		safe_write(1, buf.buf, buf.len);
+		write_or_die(1, buf.buf, buf.len);
 	strbuf_release(&buf);
 }
 
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 57a46b2..8778519 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -79,7 +79,7 @@ static void print_helper_status(struct ref *ref)
 		}
 		strbuf_addch(&buf, '\n');
 
-		safe_write(1, buf.buf, buf.len);
+		write_or_die(1, buf.buf, buf.len);
 	}
 	strbuf_release(&buf);
 }
diff --git a/fetch-pack.c b/fetch-pack.c
index 27a3e80..b53a18f 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -247,7 +247,7 @@ static void send_request(struct fetch_pack_args *args,
 		send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
 		packet_flush(fd);
 	} else
-		safe_write(fd, buf->buf, buf->len);
+		write_or_die(fd, buf->buf, buf->len);
 }
 
 static void insert_one_alternate_ref(const struct ref *ref, void *unused)
diff --git a/http-backend.c b/http-backend.c
index f50e77f..8144f3a 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -70,7 +70,7 @@ static void format_write(int fd, const char *fmt, ...)
 	if (n >= sizeof(buffer))
 		die("protocol error: impossibly long line");
 
-	safe_write(fd, buffer, n);
+	write_or_die(fd, buffer, n);
 }
 
 static void http_status(unsigned code, const char *msg)
@@ -111,7 +111,7 @@ static void end_headers(void)
 
 static void end_headers(void)
 {
-	safe_write(1, "\r\n", 2);
+	write_or_die(1, "\r\n", 2);
 }
 
 __attribute__((format (printf, 1, 2)))
@@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)
 	hdr_int(content_length, buf->len);
 	hdr_str(content_type, type);
 	end_headers();
-	safe_write(1, buf->buf, buf->len);
+	write_or_die(1, buf->buf, buf->len);
 }
 
 static void send_local_file(const char *the_type, const char *name)
@@ -185,7 +185,7 @@ static void send_local_file(const char *the_type, const char *name)
 			die_errno("Cannot read '%s'", p);
 		if (!n)
 			break;
-		safe_write(1, buf, n);
+		write_or_die(1, buf, n);
 	}
 	close(fd);
 	free(buf);
diff --git a/pkt-line.c b/pkt-line.c
index 5138f47..699c2dd 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -46,23 +46,6 @@ static void packet_trace(const char *buf, unsigned int len, int write)
 	strbuf_release(&out);
 }
 
-ssize_t safe_write(int fd, const void *buf, ssize_t n)
-{
-	ssize_t nn = n;
-	while (n) {
-		int ret = xwrite(fd, buf, n);
-		if (ret > 0) {
-			buf = (char *) buf + ret;
-			n -= ret;
-			continue;
-		}
-		if (!ret)
-			die("write error (disk full?)");
-		die_errno("write error");
-	}
-	return nn;
-}
-
 /*
  * If we buffered things up above (we don't, but we should),
  * we'd flush it here
@@ -70,7 +53,7 @@ void packet_flush(int fd)
 void packet_flush(int fd)
 {
 	packet_trace("0000", 4, 1);
-	safe_write(fd, "0000", 4);
+	write_or_die(fd, "0000", 4);
 }
 
 void packet_buf_flush(struct strbuf *buf)
@@ -106,7 +89,7 @@ void packet_write(int fd, const char *fmt, ...)
 	va_start(args, fmt);
 	n = format_packet(fmt, args);
 	va_end(args);
-	safe_write(fd, buffer, n);
+	write_or_die(fd, buffer, n);
 }
 
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
diff --git a/pkt-line.h b/pkt-line.h
index 7a67e9c..3b6c19c 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -27,6 +27,5 @@ int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
 int packet_read_line(int fd, char *buffer, unsigned size);
 int packet_read(int fd, char *buffer, unsigned size);
 int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
-ssize_t safe_write(int, const void *, ssize_t);
 
 #endif
diff --git a/remote-curl.c b/remote-curl.c
index 933c69a..7be4b53 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -685,7 +685,7 @@ static int fetch_git(struct discovery *heads,
 
 	err = rpc_service(&rpc, heads);
 	if (rpc.result.len)
-		safe_write(1, rpc.result.buf, rpc.result.len);
+		write_or_die(1, rpc.result.buf, rpc.result.len);
 	strbuf_release(&rpc.result);
 	strbuf_release(&preamble);
 	free(depth_arg);
@@ -805,7 +805,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
 
 	err = rpc_service(&rpc, heads);
 	if (rpc.result.len)
-		safe_write(1, rpc.result.buf, rpc.result.len);
+		write_or_die(1, rpc.result.buf, rpc.result.len);
 	strbuf_release(&rpc.result);
 	free(argv);
 	return err;
diff --git a/send-pack.c b/send-pack.c
index e91cbe2..bde796b 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -280,7 +280,7 @@ int send_pack(struct send_pack_args *args,
 			send_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
 		}
 	} else {
-		safe_write(out, req_buf.buf, req_buf.len);
+		write_or_die(out, req_buf.buf, req_buf.len);
 		packet_flush(out);
 	}
 	strbuf_release(&req_buf);
diff --git a/sideband.c b/sideband.c
index d5ffa1c..8f7b25b 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,3 +1,4 @@
+#include "cache.h"
 #include "pkt-line.h"
 #include "sideband.h"
 
@@ -108,7 +109,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 			} while (len);
 			continue;
 		case 1:
-			safe_write(out, buf + pf+1, len);
+			write_or_die(out, buf + pf+1, len);
 			continue;
 		default:
 			fprintf(stderr, "%s: protocol error: bad band #%d\n",
@@ -138,12 +139,12 @@ ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet
 		if (0 <= band) {
 			sprintf(hdr, "%04x", n + 5);
 			hdr[4] = band;
-			safe_write(fd, hdr, 5);
+			write_or_die(fd, hdr, 5);
 		} else {
 			sprintf(hdr, "%04x", n + 4);
-			safe_write(fd, hdr, 4);
+			write_or_die(fd, hdr, 4);
 		}
-		safe_write(fd, p, n);
+		write_or_die(fd, p, n);
 		p += n;
 		sz -= n;
 	}
diff --git a/upload-pack.c b/upload-pack.c
index 63cea91..c2b2c61 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -69,7 +69,8 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
 		xwrite(fd, data, sz);
 		return sz;
 	}
-	return safe_write(fd, data, sz);
+	write_or_die(fd, data, sz);
+	return sz;
 }
 
 static FILE *pack_pipe = NULL;
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 11/19] pkt-line: provide a generic reading function with options

[Jeff King]

Originally we had a single function for reading packetized
data: packet_read_line. Commit 46284dd grew a more "gentle"
form, packet_read, that returns an error instead of dying
upon reading a truncated input stream. However, it is not
clear from the names which should be called, or what the
difference is.

Let's instead make packet_read be a generic public interface
that can take option flags, and update the single callsite
that uses it. This is less code, more clear, and paves the
way for introducing more options into the generic interface
later. The function signature is changed, so there should be
no hidden conflicts with topics in flight.

While we're at it, we'll document how error conditions are
handled based on the options, and rename the confusing
"return_line_fail" option to "gentle_on_eof".  While we are
cleaning up the names, we can drop the "return_line_fail"
checks in packet_read_internal entirely.  They look like
this:

  ret = safe_read(..., return_line_fail);
  if (return_line_fail && ret < 0)
	  ...

The check for return_line_fail is a no-op; safe_read will
only ever return an error value if return_line_fail was true
in the first place.

Signed-off-by: Jeff King <peff@peff.net>
---
 connect.c  |  3 ++-
 pkt-line.c | 21 ++++++++-------------
 pkt-line.h | 27 ++++++++++++++++++++++++++-
 3 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/connect.c b/connect.c
index 49e56ba..0aa202f 100644
--- a/connect.c
+++ b/connect.c
@@ -76,7 +76,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
 		char *name;
 		int len, name_len;
 
-		len = packet_read(in, buffer, sizeof(buffer));
+		len = packet_read(in, buffer, sizeof(buffer),
+				  PACKET_READ_GENTLE_ON_EOF);
 		if (len < 0)
 			die_initial_contact(got_at_least_one_head);
 
diff --git a/pkt-line.c b/pkt-line.c
index 699c2dd..8700cf8 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -103,13 +103,13 @@ static int safe_read(int fd, void *buffer, unsigned size, int return_line_fail)
 	strbuf_add(buf, buffer, n);
 }
 
-static int safe_read(int fd, void *buffer, unsigned size, int return_line_fail)
+static int safe_read(int fd, void *buffer, unsigned size, int options)
 {
 	ssize_t ret = read_in_full(fd, buffer, size);
 	if (ret < 0)
 		die_errno("read error");
 	else if (ret < size) {
-		if (return_line_fail)
+		if (options & PACKET_READ_GENTLE_ON_EOF)
 			return -1;
 
 		die("The remote end hung up unexpectedly");
@@ -143,13 +143,13 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int return_
 	return len;
 }
 
-static int packet_read_internal(int fd, char *buffer, unsigned size, int return_line_fail)
+int packet_read(int fd, char *buffer, unsigned size, int options)
 {
 	int len, ret;
 	char linelen[4];
 
-	ret = safe_read(fd, linelen, 4, return_line_fail);
-	if (return_line_fail && ret < 0)
+	ret = safe_read(fd, linelen, 4, options);
+	if (ret < 0)
 		return ret;
 	len = packet_length(linelen);
 	if (len < 0)
@@ -161,22 +161,17 @@ int packet_read_line(int fd, char *buffer, unsigned size)
 	len -= 4;
 	if (len >= size)
 		die("protocol error: bad line length %d", len);
-	ret = safe_read(fd, buffer, len, return_line_fail);
-	if (return_line_fail && ret < 0)
+	ret = safe_read(fd, buffer, len, options);
+	if (ret < 0)
 		return ret;
 	buffer[len] = 0;
 	packet_trace(buffer, len, 0);
 	return len;
 }
 
-int packet_read(int fd, char *buffer, unsigned size)
-{
-	return packet_read_internal(fd, buffer, size, 1);
-}
-
 int packet_read_line(int fd, char *buffer, unsigned size)
 {
-	return packet_read_internal(fd, buffer, size, 0);
+	return packet_read(fd, buffer, size, 0);
 }
 
 int packet_get_line(struct strbuf *out,
diff --git a/pkt-line.h b/pkt-line.h
index 3b6c19c..8cd326c 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -24,8 +24,33 @@ int packet_read_line(int fd, char *buffer, unsigned size);
 void packet_buf_flush(struct strbuf *buf);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
 
+/*
+ * Read a packetized line from the descriptor into the buffer, which must be at
+ * least size bytes long. The return value specifies the number of bytes read
+ * into the buffer.
+ *
+ * If options does not contain PACKET_READ_GENTLE_ON_EOF, we will die under any
+ * of the following conditions:
+ *
+ *   1. Read error from descriptor.
+ *
+ *   2. Protocol error from the remote (e.g., bogus length characters).
+ *
+ *   3. Receiving a packet larger than "size" bytes.
+ *
+ *   4. Truncated output from the remote (e.g., we expected a packet but got
+ *      EOF, or we got a partial packet followed by EOF).
+ *
+ * If options does contain PACKET_READ_GENTLE_ON_EOF, we will not die on
+ * condition 4 (truncated input), but instead return -1. However, we will still
+ * die for the other 3 conditions.
+ */
+#define PACKET_READ_GENTLE_ON_EOF (1u<<0)
+int packet_read(int fd, char *buffer, unsigned size, int options);
+
+/* Historical convenience wrapper for packet_read that sets no options */
 int packet_read_line(int fd, char *buffer, unsigned size);
-int packet_read(int fd, char *buffer, unsigned size);
+
 int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
 
 #endif
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 12/19] pkt-line: teach packet_read_line to chomp newlines

[Jeff King]

The packets sent during ref negotiation are all terminated
by newline; even though the code to chomp these newlines is
short, we end up doing it in a lot of places.

This patch teaches packet_read_line to auto-chomp the
trailing newline; this lets us get rid of a lot of inline
chomping code.

As a result, some call-sites which are not reading
line-oriented data (e.g., when reading chunks of packfiles
alongside sideband) transition away from packet_read_line to
the generic packet_read interface. This patch converts all
of the existing callsites.

Since the function signature of packet_read_line does not
change (but its behavior does), there is a possibility of
new callsites being introduced in later commits, silently
introducing an incompatibility.  However, since a later
patch in this series will change the signature, such a
commit would have to be merged directly into this commit,
not to the tip of the series; we can therefore ignore the
issue.

This is an internal cleanup and should produce no change of
behavior in the normal case. However, there is one corner
case to note. Callers of packet_read_line have never been
able to tell the difference between a flush packet ("0000")
and an empty packet ("0004"), as both cause packet_read_line
to return a length of 0. Readers treat them identically,
even though Documentation/technical/protocol-common.txt says
we must not; it also says that implementations should not
send an empty pkt-line.

By stripping out the newline before the result gets to the
caller, we will now treat the newline-only packet ("0005\n")
the same as an empty packet, which in turn gets treated like
a flush packet. In practice this doesn't matter, as neither
empty nor newline-only packets are part of git's protocols
(at least not for the line-oriented bits, and readers who
are not expecting line-oriented packets will be calling
packet_read directly, anyway). But even if we do decide to
care about the distinction later, it is orthogonal to this
patch.  The right place to tighten would be to stop treating
empty packets as flush packets, and this change does not
make doing so any harder.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/archive.c        | 2 --
 builtin/fetch-pack.c     | 2 --
 builtin/receive-pack.c   | 2 --
 builtin/upload-archive.c | 4 ----
 connect.c                | 5 ++---
 daemon.c                 | 2 +-
 fetch-pack.c             | 2 --
 pkt-line.c               | 7 ++++++-
 pkt-line.h               | 9 ++++++++-
 remote-curl.c            | 6 +++---
 send-pack.c              | 6 +-----
 sideband.c               | 2 +-
 upload-pack.c            | 8 --------
 13 files changed, 22 insertions(+), 35 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index 9a1cfd3..d381ac4 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -56,8 +56,6 @@ static int run_remote_archiver(int argc, const char **argv,
 	len = packet_read_line(fd[0], buf, sizeof(buf));
 	if (!len)
 		die(_("git archive: expected ACK/NAK, got EOF"));
-	if (buf[len-1] == '\n')
-		buf[--len] = 0;
 	if (strcmp(buf, "ACK")) {
 		if (len > 5 && !prefixcmp(buf, "NACK "))
 			die(_("git archive: NACK %s"), buf + 5);
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 940ae35..f73664f 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -105,8 +105,6 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 				int n = packet_read_line(0, line, sizeof(line));
 				if (!n)
 					break;
-				if (line[n-1] == '\n')
-					n--;
 				string_list_append(&sought, xmemdupz(line, n));
 			}
 		}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9129563..6679e63 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -763,8 +763,6 @@ static struct command *read_head_info(void)
 		len = packet_read_line(0, line, sizeof(line));
 		if (!len)
 			break;
-		if (line[len-1] == '\n')
-			line[--len] = 0;
 		if (len < 83 ||
 		    line[40] != ' ' ||
 		    line[81] != ' ' ||
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 3393cef..7d367b5 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -40,10 +40,6 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 		if (sent_argv.argc > MAX_ARGS)
 		    die("Too many options (>%d)", MAX_ARGS - 1);
 
-		if (buf[len-1] == '\n') {
-			buf[--len] = 0;
-		}
-
 		if (prefixcmp(buf, arg_cmd))
 			die("'argument' token or flush expected");
 		argv_array_push(&sent_argv, buf + strlen(arg_cmd));
diff --git a/connect.c b/connect.c
index 0aa202f..fe8eb01 100644
--- a/connect.c
+++ b/connect.c
@@ -77,14 +77,13 @@ struct ref **get_remote_heads(int in, struct ref **list,
 		int len, name_len;
 
 		len = packet_read(in, buffer, sizeof(buffer),
-				  PACKET_READ_GENTLE_ON_EOF);
+				  PACKET_READ_GENTLE_ON_EOF |
+				  PACKET_READ_CHOMP_NEWLINE);
 		if (len < 0)
 			die_initial_contact(got_at_least_one_head);
 
 		if (!len)
 			break;
-		if (buffer[len-1] == '\n')
-			buffer[--len] = 0;
 
 		if (len > 4 && !prefixcmp(buffer, "ERR "))
 			die("remote error: %s", buffer + 4);
diff --git a/daemon.c b/daemon.c
index 4602b46..4f5cd61 100644
--- a/daemon.c
+++ b/daemon.c
@@ -612,7 +612,7 @@ static int execute(void)
 		loginfo("Connection from %s:%s", addr, port);
 
 	alarm(init_timeout ? init_timeout : timeout);
-	pktlen = packet_read_line(0, line, sizeof(line));
+	pktlen = packet_read(0, line, sizeof(line), 0);
 	alarm(0);
 
 	len = strlen(line);
diff --git a/fetch-pack.c b/fetch-pack.c
index b53a18f..f830db2 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -220,8 +220,6 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 
 	if (!len)
 		die("git fetch-pack: expected ACK/NAK, got EOF");
-	if (line[len-1] == '\n')
-		line[--len] = 0;
 	if (!strcmp(line, "NAK"))
 		return NAK;
 	if (!prefixcmp(line, "ACK ")) {
diff --git a/pkt-line.c b/pkt-line.c
index 8700cf8..dc11c40 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -164,6 +164,11 @@ int packet_read(int fd, char *buffer, unsigned size, int options)
 	ret = safe_read(fd, buffer, len, options);
 	if (ret < 0)
 		return ret;
+
+	if ((options & PACKET_READ_CHOMP_NEWLINE) &&
+	    len && buffer[len-1] == '\n')
+		len--;
+
 	buffer[len] = 0;
 	packet_trace(buffer, len, 0);
 	return len;
@@ -171,7 +176,7 @@ int packet_read_line(int fd, char *buffer, unsigned size)
 
 int packet_read_line(int fd, char *buffer, unsigned size)
 {
-	return packet_read(fd, buffer, size, 0);
+	return packet_read(fd, buffer, size, PACKET_READ_CHOMP_NEWLINE);
 }
 
 int packet_get_line(struct strbuf *out,
diff --git a/pkt-line.h b/pkt-line.h
index 8cd326c..5d2fb42 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -44,11 +44,18 @@ int packet_read(int fd, char *buffer, unsigned size, int options);
  * If options does contain PACKET_READ_GENTLE_ON_EOF, we will not die on
  * condition 4 (truncated input), but instead return -1. However, we will still
  * die for the other 3 conditions.
+ *
+ * If options contains PACKET_READ_CHOMP_NEWLINE, a trailing newline (if
+ * present) is removed from the buffer before returning.
  */
 #define PACKET_READ_GENTLE_ON_EOF (1u<<0)
+#define PACKET_READ_CHOMP_NEWLINE (1u<<1)
 int packet_read(int fd, char *buffer, unsigned size, int options);
 
-/* Historical convenience wrapper for packet_read that sets no options */
+/*
+ * Convenience wrapper for packet_read that is not gentle, and sets the
+ * CHOMP_NEWLINE option.
+ */
 int packet_read_line(int fd, char *buffer, unsigned size);
 
 int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
diff --git a/remote-curl.c b/remote-curl.c
index 7be4b53..b28f965 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -308,7 +308,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,
 
 	if (!avail) {
 		rpc->initial_buffer = 0;
-		avail = packet_read_line(rpc->out, rpc->buf, rpc->alloc);
+		avail = packet_read(rpc->out, rpc->buf, rpc->alloc, 0);
 		if (!avail)
 			return 0;
 		rpc->pos = 0;
@@ -425,7 +425,7 @@ static int post_rpc(struct rpc_state *rpc)
 			break;
 		}
 
-		n = packet_read_line(rpc->out, buf, left);
+		n = packet_read(rpc->out, buf, left, 0);
 		if (!n)
 			break;
 		rpc->len += n;
@@ -579,7 +579,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
 	rpc->hdr_accept = strbuf_detach(&buf, NULL);
 
 	while (!err) {
-		int n = packet_read_line(rpc->out, rpc->buf, rpc->alloc);
+		int n = packet_read(rpc->out, rpc->buf, rpc->alloc, 0);
 		if (!n)
 			break;
 		rpc->pos = 0;
diff --git a/send-pack.c b/send-pack.c
index bde796b..8c230bf 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -111,10 +111,7 @@ static int receive_status(int in, struct ref *refs)
 	int len = packet_read_line(in, line, sizeof(line));
 	if (prefixcmp(line, "unpack "))
 		return error("did not receive remote status");
-	if (strcmp(line, "unpack ok\n")) {
-		char *p = line + strlen(line) - 1;
-		if (*p == '\n')
-			*p = '\0';
+	if (strcmp(line, "unpack ok")) {
 		error("unpack failed: %s", line + 7);
 		ret = -1;
 	}
@@ -131,7 +128,6 @@ static int receive_status(int in, struct ref *refs)
 			break;
 		}
 
-		line[strlen(line)-1] = '\0';
 		refname = line + 3;
 		msg = strchr(refname, ' ');
 		if (msg)
diff --git a/sideband.c b/sideband.c
index 8f7b25b..15cc1ae 100644
--- a/sideband.c
+++ b/sideband.c
@@ -38,7 +38,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 
 	while (1) {
 		int band, len;
-		len = packet_read_line(in_stream, buf + pf, LARGE_PACKET_MAX);
+		len = packet_read(in_stream, buf + pf, LARGE_PACKET_MAX, 0);
 		if (len == 0)
 			break;
 		if (len < 1) {
diff --git a/upload-pack.c b/upload-pack.c
index c2b2c61..7446cb7 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -50,13 +50,6 @@ static void reset_timeout(void)
 	alarm(timeout);
 }
 
-static int strip(char *line, int len)
-{
-	if (len && line[len-1] == '\n')
-		line[--len] = 0;
-	return len;
-}
-
 static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
 {
 	if (use_sideband)
@@ -447,7 +440,6 @@ static int get_common_commits(void)
 			got_other = 0;
 			continue;
 		}
-		strip(line, len);
 		if (!prefixcmp(line, "have ")) {
 			switch (got_sha1(line+5, sha1)) {
 			case -1: /* they have what we do not */
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 13/19] pkt-line: move LARGE_PACKET_MAX definition from sideband

[Jeff King]

Having the packet sizes defined near the packet read/write
functions makes more sense.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c     | 1 +
 pkt-line.h | 3 +++
 sideband.h | 3 ---
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/http.c b/http.c
index d9d1aad..8803c70 100644
--- a/http.c
+++ b/http.c
@@ -5,6 +5,7 @@
 #include "url.h"
 #include "credential.h"
 #include "version.h"
+#include "pkt-line.h"
 
 int active_requests;
 int http_is_verbose;
diff --git a/pkt-line.h b/pkt-line.h
index 5d2fb42..6927ea5 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -58,6 +58,9 @@ int packet_read_line(int fd, char *buffer, unsigned size);
  */
 int packet_read_line(int fd, char *buffer, unsigned size);
 
+#define DEFAULT_PACKET_MAX 1000
+#define LARGE_PACKET_MAX 65520
+
 int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
 
 #endif
diff --git a/sideband.h b/sideband.h
index d72db35..e46bed0 100644
--- a/sideband.h
+++ b/sideband.h
@@ -4,9 +4,6 @@
 #define SIDEBAND_PROTOCOL_ERROR -2
 #define SIDEBAND_REMOTE_ERROR -1
 
-#define DEFAULT_PACKET_MAX 1000
-#define LARGE_PACKET_MAX 65520
-
 int recv_sideband(const char *me, int in_stream, int out);
 ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
 
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 14/19] pkt-line: provide a LARGE_PACKET_MAX static buffer

[Jeff King]

Most of the callers of packet_read_line just read into a
static 1000-byte buffer (callers which handle arbitrary
binary data already use LARGE_PACKET_MAX). This works fine
in practice, because:

  1. The only variable-sized data in these lines is a ref
     name, and refs tend to be a lot shorter than 1000
     characters.

  2. When sending ref lines, git-core always limits itself
     to 1000 byte packets.

However, the only limit given in the protocol specification
in Documentation/technical/protocol-common.txt is
LARGE_PACKET_MAX; the 1000 byte limit is mentioned only in
pack-protocol.txt, and then only describing what we write,
not as a specific limit for readers.

This patch lets us bump the 1000-byte limit to
LARGE_PACKET_MAX. Even though git-core will never write a
packet where this makes a difference, there are two good
reasons to do this:

  1. Other git implementations may have followed
     protocol-common.txt and used a larger maximum size. We
     don't bump into it in practice because it would involve
     very long ref names.

  2. We may want to increase the 1000-byte limit one day.
     Since packets are transferred before any capabilities,
     it's difficult to do this in a backwards-compatible
     way. But if we bump the size of buffer the readers can
     handle, eventually older versions of git will be
     obsolete enough that we can justify bumping the
     writers, as well. We don't have plans to do this
     anytime soon, but there is no reason not to start the
     clock ticking now.

Just bumping all of the reading bufs to LARGE_PACKET_MAX
would waste memory. Instead, since most readers just read
into a temporary buffer anyway, let's provide a single
static buffer that all callers can use. We can further wrap
this detail away by having the packet_read_line wrapper just
use the buffer transparently and return a pointer to the
static storage.  That covers most of the cases, and the
remaining ones already read into their own LARGE_PACKET_MAX
buffers.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/archive.c        | 15 +++++++--------
 builtin/fetch-pack.c     |  7 +++----
 builtin/receive-pack.c   |  6 +++---
 builtin/upload-archive.c |  7 ++-----
 connect.c                |  4 ++--
 daemon.c                 |  4 ++--
 fetch-pack.c             | 12 ++++++------
 pkt-line.c               |  9 +++++++--
 pkt-line.h               |  9 +++++++--
 send-pack.c              |  7 +++----
 upload-pack.c            | 12 +++++-------
 11 files changed, 47 insertions(+), 45 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index d381ac4..49178f1 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -27,8 +27,8 @@ static int run_remote_archiver(int argc, const char **argv,
 			       const char *remote, const char *exec,
 			       const char *name_hint)
 {
-	char buf[LARGE_PACKET_MAX];
-	int fd[2], i, len, rv;
+	char *buf;
+	int fd[2], i, rv;
 	struct transport *transport;
 	struct remote *_remote;
 
@@ -53,19 +53,18 @@ static int run_remote_archiver(int argc, const char **argv,
 		packet_write(fd[1], "argument %s\n", argv[i]);
 	packet_flush(fd[1]);
 
-	len = packet_read_line(fd[0], buf, sizeof(buf));
-	if (!len)
+	buf = packet_read_line(fd[0], NULL);
+	if (!buf)
 		die(_("git archive: expected ACK/NAK, got EOF"));
 	if (strcmp(buf, "ACK")) {
-		if (len > 5 && !prefixcmp(buf, "NACK "))
+		if (!prefixcmp(buf, "NACK "))
 			die(_("git archive: NACK %s"), buf + 5);
-		if (len > 4 && !prefixcmp(buf, "ERR "))
+		if (!prefixcmp(buf, "ERR "))
 			die(_("remote error: %s"), buf + 4);
 		die(_("git archive: protocol error"));
 	}
 
-	len = packet_read_line(fd[0], buf, sizeof(buf));
-	if (len)
+	if (packet_read_line(fd[0], NULL))
 		die(_("git archive: expected a flush"));
 
 	/* Now, start reading from fd[0] and spit it out to stdout */
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index f73664f..c21cc2c 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -100,12 +100,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			/* in stateless RPC mode we use pkt-line to read
 			 * from stdin, until we get a flush packet
 			 */
-			static char line[1000];
 			for (;;) {
-				int n = packet_read_line(0, line, sizeof(line));
-				if (!n)
+				char *line = packet_read_line(0, NULL);
+				if (!line)
 					break;
-				string_list_append(&sought, xmemdupz(line, n));
+				string_list_append(&sought, xstrdup(line));
 			}
 		}
 		else {
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 6679e63..ccebd74 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -754,14 +754,14 @@ static struct command *read_head_info(void)
 	struct command *commands = NULL;
 	struct command **p = &commands;
 	for (;;) {
-		static char line[1000];
+		char *line;
 		unsigned char old_sha1[20], new_sha1[20];
 		struct command *cmd;
 		char *refname;
 		int len, reflen;
 
-		len = packet_read_line(0, line, sizeof(line));
-		if (!len)
+		line = packet_read_line(0, &len);
+		if (!line)
 			break;
 		if (len < 83 ||
 		    line[40] != ' ' ||
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 7d367b5..2a94675 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -21,8 +21,6 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 {
 	struct argv_array sent_argv = ARGV_ARRAY_INIT;
 	const char *arg_cmd = "argument ";
-	char buf[4096];
-	int len;
 
 	if (argc != 2)
 		usage(upload_archive_usage);
@@ -33,9 +31,8 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 	/* put received options in sent_argv[] */
 	argv_array_push(&sent_argv, "git-upload-archive");
 	for (;;) {
-		/* This will die if not enough free space in buf */
-		len = packet_read_line(0, buf, sizeof(buf));
-		if (len == 0)
+		char *buf = packet_read_line(0, NULL);
+		if (!buf)
 			break;	/* got a flush */
 		if (sent_argv.argc > MAX_ARGS)
 		    die("Too many options (>%d)", MAX_ARGS - 1);
diff --git a/connect.c b/connect.c
index fe8eb01..611ffb4 100644
--- a/connect.c
+++ b/connect.c
@@ -72,11 +72,11 @@ struct ref **get_remote_heads(int in, struct ref **list,
 	for (;;) {
 		struct ref *ref;
 		unsigned char old_sha1[20];
-		static char buffer[1000];
 		char *name;
 		int len, name_len;
+		char *buffer = packet_buffer;
 
-		len = packet_read(in, buffer, sizeof(buffer),
+		len = packet_read(in, packet_buffer, sizeof(packet_buffer),
 				  PACKET_READ_GENTLE_ON_EOF |
 				  PACKET_READ_CHOMP_NEWLINE);
 		if (len < 0)
diff --git a/daemon.c b/daemon.c
index 4f5cd61..3f70e79 100644
--- a/daemon.c
+++ b/daemon.c
@@ -604,7 +604,7 @@ static int execute(void)
 
 static int execute(void)
 {
-	static char line[1000];
+	char *line = packet_buffer;
 	int pktlen, len, i;
 	char *addr = getenv("REMOTE_ADDR"), *port = getenv("REMOTE_PORT");
 
@@ -612,7 +612,7 @@ static int execute(void)
 		loginfo("Connection from %s:%s", addr, port);
 
 	alarm(init_timeout ? init_timeout : timeout);
-	pktlen = packet_read(0, line, sizeof(line), 0);
+	pktlen = packet_read(0, packet_buffer, sizeof(packet_buffer), 0);
 	alarm(0);
 
 	len = strlen(line);
diff --git a/fetch-pack.c b/fetch-pack.c
index f830db2..66ff9ad 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -172,8 +172,8 @@ static void consume_shallow_list(struct fetch_pack_args *args, int fd)
 		 * shallow and unshallow commands every time there
 		 * is a block of have lines exchanged.
 		 */
-		char line[1000];
-		while (packet_read_line(fd, line, sizeof(line))) {
+		char *line;
+		while ((line = packet_read_line(fd, NULL))) {
 			if (!prefixcmp(line, "shallow "))
 				continue;
 			if (!prefixcmp(line, "unshallow "))
@@ -215,8 +215,8 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 
 static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 {
-	static char line[1000];
-	int len = packet_read_line(fd, line, sizeof(line));
+	int len;
+	char *line = packet_read_line(fd, &len);
 
 	if (!len)
 		die("git fetch-pack: expected ACK/NAK, got EOF");
@@ -346,11 +346,11 @@ static int find_common(struct fetch_pack_args *args,
 	state_len = req_buf.len;
 
 	if (args->depth > 0) {
-		char line[1024];
+		char *line;
 		unsigned char sha1[20];
 
 		send_request(args, fd[1], &req_buf);
-		while (packet_read_line(fd[0], line, sizeof(line))) {
+		while ((line = packet_read_line(fd[0], NULL))) {
 			if (!prefixcmp(line, "shallow ")) {
 				if (get_sha1_hex(line + 8, sha1))
 					die("invalid shallow line: %s", line);
diff --git a/pkt-line.c b/pkt-line.c
index dc11c40..55fb688 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "pkt-line.h"
 
+char packet_buffer[LARGE_PACKET_MAX];
 static const char *packet_trace_prefix = "git";
 static const char trace_key[] = "GIT_TRACE_PACKET";
 
@@ -174,9 +175,13 @@ int packet_read_line(int fd, char *buffer, unsigned size)
 	return len;
 }
 
-int packet_read_line(int fd, char *buffer, unsigned size)
+char *packet_read_line(int fd, int *len_p)
 {
-	return packet_read(fd, buffer, size, PACKET_READ_CHOMP_NEWLINE);
+	int len = packet_read(fd, packet_buffer, sizeof(packet_buffer),
+			      PACKET_READ_CHOMP_NEWLINE);
+	if (len_p)
+		*len_p = len;
+	return len ? packet_buffer : NULL;
 }
 
 int packet_get_line(struct strbuf *out,
diff --git a/pkt-line.h b/pkt-line.h
index 6927ea5..fa93e32 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -54,12 +54,17 @@ int packet_read_line(int fd, char *buffer, unsigned size);
 
 /*
  * Convenience wrapper for packet_read that is not gentle, and sets the
- * CHOMP_NEWLINE option.
+ * CHOMP_NEWLINE option. The return value is NULL for a flush packet,
+ * and otherwise points to a static buffer (that may be overwritten by
+ * subsequent calls). If the size parameter is not NULL, the length of the
+ * packet is written to it.
  */
-int packet_read_line(int fd, char *buffer, unsigned size);
+char *packet_read_line(int fd, int *size);
+
 
 #define DEFAULT_PACKET_MAX 1000
 #define LARGE_PACKET_MAX 65520
+extern char packet_buffer[LARGE_PACKET_MAX];
 
 int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
 
diff --git a/send-pack.c b/send-pack.c
index 8c230bf..7d172ef 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -106,9 +106,8 @@ static int receive_status(int in, struct ref *refs)
 static int receive_status(int in, struct ref *refs)
 {
 	struct ref *hint;
-	char line[1000];
 	int ret = 0;
-	int len = packet_read_line(in, line, sizeof(line));
+	char *line = packet_read_line(in, NULL);
 	if (prefixcmp(line, "unpack "))
 		return error("did not receive remote status");
 	if (strcmp(line, "unpack ok")) {
@@ -119,8 +118,8 @@ static int receive_status(int in, struct ref *refs)
 	while (1) {
 		char *refname;
 		char *msg;
-		len = packet_read_line(in, line, sizeof(line));
-		if (!len)
+		line = packet_read_line(in, NULL);
+		if (!line)
 			break;
 		if (prefixcmp(line, "ok ") && prefixcmp(line, "ng ")) {
 			error("invalid ref status from remote: %s", line);
diff --git a/upload-pack.c b/upload-pack.c
index 7446cb7..bc241ba 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -408,7 +408,6 @@ static int get_common_commits(void)
 
 static int get_common_commits(void)
 {
-	static char line[1000];
 	unsigned char sha1[20];
 	char last_hex[41];
 	int got_common = 0;
@@ -418,10 +417,10 @@ static int get_common_commits(void)
 	save_commit_buffer = 0;
 
 	for (;;) {
-		int len = packet_read_line(0, line, sizeof(line));
+		char *line = packet_read_line(0, NULL);
 		reset_timeout();
 
-		if (!len) {
+		if (!line) {
 			if (multi_ack == 2 && got_common
 			    && !got_other && ok_to_give_up()) {
 				sent_ready = 1;
@@ -567,8 +566,7 @@ static void receive_needs(void)
 static void receive_needs(void)
 {
 	struct object_array shallows = OBJECT_ARRAY_INIT;
-	static char line[1000];
-	int len, depth = 0;
+	int depth = 0;
 	int has_non_tip = 0;
 
 	shallow_nr = 0;
@@ -576,9 +574,9 @@ static void receive_needs(void)
 		struct object *o;
 		const char *features;
 		unsigned char sha1_buf[20];
-		len = packet_read_line(0, line, sizeof(line));
+		char *line = packet_read_line(0, NULL);
 		reset_timeout();
-		if (!len)
+		if (!line)
 			break;
 
 		if (!prefixcmp(line, "shallow ")) {
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 15/19] pkt-line: share buffer/descriptor reading implementation

[Jeff King]

The packet_read function reads from a descriptor. The
packet_get_line function is similar, but reads from an
in-memory buffer, and uses a completely separate
implementation. This patch teaches the generic packet_read
function to accept either source, and we can do away with
packet_get_line's implementation.

There are two other differences to account for between the
old and new functions. The first is that we used to read
into a strbuf, but now read into a fixed size buffer. The
only two callers are fine with that, and in fact it
simplifies their code, since they can use the same
static-buffer interface as the rest of the packet_read_line
callers (and we provide a similar convenience wrapper for
reading from a buffer rather than a descriptor).

This is technically an externally-visible behavior change in
that we used to accept arbitrary sized packets up to 65532
bytes, and now cap out at LARGE_PACKET_MAX, 65520. In
practice this doesn't matter, as we use it only for parsing
smart-http headers (of which there is exactly one defined,
and it is small and fixed-size). And any extension headers
would be breaking the protocol to go over LARGE_PACKET_MAX
anyway.

The other difference is that packet_get_line would return
on error rather than dying. However, both callers of
strbuf_get_line are actually improved by dying.

The first caller does its own error checking, but we can
drop that; as a result, we'll actually get more specific
reporting about protocol breakage when packet_read dies
internally. The only downside is that packet_read will not
print the smart-http URL that failed, but that's not a big
deal; anybody not debugging can already see the remote's URL
already, and anybody debugging would want to run with
GIT_CURL_VERBOSE anyway to see way more information.

The second caller, which is just trying to skip past any
extra smart-http headers (of which there are none defined,
but which we allow to keep room for future expansion), did
not error check at all. As a result, it would treat an error
just like a flush packet. The resulting mess would generally
cause an error later in get_remote_heads, but now we get
error reporting much closer to the source of the problem.

Signed-off-by: Jeff King <peff@peff.net>
---
This adds two options to the generic packet_read interface for which
many callers will just pass (NULL, 0).  We can hide that behind a
wrapper, but I was annoyed with the proliferation of wrappers from the
last round. Pick your poison.

 connect.c     |  3 ++-
 daemon.c      |  2 +-
 pkt-line.c    | 77 ++++++++++++++++++++++++++++++-----------------------------
 pkt-line.h    | 23 +++++++++++++-----
 remote-curl.c | 22 ++++++++---------
 sideband.c    |  2 +-
 6 files changed, 70 insertions(+), 59 deletions(-)

diff --git a/connect.c b/connect.c
index 611ffb4..061aa5b 100644
--- a/connect.c
+++ b/connect.c
@@ -76,7 +76,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
 		int len, name_len;
 		char *buffer = packet_buffer;
 
-		len = packet_read(in, packet_buffer, sizeof(packet_buffer),
+		len = packet_read(in, NULL, 0,
+				  packet_buffer, sizeof(packet_buffer),
 				  PACKET_READ_GENTLE_ON_EOF |
 				  PACKET_READ_CHOMP_NEWLINE);
 		if (len < 0)
diff --git a/daemon.c b/daemon.c
index 3f70e79..9a241d9 100644
--- a/daemon.c
+++ b/daemon.c
@@ -612,7 +612,7 @@ static int execute(void)
 		loginfo("Connection from %s:%s", addr, port);
 
 	alarm(init_timeout ? init_timeout : timeout);
-	pktlen = packet_read(0, packet_buffer, sizeof(packet_buffer), 0);
+	pktlen = packet_read(0, NULL, 0, packet_buffer, sizeof(packet_buffer), 0);
 	alarm(0);
 
 	len = strlen(line);
diff --git a/pkt-line.c b/pkt-line.c
index 55fb688..2c47052 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -104,12 +104,29 @@ static int safe_read(int fd, void *buffer, unsigned size, int options)
 	strbuf_add(buf, buffer, n);
 }
 
-static int safe_read(int fd, void *buffer, unsigned size, int options)
+static int get_packet_data(int fd, char **src_buf, size_t *src_size,
+			   void *dst, unsigned size, int options)
 {
-	ssize_t ret = read_in_full(fd, buffer, size);
-	if (ret < 0)
-		die_errno("read error");
-	else if (ret < size) {
+	ssize_t ret;
+
+	if (fd >= 0 && src_buf && *src_buf)
+		die("BUG: multiple sources given to packet_read");
+
+	/* Read up to "size" bytes from our source, whatever it is. */
+	if (src_buf && *src_buf) {
+		ret = size < *src_size ? size : *src_size;
+		memcpy(dst, *src_buf, ret);
+		*src_buf += size;
+		*src_size -= size;
+	}
+	else {
+		ret = read_in_full(fd, dst, size);
+		if (ret < 0)
+			die_errno("read error");
+	}
+
+	/* And complain if we didn't get enough bytes to satisfy the read. */
+	if (ret < size) {
 		if (options & PACKET_READ_GENTLE_ON_EOF)
 			return -1;
 
@@ -144,12 +161,13 @@ int packet_read(int fd, char *buffer, unsigned size, int options)
 	return len;
 }
 
-int packet_read(int fd, char *buffer, unsigned size, int options)
+int packet_read(int fd, char **src_buf, size_t *src_len,
+		char *buffer, unsigned size, int options)
 {
 	int len, ret;
 	char linelen[4];
 
-	ret = safe_read(fd, linelen, 4, options);
+	ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
 	if (ret < 0)
 		return ret;
 	len = packet_length(linelen);
@@ -162,7 +180,7 @@ int packet_read(int fd, char *buffer, unsigned size, int options)
 	len -= 4;
 	if (len >= size)
 		die("protocol error: bad line length %d", len);
-	ret = safe_read(fd, buffer, len, options);
+	ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);
 	if (ret < 0)
 		return ret;
 
@@ -175,41 +193,24 @@ int packet_get_line(struct strbuf *out,
 	return len;
 }
 
-char *packet_read_line(int fd, int *len_p)
+static char *packet_read_line_generic(int fd,
+				      char **src, size_t *src_len,
+				      int *dst_len)
 {
-	int len = packet_read(fd, packet_buffer, sizeof(packet_buffer),
+	int len = packet_read(fd, src, src_len,
+			      packet_buffer, sizeof(packet_buffer),
 			      PACKET_READ_CHOMP_NEWLINE);
-	if (len_p)
-		*len_p = len;
+	if (dst_len)
+		*dst_len = len;
 	return len ? packet_buffer : NULL;
 }
 
-int packet_get_line(struct strbuf *out,
-	char **src_buf, size_t *src_len)
+char *packet_read_line(int fd, int *len_p)
 {
-	int len;
-
-	if (*src_len < 4)
-		return -1;
-	len = packet_length(*src_buf);
-	if (len < 0)
-		return -1;
-	if (!len) {
-		*src_buf += 4;
-		*src_len -= 4;
-		packet_trace("0000", 4, 0);
-		return 0;
-	}
-	if (*src_len < len)
-		return -2;
-
-	*src_buf += 4;
-	*src_len -= 4;
-	len -= 4;
+	return packet_read_line_generic(fd, NULL, 0, len_p);
+}
 
-	strbuf_add(out, *src_buf, len);
-	*src_buf += len;
-	*src_len -= len;
-	packet_trace(out->buf, out->len, 0);
-	return len;
+char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
+{
+	return packet_read_line_generic(-1, src, src_len, dst_len);
 }
diff --git a/pkt-line.h b/pkt-line.h
index fa93e32..47361f5 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -25,9 +25,16 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
 
 /*
- * Read a packetized line from the descriptor into the buffer, which must be at
- * least size bytes long. The return value specifies the number of bytes read
- * into the buffer.
+ * Read a packetized line into the buffer, which must be at least size bytes
+ * long. The return value specifies the number of bytes read into the buffer.
+ *
+ * If src_buffer is not NULL (and nor is *src_buffer), it should point to a
+ * buffer containing the packet data to parse, of at least *src_len bytes.
+ * After the function returns, src_buf will be increments and src_len
+ * decremented by the number of bytes consumed.
+ *
+ * If src_buffer (or *src_buffer) is NULL, then data is read from the
+ * descriptor "fd".
  *
  * If options does not contain PACKET_READ_GENTLE_ON_EOF, we will die under any
  * of the following conditions:
@@ -50,7 +57,8 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f
  */
 #define PACKET_READ_GENTLE_ON_EOF (1u<<0)
 #define PACKET_READ_CHOMP_NEWLINE (1u<<1)
-int packet_read(int fd, char *buffer, unsigned size, int options);
+int packet_read(int fd, char **src_buffer, size_t *src_len, char
+		*buffer, unsigned size, int options);
 
 /*
  * Convenience wrapper for packet_read that is not gentle, and sets the
@@ -61,11 +69,14 @@ extern char packet_buffer[LARGE_PACKET_MAX];
  */
 char *packet_read_line(int fd, int *size);
 
+/*
+ * Same as packet_read_line, but read from a buf rather than a descriptor;
+ * see packet_read for details on how src_* is used.
+ */
+char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
 
 #define DEFAULT_PACKET_MAX 1000
 #define LARGE_PACKET_MAX 65520
 extern char packet_buffer[LARGE_PACKET_MAX];
 
-int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
-
 #endif
diff --git a/remote-curl.c b/remote-curl.c
index b28f965..c0edd4c 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -138,28 +138,26 @@ static struct discovery* discover_refs(const char *service)
 	if (maybe_smart &&
 	    (5 <= last->len && last->buf[4] == '#') &&
 	    !strbuf_cmp(&exp, &type)) {
+		char *line;
+
 		/*
 		 * smart HTTP response; validate that the service
 		 * pkt-line matches our request.
 		 */
-		if (packet_get_line(&buffer, &last->buf, &last->len) <= 0)
-			die("%s has invalid packet header", refs_url);
-		if (buffer.len && buffer.buf[buffer.len - 1] == '\n')
-			strbuf_setlen(&buffer, buffer.len - 1);
+		line = packet_read_line_buf(&last->buf, &last->len, NULL);
 
 		strbuf_reset(&exp);
 		strbuf_addf(&exp, "# service=%s", service);
-		if (strbuf_cmp(&exp, &buffer))
-			die("invalid server response; got '%s'", buffer.buf);
+		if (strcmp(line, exp.buf))
+			die("invalid server response; got '%s'", line);
 		strbuf_release(&exp);
 
 		/* The header can include additional metadata lines, up
 		 * until a packet flush marker.  Ignore these now, but
 		 * in the future we might start to scan them.
 		 */
-		strbuf_reset(&buffer);
-		while (packet_get_line(&buffer, &last->buf, &last->len) > 0)
-			strbuf_reset(&buffer);
+		while (packet_read_line_buf(&last->buf, &last->len, NULL) > 0)
+			;
 
 		last->proto_git = 1;
 	}
@@ -308,7 +306,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,
 
 	if (!avail) {
 		rpc->initial_buffer = 0;
-		avail = packet_read(rpc->out, rpc->buf, rpc->alloc, 0);
+		avail = packet_read(rpc->out, NULL, 0, rpc->buf, rpc->alloc, 0);
 		if (!avail)
 			return 0;
 		rpc->pos = 0;
@@ -425,7 +423,7 @@ static int post_rpc(struct rpc_state *rpc)
 			break;
 		}
 
-		n = packet_read(rpc->out, buf, left, 0);
+		n = packet_read(rpc->out, 0, NULL, buf, left, 0);
 		if (!n)
 			break;
 		rpc->len += n;
@@ -579,7 +577,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
 	rpc->hdr_accept = strbuf_detach(&buf, NULL);
 
 	while (!err) {
-		int n = packet_read(rpc->out, rpc->buf, rpc->alloc, 0);
+		int n = packet_read(rpc->out, 0, NULL, rpc->buf, rpc->alloc, 0);
 		if (!n)
 			break;
 		rpc->pos = 0;
diff --git a/sideband.c b/sideband.c
index 15cc1ae..857954c 100644
--- a/sideband.c
+++ b/sideband.c
@@ -38,7 +38,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 
 	while (1) {
 		int band, len;
-		len = packet_read(in_stream, buf + pf, LARGE_PACKET_MAX, 0);
+		len = packet_read(in_stream, NULL, 0, buf + pf, LARGE_PACKET_MAX, 0);
 		if (len == 0)
 			break;
 		if (len < 1) {
-- 
1.8.2.rc0.9.g352092c

Re: [PATCH v3 15/19] pkt-line: share buffer/descriptor reading implementation

[Eric Sunshine]

On Wed, Feb 20, 2013 at 3:04 PM, Jeff King <peff@peff.net> wrote:
> diff --git a/pkt-line.h b/pkt-line.h
> index fa93e32..47361f5 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -25,9 +25,16 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f
>  void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
>
>  /*
> - * Read a packetized line from the descriptor into the buffer, which must be at
> - * least size bytes long. The return value specifies the number of bytes read
> - * into the buffer.
> + * Read a packetized line into the buffer, which must be at least size bytes
> + * long. The return value specifies the number of bytes read into the buffer.
> + *
> + * If src_buffer is not NULL (and nor is *src_buffer), it should point to a
> + * buffer containing the packet data to parse, of at least *src_len bytes.
> + * After the function returns, src_buf will be increments and src_len

s/increments/incremented/

> + * decremented by the number of bytes consumed.

[PATCH v3 16/19] teach get_remote_heads to read from a memory buffer

[Jeff King]

Now that we can read packet data from memory as easily as a
descriptor, get_remote_heads can take either one as a
source. This will allow further refactoring in remote-curl.

Signed-off-by: Jeff King <peff@peff.net>
---
Another "wrapper vs NULL argument" opportunity. I could go either way if
we feel strongly in one direction. A third option is:

  struct packet_source {
          /* Choose one. */
          int fd;
          char *buf;
          int len;
  };

but then each caller has to be bothered to define and fill in the
struct, which ends up even uglier.

 builtin/fetch-pack.c | 2 +-
 builtin/send-pack.c  | 2 +-
 cache.h              | 4 +++-
 connect.c            | 6 +++---
 remote-curl.c        | 2 +-
 transport.c          | 6 +++---
 6 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c21cc2c..03ed2ca 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -125,7 +125,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 				   args.verbose ? CONNECT_VERBOSE : 0);
 	}
 
-	get_remote_heads(fd[0], &ref, 0, NULL);
+	get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL);
 
 	ref = fetch_pack(&args, fd, conn, ref, dest,
 			 &sought, pack_lockfile_ptr);
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 8778519..152c4ea 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -207,7 +207,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 
 	memset(&extra_have, 0, sizeof(extra_have));
 
-	get_remote_heads(fd[0], &remote_refs, REF_NORMAL, &extra_have);
+	get_remote_heads(fd[0], NULL, 0, &remote_refs, REF_NORMAL, &extra_have);
 
 	transport_verify_remote_names(nr_refspecs, refspecs);
 
diff --git a/cache.h b/cache.h
index e493563..db646a2 100644
--- a/cache.h
+++ b/cache.h
@@ -1049,7 +1049,9 @@ struct extra_have_objects {
 	int nr, alloc;
 	unsigned char (*array)[20];
 };
-extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int flags, struct extra_have_objects *);
+extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+				     struct ref **list, unsigned int flags,
+				     struct extra_have_objects *);
 extern int server_supports(const char *feature);
 extern int parse_feature_request(const char *features, const char *feature);
 extern const char *server_feature_value(const char *feature, int *len_ret);
diff --git a/connect.c b/connect.c
index 061aa5b..f57efd0 100644
--- a/connect.c
+++ b/connect.c
@@ -62,8 +62,8 @@ static void die_initial_contact(int got_at_least_one_head)
 /*
  * Read all the refs from the other end
  */
-struct ref **get_remote_heads(int in, struct ref **list,
-			      unsigned int flags,
+struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+			      struct ref **list, unsigned int flags,
 			      struct extra_have_objects *extra_have)
 {
 	int got_at_least_one_head = 0;
@@ -76,7 +76,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
 		int len, name_len;
 		char *buffer = packet_buffer;
 
-		len = packet_read(in, NULL, 0,
+		len = packet_read(in, &src_buf, &src_len,
 				  packet_buffer, sizeof(packet_buffer),
 				  PACKET_READ_GENTLE_ON_EOF |
 				  PACKET_READ_CHOMP_NEWLINE);
diff --git a/remote-curl.c b/remote-curl.c
index c0edd4c..3bc6cb5 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -192,7 +192,7 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
 
 	if (start_async(&async))
 		die("cannot start thread to parse advertised refs");
-	get_remote_heads(async.out, &list,
+	get_remote_heads(async.out, NULL, 0, &list,
 			for_push ? REF_NORMAL : 0, NULL);
 	close(async.out);
 	if (finish_async(&async))
diff --git a/transport.c b/transport.c
index 886ffd8..62df466 100644
--- a/transport.c
+++ b/transport.c
@@ -507,7 +507,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus
 	struct ref *refs;
 
 	connect_setup(transport, for_push, 0);
-	get_remote_heads(data->fd[0], &refs,
+	get_remote_heads(data->fd[0], NULL, 0, &refs,
 			 for_push ? REF_NORMAL : 0, &data->extra_have);
 	data->got_remote_heads = 1;
 
@@ -541,7 +541,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 
 	if (!data->got_remote_heads) {
 		connect_setup(transport, 0, 0);
-		get_remote_heads(data->fd[0], &refs_tmp, 0, NULL);
+		get_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0, NULL);
 		data->got_remote_heads = 1;
 	}
 
@@ -799,7 +799,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 		struct ref *tmp_refs;
 		connect_setup(transport, 1, 0);
 
-		get_remote_heads(data->fd[0], &tmp_refs, REF_NORMAL, NULL);
+		get_remote_heads(data->fd[0], NULL, 0, &tmp_refs, REF_NORMAL, NULL);
 		data->got_remote_heads = 1;
 	}
 
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 17/19] remote-curl: pass buffer straight to get_remote_heads

[Jeff King]

Until recently, get_remote_heads only knew how to read refs
from a file descriptor. To hack around this, we spawned a
thread (or forked a process) to write the buffer back to us.

Now that we can just pass it our buffer directly, we don't
have to use this hack anymore.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote-curl.c | 26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 3bc6cb5..e07f654 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -170,33 +170,11 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
 	return last;
 }
 
-static int write_discovery(int in, int out, void *data)
-{
-	struct discovery *heads = data;
-	int err = 0;
-	if (write_in_full(out, heads->buf, heads->len) != heads->len)
-		err = 1;
-	close(out);
-	return err;
-}
-
 static struct ref *parse_git_refs(struct discovery *heads, int for_push)
 {
 	struct ref *list = NULL;
-	struct async async;
-
-	memset(&async, 0, sizeof(async));
-	async.proc = write_discovery;
-	async.data = heads;
-	async.out = -1;
-
-	if (start_async(&async))
-		die("cannot start thread to parse advertised refs");
-	get_remote_heads(async.out, NULL, 0, &list,
-			for_push ? REF_NORMAL : 0, NULL);
-	close(async.out);
-	if (finish_async(&async))
-		die("ref parsing thread failed");
+	get_remote_heads(-1, heads->buf, heads->len, &list,
+			 for_push ? REF_NORMAL : 0, NULL);
 	return list;
 }
 
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 18/19] remote-curl: move ref-parsing code up in file

[Jeff King]

The ref-parsing functions are static. Let's move them up in
the file to be available to more functions, which will help
us with later refactoring.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote-curl.c | 118 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 59 insertions(+), 59 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index e07f654..856decc 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -80,6 +80,65 @@ static struct discovery *last_discovery;
 };
 static struct discovery *last_discovery;
 
+static struct ref *parse_git_refs(struct discovery *heads, int for_push)
+{
+	struct ref *list = NULL;
+	get_remote_heads(-1, heads->buf, heads->len, &list,
+			 for_push ? REF_NORMAL : 0, NULL);
+	return list;
+}
+
+static struct ref *parse_info_refs(struct discovery *heads)
+{
+	char *data, *start, *mid;
+	char *ref_name;
+	int i = 0;
+
+	struct ref *refs = NULL;
+	struct ref *ref = NULL;
+	struct ref *last_ref = NULL;
+
+	data = heads->buf;
+	start = NULL;
+	mid = data;
+	while (i < heads->len) {
+		if (!start) {
+			start = &data[i];
+		}
+		if (data[i] == '\t')
+			mid = &data[i];
+		if (data[i] == '\n') {
+			if (mid - start != 40)
+				die("%sinfo/refs not valid: is this a git repository?", url);
+			data[i] = 0;
+			ref_name = mid + 1;
+			ref = xmalloc(sizeof(struct ref) +
+				      strlen(ref_name) + 1);
+			memset(ref, 0, sizeof(struct ref));
+			strcpy(ref->name, ref_name);
+			get_sha1_hex(start, ref->old_sha1);
+			if (!refs)
+				refs = ref;
+			if (last_ref)
+				last_ref->next = ref;
+			last_ref = ref;
+			start = NULL;
+		}
+		i++;
+	}
+
+	ref = alloc_ref("HEAD");
+	if (!http_fetch_ref(url, ref) &&
+	    !resolve_remote_symref(ref, refs)) {
+		ref->next = refs;
+		refs = ref;
+	} else {
+		free(ref);
+	}
+
+	return refs;
+}
+
 static void free_discovery(struct discovery *d)
 {
 	if (d) {
@@ -170,65 +229,6 @@ static struct discovery* discover_refs(const char *service)
 	return last;
 }
 
-static struct ref *parse_git_refs(struct discovery *heads, int for_push)
-{
-	struct ref *list = NULL;
-	get_remote_heads(-1, heads->buf, heads->len, &list,
-			 for_push ? REF_NORMAL : 0, NULL);
-	return list;
-}
-
-static struct ref *parse_info_refs(struct discovery *heads)
-{
-	char *data, *start, *mid;
-	char *ref_name;
-	int i = 0;
-
-	struct ref *refs = NULL;
-	struct ref *ref = NULL;
-	struct ref *last_ref = NULL;
-
-	data = heads->buf;
-	start = NULL;
-	mid = data;
-	while (i < heads->len) {
-		if (!start) {
-			start = &data[i];
-		}
-		if (data[i] == '\t')
-			mid = &data[i];
-		if (data[i] == '\n') {
-			if (mid - start != 40)
-				die("%sinfo/refs not valid: is this a git repository?", url);
-			data[i] = 0;
-			ref_name = mid + 1;
-			ref = xmalloc(sizeof(struct ref) +
-				      strlen(ref_name) + 1);
-			memset(ref, 0, sizeof(struct ref));
-			strcpy(ref->name, ref_name);
-			get_sha1_hex(start, ref->old_sha1);
-			if (!refs)
-				refs = ref;
-			if (last_ref)
-				last_ref->next = ref;
-			last_ref = ref;
-			start = NULL;
-		}
-		i++;
-	}
-
-	ref = alloc_ref("HEAD");
-	if (!http_fetch_ref(url, ref) &&
-	    !resolve_remote_symref(ref, refs)) {
-		ref->next = refs;
-		refs = ref;
-	} else {
-		free(ref);
-	}
-
-	return refs;
-}
-
 static struct ref *get_refs(int for_push)
 {
 	struct discovery *heads;
-- 
1.8.2.rc0.9.g352092c

[PATCH v3 19/19] remote-curl: always parse incoming refs

[Jeff King]

When remote-curl receives a list of refs from a server, it
keeps the whole buffer intact. When we get a "list" command,
we feed the result to get_remote_heads, and when we get a
"fetch" or "push" command, we feed it to fetch-pack or
send-pack, respectively.

If the HTTP response from the server is truncated for any
reason, we will get an incomplete ref advertisement. If we
then feed this incomplete list to fetch-pack, one of a few
things may happen:

  1. If the truncation is in a packet header, fetch-pack
     will notice the bogus line and complain.

  2. If the truncation is inside a packet, fetch-pack will
     keep waiting for us to send the rest of the packet,
     which we never will.

  3. If the truncation is at a packet boundary, fetch-pack
     will keep waiting for us to send the next packet, which
     we never will.

As a result, fetch-pack hangs, waiting for input.  However,
remote-curl believes it has sent all of the advertisement,
and therefore waits for fetch-pack to speak. The two
processes end up in a deadlock.

We do notice the broken ref list if we feed it to
get_remote_heads. So if git asks the helper to do a "list"
followed by a "fetch", we are safe; we'll abort during the
list operation, which parses the refs.

This patch teaches remote-curl to always parse and save the
incoming ref list when we read the ref advertisement from a
server. That means that we will always verify and abort
before even running fetch-pack (or send-pack) when reading a
corrupted list, even if we do not run the "list" command
explicitly.

Since we save the result, in the common case of running
"list" then "fetch", we do not do any extra parsing at all.
In the case of just a "fetch", we do an extra round of
parsing, but only once.

Note also that the "fetch" case will now also initialize
server_capabilities from the remote (in remote-curl; we
already would do so inside fetch-pack).  Doing "list+fetch"
already does this. It doesn't actually matter now, but the
new behavior is arguably more correct, should remote-curl
ever start caring about the server's capability list.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote-curl.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 856decc..3d2b194 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -76,6 +76,7 @@ struct discovery {
 	char *buf_alloc;
 	char *buf;
 	size_t len;
+	struct ref *refs;
 	unsigned proto_git : 1;
 };
 static struct discovery *last_discovery;
@@ -145,11 +146,12 @@ static void free_discovery(struct discovery *d)
 		if (d == last_discovery)
 			last_discovery = NULL;
 		free(d->buf_alloc);
+		free_refs(d->refs);
 		free(d);
 	}
 }
 
-static struct discovery* discover_refs(const char *service)
+static struct discovery* discover_refs(const char *service, int for_push)
 {
 	struct strbuf exp = STRBUF_INIT;
 	struct strbuf type = STRBUF_INIT;
@@ -221,6 +223,11 @@ static struct discovery* discover_refs(const char *service)
 		last->proto_git = 1;
 	}
 
+	if (last->proto_git)
+		last->refs = parse_git_refs(last, for_push);
+	else
+		last->refs = parse_info_refs(last);
+
 	free(refs_url);
 	strbuf_release(&exp);
 	strbuf_release(&type);
@@ -234,13 +241,11 @@ static struct ref *get_refs(int for_push)
 	struct discovery *heads;
 
 	if (for_push)
-		heads = discover_refs("git-receive-pack");
+		heads = discover_refs("git-receive-pack", for_push);
 	else
-		heads = discover_refs("git-upload-pack");
+		heads = discover_refs("git-upload-pack", for_push);
 
-	if (heads->proto_git)
-		return parse_git_refs(heads, for_push);
-	return parse_info_refs(heads);
+	return heads->refs;
 }
 
 static void output_refs(struct ref *refs)
@@ -254,7 +259,6 @@ static void output_refs(struct ref *refs)
 	}
 	printf("\n");
 	fflush(stdout);
-	free_refs(refs);
 }
 
 struct rpc_state {
@@ -670,7 +674,7 @@ static int fetch(int nr_heads, struct ref **to_fetch)
 
 static int fetch(int nr_heads, struct ref **to_fetch)
 {
-	struct discovery *d = discover_refs("git-upload-pack");
+	struct discovery *d = discover_refs("git-upload-pack", 0);
 	if (d->proto_git)
 		return fetch_git(d, nr_heads, to_fetch);
 	else
@@ -789,7 +793,7 @@ static int push(int nr_spec, char **specs)
 
 static int push(int nr_spec, char **specs)
 {
-	struct discovery *heads = discover_refs("git-receive-pack");
+	struct discovery *heads = discover_refs("git-receive-pack", 1);
 	int ret;
 
 	if (heads->proto_git)
-- 
1.8.2.rc0.9.g352092c