[PATCH 1/2] push: release strbufs used for refspec formatting

["René Scharfe" <l.s.r@web.de>]

map_refspec() either returns the passed in ref string or a detached
strbuf.  This makes it hard for callers to release the possibly
allocated memory, and set_refspecs() consequently leaks it.

Let map_refspec() append any refspecs directly and release its own
strbufs after use.  Rename it to refspec_append_mapped() and don't
return anything to reflect its increased responsibility.

set_refspecs() also leaks its strbufs.  Do the same here and directly
call refspec_append() in each if branch instead of holding onto a
detached strbuf, then dispose of the allocated memory after use.  We
need to add an else branch for the final call because all the other
conditional branches already add their formatted refspec now.

setup_push_upstream() and setup_push_current() forgot to release their
strbufs as well; plug these leaks, too, while at it.

None of these leaks were likely to impact users, because the number
and sizes of refspecs are usually small and the allocations are only
done once per program run.  Clean them up nevertheless, as another
step on the long road towards zero memory leaks.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
Patch generated with --function-context for easier review.

 builtin/push.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index bc94078e72..0f3c108c93 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -61,76 +61,84 @@ static struct refspec rs = REFSPEC_INIT_PUSH;

 static struct string_list push_options_config = STRING_LIST_INIT_DUP;

-static const char *map_refspec(const char *ref,
-			       struct remote *remote, struct ref *local_refs)
+static void refspec_append_mapped(struct refspec *refspec, const char *ref,
+				  struct remote *remote, struct ref *local_refs)
 {
 	const char *branch_name;
 	struct ref *matched = NULL;

 	/* Does "ref" uniquely name our ref? */
-	if (count_refspec_match(ref, local_refs, &matched) != 1)
-		return ref;
+	if (count_refspec_match(ref, local_refs, &matched) != 1) {
+		refspec_append(refspec, ref);
+		return;
+	}

 	if (remote->push.nr) {
 		struct refspec_item query;
 		memset(&query, 0, sizeof(struct refspec_item));
 		query.src = matched->name;
 		if (!query_refspecs(&remote->push, &query) && query.dst) {
 			struct strbuf buf = STRBUF_INIT;
 			strbuf_addf(&buf, "%s%s:%s",
 				    query.force ? "+" : "",
 				    query.src, query.dst);
-			return strbuf_detach(&buf, NULL);
+			refspec_append(refspec, buf.buf);
+			strbuf_release(&buf);
+			return;
 		}
 	}

 	if (push_default == PUSH_DEFAULT_UPSTREAM &&
 	    skip_prefix(matched->name, "refs/heads/", &branch_name)) {
 		struct branch *branch = branch_get(branch_name);
 		if (branch->merge_nr == 1 && branch->merge[0]->src) {
 			struct strbuf buf = STRBUF_INIT;
 			strbuf_addf(&buf, "%s:%s",
 				    ref, branch->merge[0]->src);
-			return strbuf_detach(&buf, NULL);
+			refspec_append(refspec, buf.buf);
+			strbuf_release(&buf);
+			return;
 		}
 	}

-	return ref;
+	refspec_append(refspec, ref);
 }

 static void set_refspecs(const char **refs, int nr, const char *repo)
 {
 	struct remote *remote = NULL;
 	struct ref *local_refs = NULL;
 	int i;

 	for (i = 0; i < nr; i++) {
 		const char *ref = refs[i];
 		if (!strcmp("tag", ref)) {
 			struct strbuf tagref = STRBUF_INIT;
 			if (nr <= ++i)
 				die(_("tag shorthand without <tag>"));
 			ref = refs[i];
 			if (deleterefs)
 				strbuf_addf(&tagref, ":refs/tags/%s", ref);
 			else
 				strbuf_addf(&tagref, "refs/tags/%s", ref);
-			ref = strbuf_detach(&tagref, NULL);
+			refspec_append(&rs, tagref.buf);
+			strbuf_release(&tagref);
 		} else if (deleterefs) {
 			struct strbuf delref = STRBUF_INIT;
 			if (strchr(ref, ':'))
 				die(_("--delete only accepts plain target ref names"));
 			strbuf_addf(&delref, ":%s", ref);
-			ref = strbuf_detach(&delref, NULL);
+			refspec_append(&rs, delref.buf);
+			strbuf_release(&delref);
 		} else if (!strchr(ref, ':')) {
 			if (!remote) {
 				/* lazily grab remote and local_refs */
 				remote = remote_get(repo);
 				local_refs = get_local_heads();
 			}
-			ref = map_refspec(ref, remote, local_refs);
-		}
-		refspec_append(&rs, ref);
+			refspec_append_mapped(&rs, ref, remote, local_refs);
+		} else
+			refspec_append(&rs, ref);
 	}
 }

@@ -192,45 +200,47 @@ static const char message_detached_head_die[] =
 static void setup_push_upstream(struct remote *remote, struct branch *branch,
 				int triangular, int simple)
 {
 	struct strbuf refspec = STRBUF_INIT;

 	if (!branch)
 		die(_(message_detached_head_die), remote->name);
 	if (!branch->merge_nr || !branch->merge || !branch->remote_name)
 		die(_("The current branch %s has no upstream branch.\n"
 		    "To push the current branch and set the remote as upstream, use\n"
 		    "\n"
 		    "    git push --set-upstream %s %s\n"),
 		    branch->name,
 		    remote->name,
 		    branch->name);
 	if (branch->merge_nr != 1)
 		die(_("The current branch %s has multiple upstream branches, "
 		    "refusing to push."), branch->name);
 	if (triangular)
 		die(_("You are pushing to remote '%s', which is not the upstream of\n"
 		      "your current branch '%s', without telling me what to push\n"
 		      "to update which remote branch."),
 		    remote->name, branch->name);

 	if (simple) {
 		/* Additional safety */
 		if (strcmp(branch->refname, branch->merge[0]->src))
 			die_push_simple(branch, remote);
 	}

 	strbuf_addf(&refspec, "%s:%s", branch->refname, branch->merge[0]->src);
 	refspec_append(&rs, refspec.buf);
+	strbuf_release(&refspec);
 }

 static void setup_push_current(struct remote *remote, struct branch *branch)
 {
 	struct strbuf refspec = STRBUF_INIT;

 	if (!branch)
 		die(_(message_detached_head_die), remote->name);
 	strbuf_addf(&refspec, "%s:%s", branch->refname, branch->refname);
 	refspec_append(&rs, refspec.buf);
+	strbuf_release(&refspec);
 }

 static int is_workflow_triangular(struct remote *remote)
--
2.28.0