fetch output is ugly in 'next'

BUG In review Jeff King 3 participants 14 messages Oct 21, 2016 – Oct 22, 2016

Share & tools

Jeff King

author

2016-10-21 00:27 UTC #0 lore raw

I recently started using lt/abbrev-auto via 'next'. This is the fetch output I saw today:

$ git fetch
remote: Counting objects: 332, done.
remote: Compressing objects: 100% (237/237), done.
remote: Total 332 (delta 182), reused 64 (delta 64), pack-reused 31
Receiving objects: 100% (332/332), 351.53 KiB | 0 bytes/s, done.
Resolving deltas: 100% (184/184), completed with 26 local objects.
From git://github.com/gitster/git
 + fb65135d9c...16ab66ec97 jch                                  -> origin/jch  (forced update)
   fd47ae6a5b..98985c6911 jk/diff-submodule-diff-inline        -> origin/jk/diff-submodule-diff-inline
 * [new branch]      jk/no-looking-at-dotgit-outside-repo -> origin/jk/no-looking-at-dotgit-outside-repo
 + 3a8f853f16...1369f9b5ba jt/trailer-with-cruft                -> origin/jt/trailer-with-cruft  (forced update)
 * [new branch]      pt/gitgui-updates                    -> origin/pt/gitgui-updates
 + 7594b34cbb...be8e40093b pu                                   -> origin/pu  (forced update)
 + 7b95cf9a4e...76e368c378 tg/add-chmod+x-fix                   -> origin/tg/add-chmod+x-fix  (forced update)
 + c4cf1f93cf...221912514c va/i18n-perl-scripts                 -> origin/va/i18n-perl-scripts  (forced update)
 * [new branch]      yk/git-tag-remove-mention-of-old-layout-in-doc -> origin/yk/git-tag-remove-mention-of-old-layout-in-doc

Yuck. I could maybe get over the wasted space due to the longer sha1s, but we clearly need to fix the alignment computation. I haven't dug on it at all; it's probably low-hanging fruit if somebody is interested.

-Peff

Duy Nguyen

2016-10-21 12:12 UTC #1 lore raw

Quoted 20 lines · Jeff King

I recently started using lt/abbrev-auto via 'next'. This is the fetch output I saw today:

$ git fetch
remote: Counting objects: 332, done.
remote: Compressing objects: 100% (237/237), done.
remote: Total 332 (delta 182), reused 64 (delta 64), pack-reused 31
Receiving objects: 100% (332/332), 351.53 KiB | 0 bytes/s, done.
Resolving deltas: 100% (184/184), completed with 26 local objects.
From git://github.com/gitster/git
 + fb65135d9c...16ab66ec97 jch                                  -> origin/jch  (forced update)
   fd47ae6a5b..98985c6911 jk/diff-submodule-diff-inline        -> origin/jk/diff-submodule-diff-inline
 * [new branch]      jk/no-looking-at-dotgit-outside-repo -> origin/jk/no-looking-at-dotgit-outside-repo
 + 3a8f853f16...1369f9b5ba jt/trailer-with-cruft                -> origin/jt/trailer-with-cruft  (forced update)
 * [new branch]      pt/gitgui-updates                    -> origin/pt/gitgui-updates
 + 7594b34cbb...be8e40093b pu                                   -> origin/pu  (forced update)
 + 7b95cf9a4e...76e368c378 tg/add-chmod+x-fix                   -> origin/tg/add-chmod+x-fix  (forced update)
 + c4cf1f93cf...221912514c va/i18n-perl-scripts                 -> origin/va/i18n-perl-scripts  (forced update)
 * [new branch]      yk/git-tag-remove-mention-of-old-layout-in-doc -> origin/yk/git-tag-remove-mention-of-old-layout-in-doc

Quoted 1 line

Yuck.

My eyes!

Quoted 3 lines

I could maybe get over the wasted space due to the longer sha1s, but we clearly need to fix the alignment computation. I haven't dug on it at all; it's probably low-hanging fruit if somebody is interested.

Yeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something sensible should do it.

Signature · 1 line

Duy

Duy Nguyen

2016-10-21 13:08 UTC #2 lore raw

Quoted 2 lines · Duy Nguyen

Yeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something sensible should do it.

Correction (if somebody will pick this up), it's TRANSPORT_SUMMARY_WIDTH that needs to be adjusted, not those four.

Signature · 1 line

Duy

Junio C Hamano

2016-10-21 16:50 UTC #3 lore raw

Quoted 6 lines · Duy Nguyen

Quoted 2 lines · Duy Nguyen
Yeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something sensible should do it.
Correction (if somebody will pick this up), it's TRANSPORT_SUMMARY_WIDTH that needs to be adjusted, not those four.

Yes, it used to be and it still is (2 * DEFAULT_ABBREV + 3) but in the new world order where default-abbrev is often -1 the expression does not make much sense.

Perhaps something along this line?

 transport.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

transport.h +1 -1

index 18d2cf8275..5339fabbad 100644
@@ -127,7 +127,7 @@ struct transport {
 #define TRANSPORT_PUSH_CERT 2048
 #define TRANSPORT_PUSH_ATOMIC 4096
 
-#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
+#define TRANSPORT_SUMMARY_WIDTH (2 * (DEFAULT_ABBREV < 0 ? 7 : DEFAULT_ABBREV) + 3)
 #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
 
 /* Returns a transport suitable for the url */

Jeff King

author

2016-10-21 21:42 UTC #4 lore raw

Quoted 23 lines · Junio C Hamano

Quoted 6 lines · Duy Nguyen
Quoted 2 lines · Duy Nguyen
Yeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something sensible should do it.
Correction (if somebody will pick this up), it's TRANSPORT_SUMMARY_WIDTH that needs to be adjusted, not those four.
Yes, it used to be and it still is (2 * DEFAULT_ABBREV + 3) but in the new world order where default-abbrev is often -1 the expression does not make much sense.
Perhaps something along this line?
 transport.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
transport.h +0 -0
index 18d2cf8275..5339fabbad 100644

transport.h struct transport { · line 131

Quoted 6 lines

transport.h +1 -1

@@ -127,7 +127,7 @@ struct transport {
 #define TRANSPORT_PUSH_CERT 2048
 #define TRANSPORT_PUSH_ATOMIC 4096
 
-#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
+#define TRANSPORT_SUMMARY_WIDTH (2 * (DEFAULT_ABBREV < 0 ? 7 : DEFAULT_ABBREV) + 3)
 #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)

That doesn't apply on 'next', because we have already done the equivalent there. :)

The right way to spell "7" is FALLBACK_DEFAULT_ABBREV, which is handled by your 65acfeacaa.

The remaining issue is that the static abbreviation is not nearly long enough for git.git anymore; the auto-abbrev feature bumps my repo to a minimum of 10 characters (it may only be 9 on a fresh clone; I have a couple remotes and active work in progress). So this isn't exactly a regression; it has always been the case that we may mis-align when the abbreviations ended up longer than the minimum. It's just that it didn't happen all that often in most repos (but it probably did constantly in linux.git).

The simplest band-aid fix would be to compute TRANSPORT_SUMMARY_WIDTH on the fly, taking into account the minimum found by actually counting the objects. That at least gets us back to where we were, with it mostly working and occasionally ugly when there's an oddball collision (for git.git anyway; it probably makes the kernel output much nicer).

The "right" fix is to queue up the list of ref updates to print, find the abbreviations for each, and then print them all in one shot, knowing ahead of time the size necessary to align them. This could also let us improve the name-alignment.

-Peff

Junio C Hamano

2016-10-21 22:14 UTC #5 lore raw

Quoted 4 lines · Jeff King

The "right" fix is to queue up the list of ref updates to print, find the abbreviations for each, and then print them all in one shot, knowing ahead of time the size necessary to align them. This could also let us improve the name-alignment.

Heh, that was what I was starting to code while listening to Jonathan and Stefan talk in a meeting ;-)

Junio C Hamano

2016-10-21 22:39 UTC #6 lore raw

cover fetch output is ugly in 'next'

It turns out that there are three codepaths, all of which have a set of refs that they want to show them using TRANSPORT_SUMMARY_WIDTH constant. The two preparatory steps turn the constant used at the leaf level into a parameter that is passed down through these callchains, and the last step introduces a helper function that can be used to compute the appropriate width to be fed to these callchains.

Junio C Hamano (3):
transport: pass summary_width down the callchain
fetch: pass summary_width down the callchain
transport: allow summary-width to be computed dynamically

 builtin/fetch.c | 37 +++++++++++++++++--------------
 transport.c     | 68 ++++++++++++++++++++++++++++++++++++---------------------
 transport.h     |  2 +-
 3 files changed, 65 insertions(+), 42 deletions(-)

Signature · 1 line

2.10.1-723-g2384e83bc3

Junio C Hamano

2016-10-21 22:39 UTC #7 lore raw

PATCH 1/3 transport: pass summary_width down the callchain

The callchain that originates at transport_print_push_status() eventually hits a single leaf function, print_ref_status(), that is used to show from what old object to what new object a ref got updated, and the width of the part that shows old and new object names used a constant TRANSPORT_SUMMARY_WIDTH.

Teach the callchain to pass the width down from the top instead. This allows a future enhancement to compute the necessary display width before calling down this callchain.

Signed-off-by Junio C Hamano <[email protected]>

---
 transport.c | 63 +++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 25 deletions(-)

transport.c +38 ~~-25~~

index 94d6dc3725..ec02b78924 100644
@@ -295,7 +295,9 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
 	}
 }
 
-static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg, int porcelain)
+static void print_ref_status(char flag, const char *summary,
+			     struct ref *to, struct ref *from, const char *msg,
+			     int porcelain, int summary_width)
 {
 	if (porcelain) {
 		if (from)

@@ -307,7 +309,7 @@ static void print_ref_status(char flag, const char *summary, struct ref *to, str
 		else
 			fprintf(stdout, "%s\n", summary);
 	} else {
-		fprintf(stderr, " %c %-*s ", flag, TRANSPORT_SUMMARY_WIDTH, summary);
+		fprintf(stderr, " %c %-*s ", flag, summary_width, summary);
 		if (from)
 			fprintf(stderr, "%s -> %s", prettify_refname(from->name), prettify_refname(to->name));
 		else

@@ -321,15 +323,16 @@ static void print_ref_status(char flag, const char *summary, struct ref *to, str
 	}
 }
 
-static void print_ok_ref_status(struct ref *ref, int porcelain)
+static void print_ok_ref_status(struct ref *ref, int porcelain, int summary_width)
 {
 	if (ref->deletion)
-		print_ref_status('-', "[deleted]", ref, NULL, NULL, porcelain);
+		print_ref_status('-', "[deleted]", ref, NULL, NULL,
+				 porcelain, summary_width);
 	else if (is_null_oid(&ref->old_oid))
 		print_ref_status('*',
 			(starts_with(ref->name, "refs/tags/") ? "[new tag]" :
 			"[new branch]"),
-			ref, ref->peer_ref, NULL, porcelain);
+			ref, ref->peer_ref, NULL, porcelain, summary_width);
 	else {
 		struct strbuf quickref = STRBUF_INIT;
 		char type;

@@ -349,12 +352,14 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)
 		strbuf_add_unique_abbrev(&quickref, ref->new_oid.hash,
 					 DEFAULT_ABBREV);
 
-		print_ref_status(type, quickref.buf, ref, ref->peer_ref, msg, porcelain);
+		print_ref_status(type, quickref.buf, ref, ref->peer_ref, msg,
+				 porcelain, summary_width);
 		strbuf_release(&quickref);
 	}
 }
 
-static int print_one_push_status(struct ref *ref, const char *dest, int count, int porcelain)
+static int print_one_push_status(struct ref *ref, const char *dest, int count,
+				 int porcelain, int summary_width)
 {
 	if (!count) {
 		char *url = transport_anonymize_url(dest);

@@ -364,56 +369,60 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 
 	switch(ref->status) {
 	case REF_STATUS_NONE:
-		print_ref_status('X', "[no match]", ref, NULL, NULL, porcelain);
+		print_ref_status('X', "[no match]", ref, NULL, NULL,
+				 porcelain, summary_width);
 		break;
 	case REF_STATUS_REJECT_NODELETE:
 		print_ref_status('!', "[rejected]", ref, NULL,
-						 "remote does not support deleting refs", porcelain);
+				 "remote does not support deleting refs",
+				 porcelain, summary_width);
 		break;
 	case REF_STATUS_UPTODATE:
 		print_ref_status('=', "[up to date]", ref,
-						 ref->peer_ref, NULL, porcelain);
+				 ref->peer_ref, NULL, porcelain, summary_width);
 		break;
 	case REF_STATUS_REJECT_NONFASTFORWARD:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
-						 "non-fast-forward", porcelain);
+				 "non-fast-forward", porcelain, summary_width);
 		break;
 	case REF_STATUS_REJECT_ALREADY_EXISTS:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
-						 "already exists", porcelain);
+				 "already exists", porcelain, summary_width);
 		break;
 	case REF_STATUS_REJECT_FETCH_FIRST:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
-						 "fetch first", porcelain);
+				 "fetch first", porcelain, summary_width);
 		break;
 	case REF_STATUS_REJECT_NEEDS_FORCE:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
-						 "needs force", porcelain);
+				 "needs force", porcelain, summary_width);
 		break;
 	case REF_STATUS_REJECT_STALE:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
-						 "stale info", porcelain);
+				 "stale info", porcelain, summary_width);
 		break;
 	case REF_STATUS_REJECT_SHALLOW:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
-						 "new shallow roots not allowed", porcelain);
+				 "new shallow roots not allowed",
+				 porcelain, summary_width);
 		break;
 	case REF_STATUS_REMOTE_REJECT:
 		print_ref_status('!', "[remote rejected]", ref,
-						 ref->deletion ? NULL : ref->peer_ref,
-						 ref->remote_status, porcelain);
+				 ref->deletion ? NULL : ref->peer_ref,
+				 ref->remote_status, porcelain, summary_width);
 		break;
 	case REF_STATUS_EXPECTING_REPORT:
 		print_ref_status('!', "[remote failure]", ref,
-						 ref->deletion ? NULL : ref->peer_ref,
-						 "remote failed to report status", porcelain);
+				 ref->deletion ? NULL : ref->peer_ref,
+				 "remote failed to report status",
+				 porcelain, summary_width);
 		break;
 	case REF_STATUS_ATOMIC_PUSH_FAILED:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
-						 "atomic push failed", porcelain);
+				 "atomic push failed", porcelain, summary_width);
 		break;
 	case REF_STATUS_OK:
-		print_ok_ref_status(ref, porcelain);
+		print_ok_ref_status(ref, porcelain, summary_width);
 		break;
 	}

@@ -427,25 +436,29 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 	int n = 0;
 	unsigned char head_sha1[20];
 	char *head;
+	int summary_width = TRANSPORT_SUMMARY_WIDTH;
 
 	head = resolve_refdup("HEAD", RESOLVE_REF_READING, head_sha1, NULL);
 
 	if (verbose) {
 		for (ref = refs; ref; ref = ref->next)
 			if (ref->status == REF_STATUS_UPTODATE)
-				n += print_one_push_status(ref, dest, n, porcelain);
+				n += print_one_push_status(ref, dest, n,
+							   porcelain, summary_width);
 	}
 
 	for (ref = refs; ref; ref = ref->next)
 		if (ref->status == REF_STATUS_OK)
-			n += print_one_push_status(ref, dest, n, porcelain);
+			n += print_one_push_status(ref, dest, n,
+						   porcelain, summary_width);
 
 	*reject_reasons = 0;
 	for (ref = refs; ref; ref = ref->next) {
 		if (ref->status != REF_STATUS_NONE &&
 		    ref->status != REF_STATUS_UPTODATE &&
 		    ref->status != REF_STATUS_OK)
-			n += print_one_push_status(ref, dest, n, porcelain);
+			n += print_one_push_status(ref, dest, n,
+						   porcelain, summary_width);
 		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) {
 			if (head != NULL && !strcmp(head, ref->name))
 				*reject_reasons |= REJECT_NON_FF_HEAD;

Signature · 1 line

2.10.1-723-g2384e83bc3

Junio C Hamano

2016-10-21 22:39 UTC #8 lore raw

PATCH 2/3 fetch: pass summary_width down the callchain

In reply to Junio C Hamano

The leaf function on the "fetch" side that uses TRANSPORT_SUMMARY_WIDTH constant is builtin/fetch.c::format_display() and it has two distinct callchains. The one that reports the primary result of fetch originates at store_updated_refs(); the other one that reports the pruning of the remote-tracking refs originates at prune_refs().

Teach these two places to pass summary_width down the callchain, just like we did for the "push" side in the previous commit.

Signed-off-by Junio C Hamano <[email protected]>

---
 builtin/fetch.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

builtin/fetch.c +21 ~~-16~~

index a9f12cc5cf..40696e5338 100644
@@ -17,9 +17,6 @@
 #include "argv-array.h"
 #include "utf8.h"
 
-#define TRANSPORT_SUMMARY(x) \
-	(int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
-
 static const char * const builtin_fetch_usage[] = {
 	N_("git fetch [<options>] [<repository> [<refspec>...]]"),
 	N_("git fetch [<options>] <group>"),

@@ -569,9 +566,12 @@ static void print_compact(struct strbuf *display,
 
 static void format_display(struct strbuf *display, char code,
 			   const char *summary, const char *error,
-			   const char *remote, const char *local)
+			   const char *remote, const char *local,
+			   int summary_width)
 {
-	strbuf_addf(display, "%c %-*s ", code, TRANSPORT_SUMMARY(summary));
+	int width = (summary_width + strlen(summary) - gettext_width(summary));
+
+	strbuf_addf(display, "%c %-*s ", code, width, summary);
 	if (!compact_format)
 		print_remote_to_local(display, remote, local);
 	else

@@ -583,7 +583,8 @@ static void format_display(struct strbuf *display, char code,
 static int update_local_ref(struct ref *ref,
 			    const char *remote,
 			    const struct ref *remote_ref,
-			    struct strbuf *display)
+			    struct strbuf *display,
+			    int summary_width)
 {
 	struct commit *current = NULL, *updated;
 	enum object_type type;

@@ -597,7 +598,7 @@ static int update_local_ref(struct ref *ref,
 	if (!oidcmp(&ref->old_oid, &ref->new_oid)) {
 		if (verbosity > 0)
 			format_display(display, '=', _("[up to date]"), NULL,
-				       remote, pretty_ref);
+				       remote, pretty_ref, summary_width);
 		return 0;
 	}

@@ -611,7 +612,7 @@ static int update_local_ref(struct ref *ref,
 		 */
 		format_display(display, '!', _("[rejected]"),
 			       _("can't fetch in current branch"),
-			       remote, pretty_ref);
+			       remote, pretty_ref, summary_width);
 		return 1;
 	}

@@ -621,7 +622,7 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref("updating tag", ref, 0);
 		format_display(display, r ? '!' : 't', _("[tag update]"),
 			       r ? _("unable to update local ref") : NULL,
-			       remote, pretty_ref);
+			       remote, pretty_ref, summary_width);
 		return r;
 	}

@@ -654,7 +655,7 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref(msg, ref, 0);
 		format_display(display, r ? '!' : '*', what,
 			       r ? _("unable to update local ref") : NULL,
-			       remote, pretty_ref);
+			       remote, pretty_ref, summary_width);
 		return r;
 	}

@@ -670,7 +671,7 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref("fast-forward", ref, 1);
 		format_display(display, r ? '!' : ' ', quickref.buf,
 			       r ? _("unable to update local ref") : NULL,
-			       remote, pretty_ref);
+			       remote, pretty_ref, summary_width);
 		strbuf_release(&quickref);
 		return r;
 	} else if (force || ref->force) {

@@ -685,12 +686,12 @@ static int update_local_ref(struct ref *ref,
 		r = s_update_ref("forced-update", ref, 1);
 		format_display(display, r ? '!' : '+', quickref.buf,
 			       r ? _("unable to update local ref") : _("forced update"),
-			       remote, pretty_ref);
+			       remote, pretty_ref, summary_width);
 		strbuf_release(&quickref);
 		return r;
 	} else {
 		format_display(display, '!', _("[rejected]"), _("non-fast-forward"),
-			       remote, pretty_ref);
+			       remote, pretty_ref, summary_width);
 		return 1;
 	}
 }

@@ -721,6 +722,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 	char *url;
 	const char *filename = dry_run ? "/dev/null" : git_path_fetch_head();
 	int want_status;
+	int summary_width = TRANSPORT_SUMMARY_WIDTH;
 
 	fp = fopen(filename, "a");
 	if (!fp)

@@ -830,13 +832,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 
 			strbuf_reset(&note);
 			if (ref) {
-				rc |= update_local_ref(ref, what, rm, &note);
+				rc |= update_local_ref(ref, what, rm, &note,
+						       summary_width);
 				free(ref);
 			} else
 				format_display(&note, '*',
 					       *kind ? kind : "branch", NULL,
 					       *what ? what : "HEAD",
-					       "FETCH_HEAD");
+					       "FETCH_HEAD", summary_width);
 			if (note.len) {
 				if (verbosity >= 0 && !shown_url) {
 					fprintf(stderr, _("From %.*s\n"),

@@ -903,6 +906,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,
 	int url_len, i, result = 0;
 	struct ref *ref, *stale_refs = get_stale_heads(refs, ref_count, ref_map);
 	char *url;
+	int summary_width = TRANSPORT_SUMMARY_WIDTH;
 	const char *dangling_msg = dry_run
 		? _("   (%s will become dangling)")
 		: _("   (%s has become dangling)");

@@ -938,7 +942,8 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,
 				shown_url = 1;
 			}
 			format_display(&sb, '-', _("[deleted]"), NULL,
-				       _("(none)"), prettify_refname(ref->name));
+				       _("(none)"), prettify_refname(ref->name),
+				       summary_width);
 			fprintf(stderr, " %s\n",sb.buf);
 			strbuf_release(&sb);
 			warn_dangling_symref(stderr, dangling_msg, ref->name);

Signature · 1 line

2.10.1-723-g2384e83bc3

Junio C Hamano

2016-10-21 22:39 UTC #9 lore raw

PATCH 3/3 transport: allow summary-width to be computed dynamically

In reply to Junio C Hamano

Now we have identified three callchains that have a set of refs that they want to show their <old, new> object names in an aligned output, we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH with a helper function call to transport_summary_width() that takes the set of ref as a parameter. This step does not yet iterate over the refs and compute, which is left as an exercise to the readers.

Signed-off-by Junio C Hamano <[email protected]>

---
 builtin/fetch.c | 4 ++--
 transport.c     | 7 ++++++-
 transport.h     | 2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

builtin/fetch.c +2 -2

index 40696e5338..09813cd826 100644
@@ -722,7 +722,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 	char *url;
 	const char *filename = dry_run ? "/dev/null" : git_path_fetch_head();
 	int want_status;
-	int summary_width = TRANSPORT_SUMMARY_WIDTH;
+	int summary_width = transport_summary_width(ref_map);
 
 	fp = fopen(filename, "a");
 	if (!fp)

@@ -906,7 +906,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,
 	int url_len, i, result = 0;
 	struct ref *ref, *stale_refs = get_stale_heads(refs, ref_count, ref_map);
 	char *url;
-	int summary_width = TRANSPORT_SUMMARY_WIDTH;
+	int summary_width = transport_summary_width(stale_refs);
 	const char *dangling_msg = dry_run
 		? _("   (%s will become dangling)")
 		: _("   (%s has become dangling)");

transport.c +6 -1

index ec02b78924..d4b8bf5f25 100644
@@ -429,6 +429,11 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count,
 	return 1;
 }
 
+int transport_summary_width(const struct ref *refs)
+{
+	return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+}
+
 void transport_print_push_status(const char *dest, struct ref *refs,
 				  int verbose, int porcelain, unsigned int *reject_reasons)
 {

@@ -436,7 +441,7 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 	int n = 0;
 	unsigned char head_sha1[20];
 	char *head;
-	int summary_width = TRANSPORT_SUMMARY_WIDTH;
+	int summary_width = transport_summary_width(refs);
 
 	head = resolve_refdup("HEAD", RESOLVE_REF_READING, head_sha1, NULL);

transport.h +1 -1

index e783377e40..706d99e818 100644
@@ -142,7 +142,7 @@ struct transport {
 #define TRANSPORT_PUSH_ATOMIC 8192
 #define TRANSPORT_PUSH_OPTIONS 16384
 
-#define TRANSPORT_SUMMARY_WIDTH (2 * FALLBACK_DEFAULT_ABBREV + 3)
+extern int transport_summary_width(const struct ref *refs);
 
 /* Returns a transport suitable for the url */
 struct transport *transport_get(struct remote *, const char *);

Signature · 1 line

2.10.1-723-g2384e83bc3

Jeff King

author

2016-10-22 04:10 UTC #10 lore raw

Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically

Quoted 6 lines · Junio C Hamano

Now we have identified three callchains that have a set of refs that they want to show their <old, new> object names in an aligned output, we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH with a helper function call to transport_summary_width() that takes the set of ref as a parameter. This step does not yet iterate over the refs and compute, which is left as an exercise to the readers.

The final step could be something like this:

transport.c +15 -1

index 4dac713063..c1eaa4a 100644
@@ -443,7 +443,21 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count,
 
 int transport_summary_width(const struct ref *refs)
 {
-	return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+	int max_abbrev;
+
+	/*
+	 * Computing the complete set of abbreviated sha1s is expensive just to
+	 * find their lengths, but we can at least find our real dynamic
+	 * minimum by picking an arbitrary sha1.
+	 */
+	if (refs)
+		max_abbrev = strlen(find_unique_abbrev(refs->old_oid.hash,
+						       DEFAULT_ABBREV));
+	else
+		max_abbrev = FALLBACK_DEFAULT_ABBREV;
+
+	/* 2 abbreviated sha1s, plus "..." in between */
+	return (2 * max_abbrev + 3);
 }
 
 void transport_print_push_status(const char *dest, struct ref *refs,

which produces reasonable results for me. But if we really wanted the true value, I think we'd want to compute and store the abbreviated sha1s, and then the refactoring done by your series probably isn't the right direction.

I think we'd instead want to replace "struct strbuf *display" passed down to update_local_ref() with something more like:

  struct ref_status_table {
	struct ref_status_item {
		char old_hash[GIT_SHA1_HEX + 1];
		char new_hash[GIT_SHA1_HEX + 1];
		char *remote_ref;
		char *local_ref;
		char *summary;
		char code;
	} *items;
	size_t alloc, nr;

};

and then format_display() would just add to the list (including calling find_unique_abbrev()), and then at the end we'd call a function to show them all.

That would also get rid of prepare_format_display(), as we could easily walk over the prepared list before printing any of them (as opposed to what that function does now, which is to walk over the ref map; that requires that it know which refs are going to be printed).

-Peff

Junio C Hamano

2016-10-22 04:39 UTC #11 lore raw

Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically

In reply to Junio C Hamano

Quoted 6 lines · Junio C Hamano

Now we have identified three callchains that have a set of refs that they want to show their <old, new> object names in an aligned output, we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH with a helper function call to transport_summary_width() that takes the set of ref as a parameter. This step does not yet iterate over the refs and compute, which is left as an exercise to the readers.

And this is the final one.

-- >8 --
From: Junio C Hamano <[email protected]>
Date: Fri, 21 Oct 2016 21:33:06 -0700
Subject: [PATCH] transport: compute summary-width dynamically

Now all that is left to do is to actually iterate over the refs and measure the display width needed to show their abbreviation.

Signed-off-by Junio C Hamano <[email protected]>

---
 transport.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

transport.c +17 -1

index d4b8bf5f25..f1f95cf7c7 100644
@@ -429,9 +429,25 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count,
 	return 1;
 }
 
+static int measure_abbrev(const struct object_id *oid, int sofar)
+{
+	char hex[GIT_SHA1_HEXSZ + 1];
+	int w = find_unique_abbrev_r(hex, oid->hash, DEFAULT_ABBREV);
+
+	return (w < sofar) ? sofar : w;
+}
+
 int transport_summary_width(const struct ref *refs)
 {
-	return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+	int maxw;
+
+	for (maxw = -1; refs; refs = refs->next) {
+		maxw = measure_abbrev(&refs->old_oid, maxw);
+		maxw = measure_abbrev(&refs->new_oid, maxw);
+	}
+	if (maxw < 0)
+		maxw = FALLBACK_DEFAULT_ABBREV;
+	return (2 * maxw + 3);
 }
 
 void transport_print_push_status(const char *dest, struct ref *refs,

Signature · 1 line

2.10.1-723-g2384e83bc3

Jeff King

author

2016-10-22 05:04 UTC #12 lore raw

Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically

Quoted 9 lines · Junio C Hamano

And this is the final one.
-- >8 --
From: Junio C Hamano <[email protected]>
Date: Fri, 21 Oct 2016 21:33:06 -0700
Subject: [PATCH] transport: compute summary-width dynamically
Now all that is left to do is to actually iterate over the refs and measure the display width needed to show their abbreviation.

I think we crossed emails. :) This is obviously correct, if we don't mind paying the find_unique_abbrev cost twice for each sha1.

transport.c static int print_one_push_status(struct ref *ref, const char *dest, int count, · line 447

Quoted 9 lines

int transport_summary_width(const struct ref *refs) {

transport.c +6 -1

-	return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+	int maxw;
+
+	for (maxw = -1; refs; refs = refs->next) {
+		maxw = measure_abbrev(&refs->old_oid, maxw);
+		maxw = measure_abbrev(&refs->new_oid, maxw);
+	}

This is a minor style nit, but I think it's better to avoid mixing unrelated bits between the initialization, condition, and iteration bits of a for loop. IOW:

  int maxw = -1;

  for (; refs; refs = refs->next) {
	...

}

makes the for-loop _just_ about iteration, and it is more clear that the manipulation of "maxw" is a side effect of the loop that is valid after it finishes.

Though in this particular case, the loop and the whole function are short enough that I don't care that much. Just raising a philosophical point. :)

-Peff

Junio C Hamano

2016-10-22 16:25 UTC #13 lore raw

Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically

Quoted 9 lines · Jeff King

Quoted 7 lines · Junio C Hamano
And this is the final one.
-- >8 --
From: Junio C Hamano <[email protected]>
Date: Fri, 21 Oct 2016 21:33:06 -0700
Subject: [PATCH] transport: compute summary-width dynamically

Quoted 5 lines

Quoted 2 lines
Now all that is left to do is to actually iterate over the refs and measure the display width needed to show their abbreviation.
I think we crossed emails. :) This is obviously correct, if we don't mind paying the find_unique_abbrev cost twice for each sha1.

Indeed we did. I do not think the cost matters that much in the codepath to produce the final summary output.

Quoted 3 lines

This is a minor style nit, but I think it's better to avoid mixing unrelated bits between the initialization, condition, and iteration bits of a for loop.

Yeah, you're right.