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

Re: [PATCH v3 07/14] remote.c: introduce branch_get_upstream helper

From
Jeff King <peff@peff.net>
Date
May 21, 2015, 18:14 UTC
Message-ID
<20150521181429.GA6684@peff.net>
In-Reply-To
<xmqqbnhdkdne.fsf@gitster.dls.corp.google.com>
On Thu, May 21, 2015 at 11:07:33AM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > All of the information needed to find the @{upstream} of a
> > branch is included in the branch struct, but callers have to
> > navigate a series of possible-NULL values to get there.
> > Let's wrap that logic up in an easy-to-read helper.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> 
> This step in the whole series is a gem.  I cannot believe that we
> were content having to repeat that "branch->merge[0]->dst if we can
> dereference down to that level" this many times.  Nice clean-up.

There is a related cleanup I resisted, which is that several call-sites will call stat_tracking_info, then later look directly at branch->merge[0]->dst without a check for NULL (fill_tracking_info is such a site).

This works because stat_tracking_info's return value tells us that we did indeed find an upstream to compare with. But it feels a little leaky to me. One solution is for stat_tracking_info to pass out the name of thte upstream, making the caller side something like:

diff --git a/builtin/branch.c b/builtin/branch.c
index cc55ff2..edc4deb 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -425,11 +425,12 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
 	int ours, theirs;
 	char *ref = NULL;
 	struct branch *branch = branch_get(branch_name);
+	const char *upstream;
 	struct strbuf fancy = STRBUF_INIT;
 	int upstream_is_gone = 0;
 	int added_decoration = 1;
 
-	switch (stat_tracking_info(branch, &ours, &theirs)) {
+	switch (stat_tracking_info(branch, &ours, &theirs, &upstream)) {
 	case 0:
 		/* no base */
 		return;
@@ -443,7 +444,7 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
 	}
 
 	if (show_upstream_ref) {
-		ref = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
+		ref = shorten_unambiguous_ref(upstream, 0);
 		if (want_color(branch_use_color))
 			strbuf_addf(&fancy, "%s%s%s",
 					branch_get_color(BRANCH_COLOR_UPSTREAM),


This is still a little error-prone, though. We assume "upstream" was
filled in depending on the return value of stat_tracking_info. I wonder
if we could get rid of the weird tri-state return value from
stat_tracking_info, and just have callers detect the "there is no base"
case by checking "upstream != NULL".

I dunno. It is not buggy in any of the current callers, so it might not
be worth spending too much time on.

-Peff
Previous: Junio C HamanoNext: Jeff King
Message 11 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.