git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 02/14] remote.c: refactor setup of branch->merge list

From
Jeff King <peff@peff.net>
Date
May 21, 2015, 04:45 UTC
Message-ID
<20150521044509.GB23409@peff.net>
In-Reply-To
<20150521044429.GA5857@peff.net>

When we call branch_get() to lookup or create a "struct branch", we make sure the "merge" field is filled in so that callers can access it. But the conditions under which we do so are a little confusing, and can lead to two funny situations:

  1. If there's no branch.*.remote config, we cannot provide
     branch->merge (because it is really just an application
     of branch.*.merge to our remote's refspecs). But
     branch->merge_nr may be non-zero, leading callers to be
     believe they can access branch->merge (e.g., in
     branch_merge_matches and elsewhere).
     It doesn't look like this can cause a segfault in
     practice, as most code paths dealing with merge config
     will bail early if there is no remote defined. But it's
     a bit of a dangerous construct.
     We can fix this by setting merge_nr to "0" explicitly
     when we realize that we have no merge config. Note that
     merge_nr also counts the "merge_name" fields (which we
     _do_ have; that's how merge_nr got incremented), so we
     will "lose" access to them, in the sense that we forget
     how many we had. But no callers actually care; we use
     merge_name only while iteratively reading the config,
     and then convert it to the final "merge" form the first
     time somebody calls branch_get().
  2. We set up the "merge" field every time branch_get is
     called, even if it has already been done. This leaks
     memory.
     It's not a big deal in practice, since most code paths
     will access only one branch, or perhaps each branch
     only one time. But if you want to be pathological, you
     can leak arbitrary memory with:
       yes @{upstream} | head -1000 | git rev-list --stdin
     We can fix this by skipping setup when branch->merge is
     already non-NULL.

In addition to those two fixes, this patch pushes the "do we need to setup merge?" logic down into set_merge, where it is a bit easier to follow.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/remote.c b/remote.c
index bec8b31..ac17e66 100644
--- a/remote.c
+++ b/remote.c
@@ -1636,6 +1636,19 @@ static void set_merge(struct branch *ret)
 	unsigned char sha1[20];
 	int i;
 
+	if (!ret)
+		return; /* no branch */
+	if (ret->merge)
+		return; /* already run */
+	if (!ret->remote_name || !ret->merge_nr) {
+		/*
+		 * no merge config; let's make sure we don't confuse callers
+		 * with a non-zero merge_nr but a NULL merge
+		 */
+		ret->merge_nr = 0;
+		return;
+	}
+
 	ret->merge = xcalloc(ret->merge_nr, sizeof(*ret->merge));
 	for (i = 0; i < ret->merge_nr; i++) {
 		ret->merge[i] = xcalloc(1, sizeof(**ret->merge));
@@ -1660,11 +1673,9 @@ struct branch *branch_get(const char *name)
 		ret = current_branch;
 	else
 		ret = make_branch(name, 0);
-	if (ret && ret->remote_name) {
+	if (ret && ret->remote_name)
 		ret->remote = remote_get(ret->remote_name);
-		if (ret->merge_nr)
-			set_merge(ret);
-	}
+	set_merge(ret);
 	return ret;
 }
 
-- 
2.4.1.528.g00591e3
Previous: Jeff KingNext: Junio C Hamano
Message 3 of 27 in “implement @{push} shorthand”
  1. 0/14 implement @{push} shorthandJeff King, May 21, 2015
  2. 01/14 remote.c: drop default_remote_name variableJeff King, May 21, 2015
  3. 02/14 remote.c: refactor setup of branch->merge listJeff King, May 21, 2015
  4. Junio C HamanoMay 21, 2015
  5. 03/14 remote.c: drop "remote" pointer from "struct branch"Jeff King, May 21, 2015
  6. 04/14 remote.c: hoist branch.*.remote lookup out of remote_get_1Jeff King, May 21, 2015
  7. 05/14 remote.c: provide per-branch pushremote nameJeff King, May 21, 2015
  8. 06/14 remote.c: hoist read_config into remote_get_1Jeff King, May 21, 2015
  9. 07/14 remote.c: introduce branch_get_upstream helperJeff King, May 21, 2015
  10. Junio C HamanoMay 21, 2015
  11. Jeff KingMay 21, 2015
  12. Jeff KingMay 21, 2015
  13. Junio C HamanoMay 21, 2015
  14. 08/14 remote.c: report specific errors from branch_get_upstreamJeff King, May 21, 2015
  15. Junio C HamanoMay 21, 2015
  16. Jeff KingMay 21, 2015
  17. Junio C HamanoMay 21, 2015
  18. Jeff KingMay 22, 2015
  19. Jeff KingMay 22, 2015
  20. 09/14 remote.c: add branch_get_pushJeff King, May 21, 2015
  21. 10/14 sha1_name: refactor upstream_markJeff King, May 21, 2015
  22. 11/14 sha1_name: refactor interpret_upstream_markJeff King, May 21, 2015
  23. 12/14 sha1_name: implement @{push} shorthandJeff King, May 21, 2015
  24. 13/14 for-each-ref: use skip_prefix instead of starts_withJeff King, May 21, 2015
  25. 14/14 for-each-ref: accept "%(push)" formatJeff King, May 21, 2015
  26. Jeff KingMay 21, 2015
  27. Junio C HamanoMay 21, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.