[PATCH 00/27] nd/shallow-deepen updates

["Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>]

This contains cleanups after Eric's comments (all good points,
thanks!). Changed patches have Junio's s-o-b line removed, so it's
easy to see which is changed and which is not. 09/27 is a new one,
split out of 10/27.

Interdiff

diff --git a/builtin/clone.c b/builtin/clone.c
index 5ccf6b7..3849231 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -53,13 +53,6 @@ static struct string_list option_config;
 static struct string_list option_reference;
 static int option_dissociate;
 
-static int option_parse_deepen_not(const struct option *opt,
-				   const char *arg, int unset)
-{
-	string_list_append(&option_not, arg);
-	return 0;
-}
-
 static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
 	OPT_BOOL(0, "progress", &option_progress,
@@ -97,9 +90,8 @@ static struct option builtin_clone_options[] = {
 		    N_("create a shallow clone of that depth")),
 	OPT_STRING(0, "shallow-since", &option_since, N_("time"),
 		    N_("create a shallow clone since a specific time")),
-	{ OPTION_CALLBACK, 0, "shallow-exclude", NULL, N_("revision"),
-		    N_("deepen history of shallow clone by excluding rev"),
-		    PARSE_OPT_NONEG, option_parse_deepen_not },
+	OPT_STRING_LIST(0, "shallow-exclude", &option_not, N_("revision"),
+			N_("deepen history of shallow clone by excluding rev")),
 	OPT_BOOL(0, "single-branch", &option_single_branch,
 		    N_("clone only one branch, HEAD or --branch")),
 	OPT_STRING(0, "separate-git-dir", &real_git_dir, N_("gitdir"),
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 68b44ba..7b0ea1c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -51,13 +51,6 @@ static int shown_url = 0;
 static int refmap_alloc, refmap_nr;
 static const char **refmap_array;
 
-static int option_parse_deepen_not(const struct option *opt,
-				   const char *arg, int unset)
-{
-	string_list_append(&deepen_not, arg);
-	return 0;
-}
-
 static int option_parse_recurse_submodules(const struct option *opt,
 				   const char *arg, int unset)
 {
@@ -126,9 +119,8 @@ static struct option builtin_fetch_options[] = {
 		   N_("deepen history of shallow clone")),
 	OPT_STRING(0, "shallow-since", &deepen_since, N_("time"),
 		   N_("deepen history of shallow repository based on time")),
-	{ OPTION_CALLBACK, 0, "shallow-exclude", NULL, N_("revision"),
-		    N_("deepen history of shallow clone by excluding rev"),
-		    PARSE_OPT_NONEG, option_parse_deepen_not },
+	OPT_STRING_LIST(0, "shallow-exclude", &deepen_not, N_("revision"),
+			N_("deepen history of shallow clone by excluding rev")),
 	OPT_INTEGER(0, "deepen", &deepen_relative,
 		    N_("deepen history of shallow clone")),
 	{ OPTION_SET_INT, 0, "unshallow", &unshallow, NULL,
diff --git a/remote-curl.c b/remote-curl.c
index 3f1a8f5..d56412d 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -745,7 +745,6 @@ static int fetch_git(struct discovery *heads,
 {
 	struct rpc_state rpc;
 	struct strbuf preamble = STRBUF_INIT;
-	char *depth_arg = NULL;
 	int i, err;
 	struct argv_array args = ARGV_ARRAY_INIT;
 
@@ -755,10 +754,8 @@ static int fetch_git(struct discovery *heads,
 		argv_array_push(&args, "--include-tag");
 	if (options.thin)
 		argv_array_push(&args, "--thin");
-	if (options.verbosity >= 3) {
-		argv_array_push(&args, "-v");
-		argv_array_push(&args, "-v");
-	}
+	if (options.verbosity >= 3)
+		argv_array_pushl(&args, "-v", "-v", NULL);
 	if (options.check_self_contained_and_connected)
 		argv_array_push(&args, "--check-self-contained-and-connected");
 	if (options.cloning)
@@ -798,7 +795,6 @@ static int fetch_git(struct discovery *heads,
 		write_or_die(1, rpc.result.buf, rpc.result.len);
 	strbuf_release(&rpc.result);
 	strbuf_release(&preamble);
-	free(depth_arg);
 	argv_array_clear(&args);
 	return err;
 }
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index f512098..a908036 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -678,8 +678,7 @@ test_expect_success 'shallow clone exclude tag two' '
 test_expect_success 'fetch exclude tag one' '
 	git -C shallow12 fetch --shallow-exclude one origin &&
 	git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
-	echo three >expected &&
-	echo two  >>expected &&
+	test_write_lines three two >expected &&
 	test_cmp expected actual
 '
 
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 25f8968..5fbf67c 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -116,8 +116,7 @@ test_expect_success 'shallow clone exclude tag two' '
 test_expect_success 'fetch exclude tag one' '
 	git -C shallow12 fetch --shallow-exclude one origin &&
 	git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
-	echo three >expected &&
-	echo two  >>expected &&
+	test_write_lines three two >expected &&
 	test_cmp expected actual
 '
 
diff --git a/transport-helper.c b/transport-helper.c
index b894b60..a5cdd77 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -272,9 +272,9 @@ static int strbuf_set_helper_option(struct helper_data *data,
 
 	if (!strcmp(buf->buf, "ok"))
 		ret = 0;
-	else if (starts_with(buf->buf, "error")) {
+	else if (starts_with(buf->buf, "error"))
 		ret = -1;
-	} else if (!strcmp(buf->buf, "unsupported"))
+	else if (!strcmp(buf->buf, "unsupported"))
 		ret = 1;
 	else {
 		warning("%s unexpectedly said: '%s'", data->name, buf->buf);
diff --git a/upload-pack.c b/upload-pack.c
index 18b914a..ef693bd 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -471,7 +471,7 @@ static int do_reachable_revlist(struct child_process *cmd,
 	cmd->out = -1;
 
 	if (start_command(cmd))
-		return -1;
+		goto error;
 
 	/*
 	 * If rev-list --stdin encounters an unknown commit, it
@@ -491,8 +491,10 @@ static int do_reachable_revlist(struct child_process *cmd,
 		if (!is_our_ref(o))
 			continue;
 		memcpy(namebuf + 1, oid_to_hex(&o->oid), GIT_SHA1_HEXSZ);
-		if (write_in_full(cmd->in, namebuf, 42) < 0)
-			return -1;
+		if (write_in_full(cmd->in, namebuf, 42) < 0) {
+			sigchain_pop(SIGPIPE);
+			goto error;
+		}
 	}
 	namebuf[40] = '\n';
 	for (i = 0; i < src->nr; i++) {
@@ -505,13 +507,23 @@ static int do_reachable_revlist(struct child_process *cmd,
 		if (reachable && o->type == OBJ_COMMIT)
 			o->flags |= TMP_MARK;
 		memcpy(namebuf, oid_to_hex(&o->oid), GIT_SHA1_HEXSZ);
-		if (write_in_full(cmd->in, namebuf, 41) < 0)
-			return -1;
+		if (write_in_full(cmd->in, namebuf, 41) < 0) {
+			sigchain_pop(SIGPIPE);
+			goto error;
+		}
 	}
 	close(cmd->in);
+	cmd->in = -1;
 
 	sigchain_pop(SIGPIPE);
 	return 0;
+
+error:
+	if (cmd->in >= 0)
+		close(cmd->in);
+	if (cmd->out >= 0)
+		close(cmd->out);
+	return -1;
 }
 
 static int get_reachable_list(struct object_array *src,
@@ -555,10 +567,10 @@ static int get_reachable_list(struct object_array *src,
 static int check_unreachable(struct object_array *src)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
-	int i, ret = do_reachable_revlist(&cmd, src, NULL);
 	char buf[1];
+	int i;
 
-	if (ret < 0)
+	if (do_reachable_revlist(&cmd, src, NULL) < 0)
 		return 0;
 
 	/*
@@ -567,8 +579,9 @@ static int check_unreachable(struct object_array *src)
 	 */
 	i = read_in_full(cmd.out, buf, 1);
 	if (i)
-		return 0;
+		goto error;
 	close(cmd.out);
+	cmd.out = -1;
 
 	/*
 	 * rev-list may have died by encountering a bad commit
@@ -576,10 +589,17 @@ static int check_unreachable(struct object_array *src)
 	 * even when it showed no commit.
 	 */
 	if (finish_command(&cmd))
-		return 0;
+		goto error;
 
 	/* All the non-tip ones are ancestors of what we advertised */
 	return 1;
+
+error:
+	if (cmd.in >= 0)
+		close(cmd.in);
+	if (cmd.out >= 0)
+		close(cmd.out);
+	return 0;
 }
 
 static void check_non_tip(void)
@@ -592,10 +612,12 @@ static void check_non_tip(void)
 	 * non-tip requests can never happen.
 	 */
 	if (!stateless_rpc && !(allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1))
-		;		/* error */
-	else if (check_unreachable(&want_obj))
+		goto error;
+	if (check_unreachable(&want_obj))
+		/* All the non-tip ones are ancestors of what we advertised */
 		return;
 
+error:
 	/* Pick one of them (we know there at least is one) */
 	for (i = 0; i < want_obj.nr; i++) {
 		struct object *o = want_obj.objects[i].item;
@@ -993,7 +1015,7 @@ int main(int argc, char **argv)
 	check_replace_refs = 0;
 
 	for (i = 1; i < argc; i++) {
-		char *arg = argv[i];
+		const char *arg = argv[i];
 
 		if (arg[0] != '-')
 			break;
@@ -1009,8 +1031,8 @@ int main(int argc, char **argv)
 			strict = 1;
 			continue;
 		}
-		if (starts_with(arg, "--timeout=")) {
-			timeout = atoi(arg+10);
+		if (skip_prefix(arg, "--timeout=", &arg)) {
+			timeout = atoi(arg);
 			daemon_mode = 1;
 			continue;
 		}
-- 
2.8.2.524.g6ff3d78