[BUG] fetch output is ugly in 'next'

[BUG] fetch output is ugly in 'next'

[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

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

Re: [BUG] fetch output is ugly in 'next'

[Duy Nguyen]

On Fri, Oct 21, 2016 at 7:26 AM, Jeff King <peff@peff.net> wrote:
> 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.

My eyes!

> 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.
-- 
Duy

Re: [BUG] fetch output is ugly in 'next'

[Duy Nguyen]

On Fri, Oct 21, 2016 at 7:11 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> 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.
-- 
Duy

Re: [BUG] fetch output is ugly in 'next'

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Fri, Oct 21, 2016 at 7:11 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> 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(-)

diff --git a/transport.h b/transport.h
index 18d2cf8275..5339fabbad 100644
--- a/transport.h
+++ b/transport.h
@@ -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 */

Re: [BUG] fetch output is ugly in 'next'

[Jeff King]

On Fri, Oct 21, 2016 at 09:50:49AM -0700, Junio C Hamano wrote:

> Duy Nguyen <pclouds@gmail.com> writes:
> 
> > On Fri, Oct 21, 2016 at 7:11 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> >> 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(-)
> 
> diff --git a/transport.h b/transport.h
> index 18d2cf8275..5339fabbad 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -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

Re: [BUG] fetch output is ugly in 'next'

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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 ;-)

[PATCH 0/3] fetch output is ugly in 'next'

[Junio C Hamano]

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(-)

-- 
2.10.1-723-g2384e83bc3

[PATCH 1/3] transport: pass summary_width down the callchain

[Junio C Hamano]

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 <gitster@pobox.com>
---
 transport.c | 63 +++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 25 deletions(-)

diff --git a/transport.c b/transport.c
index 94d6dc3725..ec02b78924 100644
--- a/transport.c
+++ b/transport.c
@@ -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;
-- 
2.10.1-723-g2384e83bc3

[PATCH 2/3] fetch: pass summary_width down the callchain

[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 <gitster@pobox.com>
---
 builtin/fetch.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index a9f12cc5cf..40696e5338 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -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);
-- 
2.10.1-723-g2384e83bc3

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

[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 <gitster@pobox.com>
---
 builtin/fetch.c | 4 ++--
 transport.c     | 7 ++++++-
 transport.h     | 2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 40696e5338..09813cd826 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -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)");
diff --git a/transport.c b/transport.c
index ec02b78924..d4b8bf5f25 100644
--- a/transport.c
+++ b/transport.c
@@ -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);
 
diff --git a/transport.h b/transport.h
index e783377e40..706d99e818 100644
--- a/transport.h
+++ b/transport.h
@@ -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 *);
-- 
2.10.1-723-g2384e83bc3

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

[Jeff King]

On Fri, Oct 21, 2016 at 03:39:27PM -0700, Junio C Hamano wrote:

> 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:

diff --git a/transport.c b/transport.c
index 4dac713063..c1eaa4a 100644
--- a/transport.c
+++ b/transport.c
@@ -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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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 <gitster@pobox.com>
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 <gitster@pobox.com>
---
 transport.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/transport.c b/transport.c
index d4b8bf5f25..f1f95cf7c7 100644
--- a/transport.c
+++ b/transport.c
@@ -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,
-- 
2.10.1-723-g2384e83bc3

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

[Jeff King]

On Fri, Oct 21, 2016 at 09:39:45PM -0700, Junio C Hamano wrote:

> And this is the final one.
> 
> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> 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.

>  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);
> +	}

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Oct 21, 2016 at 09:39:45PM -0700, Junio C Hamano wrote:
>
>> And this is the final one.
>> 
>> -- >8 --
>> From: Junio C Hamano <gitster@pobox.com>
>> 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.

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

> 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.