Commit 0b3dfbf425d689101663341beb94237614f1b5c2

Patrick Steinhardt 2018-08-09T11:13:59

smart_pkt: reorder and rename parameters of `git_pkt_parse_line` The parameters of the `git_pkt_parse_line` function are quite confusing. First, there is no real indicator what the `out` parameter is actually all about, and it's not really clear what the `bufflen` parameter refers to. Reorder and rename the parameters to make this more obvious.

diff --git a/src/transports/smart.h b/src/transports/smart.h
index 8983dee..abf8028 100644
--- a/src/transports/smart.h
+++ b/src/transports/smart.h
@@ -188,7 +188,7 @@ int git_smart__get_push_stream(transport_smart *t, git_smart_subtransport_stream
 int git_smart__update_heads(transport_smart *t, git_vector *symrefs);
 
 /* smart_pkt.c */
-int git_pkt_parse_line(git_pkt **head, const char *line, const char **out, size_t len);
+int git_pkt_parse_line(git_pkt **head, const char **endptr, const char *line, size_t linelen);
 int git_pkt_buffer_flush(git_buf *buf);
 int git_pkt_send_flush(GIT_SOCKET s);
 int git_pkt_buffer_done(git_buf *buf);
diff --git a/src/transports/smart_pkt.c b/src/transports/smart_pkt.c
index a19b226..419a76e 100644
--- a/src/transports/smart_pkt.c
+++ b/src/transports/smart_pkt.c
@@ -407,13 +407,13 @@ static int32_t parse_len(const char *line)
  */
 
 int git_pkt_parse_line(
-	git_pkt **head, const char *line, const char **out, size_t bufflen)
+	git_pkt **pkt, const char **endptr, const char *line, size_t linelen)
 {
 	int ret;
 	int32_t len;
 
 	/* Not even enough for the length */
-	if (bufflen > 0 && bufflen < PKT_LEN_SIZE)
+	if (linelen > 0 && linelen < PKT_LEN_SIZE)
 		return GIT_EBUFS;
 
 	len = parse_len(line);
@@ -422,7 +422,7 @@ int git_pkt_parse_line(
 		 * If we fail to parse the length, it might be because the
 		 * server is trying to send us the packfile already.
 		 */
-		if (bufflen >= 4 && !git__prefixcmp(line, "PACK")) {
+		if (linelen >= 4 && !git__prefixcmp(line, "PACK")) {
 			giterr_set(GITERR_NET, "unexpected pack file");
 		} else {
 			giterr_set(GITERR_NET, "bad packet length");
@@ -435,7 +435,7 @@ int git_pkt_parse_line(
 	 * If we were given a buffer length, then make sure there is
 	 * enough in the buffer to satisfy this line
 	 */
-	if (bufflen > 0 && bufflen < (size_t)len)
+	if (linelen > 0 && linelen < (size_t)len)
 		return GIT_EBUFS;
 
 	/*
@@ -458,36 +458,36 @@ int git_pkt_parse_line(
 	}
 
 	if (len == 0) { /* Flush pkt */
-		*out = line;
-		return flush_pkt(head);
+		*endptr = line;
+		return flush_pkt(pkt);
 	}
 
 	len -= PKT_LEN_SIZE; /* the encoded length includes its own size */
 
 	if (*line == GIT_SIDE_BAND_DATA)
-		ret = data_pkt(head, line, len);
+		ret = data_pkt(pkt, line, len);
 	else if (*line == GIT_SIDE_BAND_PROGRESS)
-		ret = sideband_progress_pkt(head, line, len);
+		ret = sideband_progress_pkt(pkt, line, len);
 	else if (*line == GIT_SIDE_BAND_ERROR)
-		ret = sideband_error_pkt(head, line, len);
+		ret = sideband_error_pkt(pkt, line, len);
 	else if (!git__prefixncmp(line, len, "ACK"))
-		ret = ack_pkt(head, line, len);
+		ret = ack_pkt(pkt, line, len);
 	else if (!git__prefixncmp(line, len, "NAK"))
-		ret = nak_pkt(head);
+		ret = nak_pkt(pkt);
 	else if (!git__prefixncmp(line, len, "ERR"))
-		ret = err_pkt(head, line, len);
+		ret = err_pkt(pkt, line, len);
 	else if (*line == '#')
-		ret = comment_pkt(head, line, len);
+		ret = comment_pkt(pkt, line, len);
 	else if (!git__prefixncmp(line, len, "ok"))
-		ret = ok_pkt(head, line, len);
+		ret = ok_pkt(pkt, line, len);
 	else if (!git__prefixncmp(line, len, "ng"))
-		ret = ng_pkt(head, line, len);
+		ret = ng_pkt(pkt, line, len);
 	else if (!git__prefixncmp(line, len, "unpack"))
-		ret = unpack_pkt(head, line, len);
+		ret = unpack_pkt(pkt, line, len);
 	else
-		ret = ref_pkt(head, line, len);
+		ret = ref_pkt(pkt, line, len);
 
-	*out = line + len;
+	*endptr = line + len;
 
 	return ret;
 }
diff --git a/src/transports/smart_protocol.c b/src/transports/smart_protocol.c
index ce63cb1..10a5b4e 100644
--- a/src/transports/smart_protocol.c
+++ b/src/transports/smart_protocol.c
@@ -44,7 +44,7 @@ int git_smart__store_refs(transport_smart *t, int flushes)
 
 	do {
 		if (buf->offset > 0)
-			error = git_pkt_parse_line(&pkt, buf->data, &line_end, buf->offset);
+			error = git_pkt_parse_line(&pkt, &line_end, buf->data, buf->offset);
 		else
 			error = GIT_EBUFS;
 
@@ -217,7 +217,7 @@ static int recv_pkt(git_pkt **out_pkt, git_pkt_type *out_type, gitno_buffer *buf
 
 	do {
 		if (buf->offset > 0)
-			error = git_pkt_parse_line(&pkt, ptr, &line_end, buf->offset);
+			error = git_pkt_parse_line(&pkt, &line_end, ptr, buf->offset);
 		else
 			error = GIT_EBUFS;
 
@@ -755,7 +755,7 @@ static int add_push_report_sideband_pkt(git_push *push, git_pkt_data *data_pkt, 
 	}
 
 	while (line_len > 0) {
-		error = git_pkt_parse_line(&pkt, line, &line_end, line_len);
+		error = git_pkt_parse_line(&pkt, &line_end, line, line_len);
 
 		if (error == GIT_EBUFS) {
 			/* Buffer the data when the inner packet is split
@@ -798,8 +798,8 @@ static int parse_report(transport_smart *transport, git_push *push)
 
 	for (;;) {
 		if (buf->offset > 0)
-			error = git_pkt_parse_line(&pkt, buf->data,
-						   &line_end, buf->offset);
+			error = git_pkt_parse_line(&pkt, &line_end,
+						   buf->data, buf->offset);
 		else
 			error = GIT_EBUFS;
 
diff --git a/tests/transports/smart/packet.c b/tests/transports/smart/packet.c
index f887563..5b623a3 100644
--- a/tests/transports/smart/packet.c
+++ b/tests/transports/smart/packet.c
@@ -12,7 +12,7 @@ static void assert_flush_parses(const char *line)
 	const char *endptr;
 	git_pkt *pkt;
 
-	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, line, &endptr, linelen));
+	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, &endptr, line, linelen));
 	cl_assert_equal_i(pkt->type, GIT_PKT_FLUSH);
 	cl_assert_equal_strn(endptr, line + 4, linelen - 4);
 
@@ -25,7 +25,7 @@ static void assert_data_pkt_parses(const char *line, const char *expected_data, 
 	const char *endptr;
 	git_pkt_data *pkt;
 
-	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, line, &endptr, linelen));
+	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, &endptr, line, linelen));
 	cl_assert_equal_i(pkt->type, GIT_PKT_DATA);
 	cl_assert_equal_i(pkt->len, expected_len);
 	cl_assert_equal_strn(pkt->data, expected_data, expected_len);
@@ -39,7 +39,7 @@ static void assert_sideband_progress_parses(const char *line, const char *expect
 	const char *endptr;
 	git_pkt_progress *pkt;
 
-	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, line, &endptr, linelen));
+	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, &endptr, line, linelen));
 	cl_assert_equal_i(pkt->type, GIT_PKT_PROGRESS);
 	cl_assert_equal_i(pkt->len, expected_len);
 	cl_assert_equal_strn(pkt->data, expected_data, expected_len);
@@ -53,7 +53,7 @@ static void assert_error_parses(const char *line, const char *expected_error, si
 	const char *endptr;
 	git_pkt_err *pkt;
 
-	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, line, &endptr, linelen));
+	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, &endptr, line, linelen));
 	cl_assert_equal_i(pkt->type, GIT_PKT_ERR);
 	cl_assert_equal_i(pkt->len, expected_len);
 	cl_assert_equal_strn(pkt->error, expected_error, expected_len);
@@ -70,7 +70,7 @@ static void assert_ack_parses(const char *line, const char *expected_oid, enum g
 
 	cl_git_pass(git_oid_fromstr(&oid, expected_oid));
 
-	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, line, &endptr, linelen));
+	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, &endptr, line, linelen));
 	cl_assert_equal_i(pkt->type, GIT_PKT_ACK);
 	cl_assert_equal_oid(&pkt->oid, &oid);
 	cl_assert_equal_i(pkt->status, expected_status);
@@ -84,7 +84,7 @@ static void assert_nak_parses(const char *line)
 	const char *endptr;
 	git_pkt *pkt;
 
-	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, line, &endptr, linelen));
+	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, &endptr, line, linelen));
 	cl_assert_equal_i(pkt->type, GIT_PKT_NAK);
 	cl_assert_equal_strn(endptr, line + 7, linelen - 7);
 
@@ -97,7 +97,7 @@ static void assert_comment_parses(const char *line, const char *expected_comment
 	const char *endptr;
 	git_pkt_comment *pkt;
 
-	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, line, &endptr, linelen));
+	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, &endptr, line, linelen));
 	cl_assert_equal_i(pkt->type, GIT_PKT_COMMENT);
 	cl_assert_equal_strn(pkt->comment, expected_comment, strlen(expected_comment));
 
@@ -110,7 +110,7 @@ static void assert_ok_parses(const char *line, const char *expected_ref)
 	const char *endptr;
 	git_pkt_ok *pkt;
 
-	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, line, &endptr, linelen));
+	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, &endptr, line, linelen));
 	cl_assert_equal_i(pkt->type, GIT_PKT_OK);
 	cl_assert_equal_strn(pkt->ref, expected_ref, strlen(expected_ref));
 
@@ -123,7 +123,7 @@ static void assert_unpack_parses(const char *line, bool ok)
 	const char *endptr;
 	git_pkt_unpack *pkt;
 
-	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, line, &endptr, linelen));
+	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, &endptr, line, linelen));
 	cl_assert_equal_i(pkt->type, GIT_PKT_UNPACK);
 	cl_assert_equal_i(pkt->unpack_ok, ok);
 
@@ -136,7 +136,7 @@ static void assert_ng_parses(const char *line, const char *expected_ref, const c
 	const char *endptr;
 	git_pkt_ng *pkt;
 
-	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, line, &endptr, linelen));
+	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, &endptr, line, linelen));
 	cl_assert_equal_i(pkt->type, GIT_PKT_NG);
 	cl_assert_equal_strn(pkt->ref, expected_ref, strlen(expected_ref));
 	cl_assert_equal_strn(pkt->msg, expected_msg, strlen(expected_msg));
@@ -156,7 +156,7 @@ static void assert_ref_parses_(const char *line, size_t linelen, const char *exp
 
 	cl_git_pass(git_oid_fromstr(&oid, expected_oid));
 
-	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, line, &endptr, linelen));
+	cl_git_pass(git_pkt_parse_line((git_pkt **) &pkt, &endptr, line, linelen));
 	cl_assert_equal_i(pkt->type, GIT_PKT_REF);
 	cl_assert_equal_oid(&pkt->head.oid, &oid);
 	cl_assert_equal_strn(pkt->head.name, expected_ref, strlen(expected_ref));
@@ -172,7 +172,7 @@ static void assert_pkt_fails(const char *line)
 {
 	const char *endptr;
 	git_pkt *pkt;
-	cl_git_fail(git_pkt_parse_line(&pkt, line, &endptr, strlen(line) + 1));
+	cl_git_fail(git_pkt_parse_line(&pkt, &endptr, line, strlen(line) + 1));
 }
 
 void test_transports_smart_packet__parsing_garbage_fails(void)