{"thread":{"id":"44348","subject":"[BUG] fetch output is ugly in 'next'","startedAt":"2016-10-21T00:27:01Z","lastAt":"2016-10-22T16:25:35Z","messageCount":14,"participants":["Jeff King","Duy Nguyen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"304552","messageId":"20161021002654.qo6kcya4gocp73rs@sigill.intra.peff.net","threadId":"44348","inReplyTo":null,"subject":"[BUG] fetch output is ugly in 'next'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-21T00:26:54Z","receivedAt":"2016-10-21T00:27:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I recently started using lt/abbrev-auto via 'next'. This is the fetch\noutput I saw today:\n\n$ git fetch\nremote: Counting objects: 332, done.\nremote: Compressing objects: 100% (237/237), done.\nremote: Total 332 (delta 182), reused 64 (delta 64), pack-reused 31\nReceiving objects: 100% (332/332), 351.53 KiB | 0 bytes/s, done.\nResolving deltas: 100% (184/184), completed with 26 local objects.\nFrom git://github.com/gitster/git\n + fb65135d9c...16ab66ec97 jch                                  -> origin/jch  (forced update)\n   fd47ae6a5b..98985c6911 jk/diff-submodule-diff-inline        -> origin/jk/diff-submodule-diff-inline\n * [new branch]      jk/no-looking-at-dotgit-outside-repo -> origin/jk/no-looking-at-dotgit-outside-repo\n + 3a8f853f16...1369f9b5ba jt/trailer-with-cruft                -> origin/jt/trailer-with-cruft  (forced update)\n * [new branch]      pt/gitgui-updates                    -> origin/pt/gitgui-updates\n + 7594b34cbb...be8e40093b pu                                   -> origin/pu  (forced update)\n + 7b95cf9a4e...76e368c378 tg/add-chmod+x-fix                   -> origin/tg/add-chmod+x-fix  (forced update)\n + c4cf1f93cf...221912514c va/i18n-perl-scripts                 -> origin/va/i18n-perl-scripts  (forced update)\n * [new branch]      yk/git-tag-remove-mention-of-old-layout-in-doc -> origin/yk/git-tag-remove-mention-of-old-layout-in-doc\n\nYuck. I could maybe get over the wasted space due to the longer sha1s,\nbut we clearly need to fix the alignment computation. I haven't dug on\nit at all; it's probably low-hanging fruit if somebody is interested.\n\n-Peff\n"},{"id":"304575","messageId":"CACsJy8Bxn1qV+xXNnCpuOQ7qWNsz3oLt5-VG=VXZvV9dvJVvTQ@mail.gmail.com","threadId":"44348","inReplyTo":"20161021002654.qo6kcya4gocp73rs@sigill.intra.peff.net","subject":"Re: [BUG] fetch output is ugly in 'next'","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-21T12:11:26Z","receivedAt":"2016-10-21T12:12:02Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Oct 21, 2016 at 7:26 AM, Jeff King <peff@peff.net> wrote:\n> I recently started using lt/abbrev-auto via 'next'. This is the fetch\n> output I saw today:\n>\n> $ git fetch\n> remote: Counting objects: 332, done.\n> remote: Compressing objects: 100% (237/237), done.\n> remote: Total 332 (delta 182), reused 64 (delta 64), pack-reused 31\n> Receiving objects: 100% (332/332), 351.53 KiB | 0 bytes/s, done.\n> Resolving deltas: 100% (184/184), completed with 26 local objects.\n> From git://github.com/gitster/git\n>  + fb65135d9c...16ab66ec97 jch                                  -> origin/jch  (forced update)\n>    fd47ae6a5b..98985c6911 jk/diff-submodule-diff-inline        -> origin/jk/diff-submodule-diff-inline\n>  * [new branch]      jk/no-looking-at-dotgit-outside-repo -> origin/jk/no-looking-at-dotgit-outside-repo\n>  + 3a8f853f16...1369f9b5ba jt/trailer-with-cruft                -> origin/jt/trailer-with-cruft  (forced update)\n>  * [new branch]      pt/gitgui-updates                    -> origin/pt/gitgui-updates\n>  + 7594b34cbb...be8e40093b pu                                   -> origin/pu  (forced update)\n>  + 7b95cf9a4e...76e368c378 tg/add-chmod+x-fix                   -> origin/tg/add-chmod+x-fix  (forced update)\n>  + c4cf1f93cf...221912514c va/i18n-perl-scripts                 -> origin/va/i18n-perl-scripts  (forced update)\n>  * [new branch]      yk/git-tag-remove-mention-of-old-layout-in-doc -> origin/yk/git-tag-remove-mention-of-old-layout-in-doc\n>\n> Yuck.\n\nMy eyes!\n\n> I could maybe get over the wasted space due to the longer sha1s,\n> but we clearly need to fix the alignment computation. I haven't dug on\n> it at all; it's probably low-hanging fruit if somebody is interested.\n\nYeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something\nsensible should do it.\n-- \nDuy\n"},{"id":"304606","messageId":"CACsJy8CkK8K0ty3fv9qyD7wwB+81VPB-P1UUCbDJTJ4iM0Y+Cw@mail.gmail.com","threadId":"44348","inReplyTo":"CACsJy8Bxn1qV+xXNnCpuOQ7qWNsz3oLt5-VG=VXZvV9dvJVvTQ@mail.gmail.com","subject":"Re: [BUG] fetch output is ugly in 'next'","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-21T13:07:22Z","receivedAt":"2016-10-21T13:08:07Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Oct 21, 2016 at 7:11 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n> Yeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something\n> sensible should do it.\n\nCorrection (if somebody will pick this up), it's\nTRANSPORT_SUMMARY_WIDTH that needs to be adjusted, not those four.\n-- \nDuy\n"},{"id":"304621","messageId":"xmqqd1itd4t2.fsf@gitster.mtv.corp.google.com","threadId":"44348","inReplyTo":"CACsJy8CkK8K0ty3fv9qyD7wwB+81VPB-P1UUCbDJTJ4iM0Y+Cw@mail.gmail.com","subject":"Re: [BUG] fetch output is ugly in 'next'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-21T16:50:49Z","receivedAt":"2016-10-21T16:50:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Fri, Oct 21, 2016 at 7:11 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n>> Yeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something\n>> sensible should do it.\n>\n> Correction (if somebody will pick this up), it's\n> TRANSPORT_SUMMARY_WIDTH that needs to be adjusted, not those four.\n\nYes, it used to be and it still is (2 * DEFAULT_ABBREV + 3) but in\nthe new world order where default-abbrev is often -1 the expression\ndoes not make much sense.\n\nPerhaps something along this line?\n\n transport.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/transport.h b/transport.h\nindex 18d2cf8275..5339fabbad 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -127,7 +127,7 @@ struct transport {\n #define TRANSPORT_PUSH_CERT 2048\n #define TRANSPORT_PUSH_ATOMIC 4096\n \n-#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n+#define TRANSPORT_SUMMARY_WIDTH (2 * (DEFAULT_ABBREV < 0 ? 7 : DEFAULT_ABBREV) + 3)\n #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)\n \n /* Returns a transport suitable for the url */\n"},{"id":"304646","messageId":"20161021214218.u46qf3lch3wwiutp@sigill.intra.peff.net","threadId":"44348","inReplyTo":"xmqqd1itd4t2.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] fetch output is ugly in 'next'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-21T21:42:18Z","receivedAt":"2016-10-21T21:42:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2016 at 09:50:49AM -0700, Junio C Hamano wrote:\n\n> Duy Nguyen <pclouds@gmail.com> writes:\n> \n> > On Fri, Oct 21, 2016 at 7:11 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n> >> Yeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something\n> >> sensible should do it.\n> >\n> > Correction (if somebody will pick this up), it's\n> > TRANSPORT_SUMMARY_WIDTH that needs to be adjusted, not those four.\n> \n> Yes, it used to be and it still is (2 * DEFAULT_ABBREV + 3) but in\n> the new world order where default-abbrev is often -1 the expression\n> does not make much sense.\n> \n> Perhaps something along this line?\n> \n>  transport.h | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/transport.h b/transport.h\n> index 18d2cf8275..5339fabbad 100644\n> --- a/transport.h\n> +++ b/transport.h\n> @@ -127,7 +127,7 @@ struct transport {\n>  #define TRANSPORT_PUSH_CERT 2048\n>  #define TRANSPORT_PUSH_ATOMIC 4096\n>  \n> -#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n> +#define TRANSPORT_SUMMARY_WIDTH (2 * (DEFAULT_ABBREV < 0 ? 7 : DEFAULT_ABBREV) + 3)\n>  #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)\n\nThat doesn't apply on 'next', because we have already done the\nequivalent there. :)\n\nThe right way to spell \"7\" is FALLBACK_DEFAULT_ABBREV, which is handled\nby your 65acfeacaa.\n\nThe remaining issue is that the static abbreviation is not nearly long\nenough for git.git anymore; the auto-abbrev feature bumps my repo to a\nminimum of 10 characters (it may only be 9 on a fresh clone; I have a\ncouple remotes and active work in progress). So this isn't exactly a\nregression; it has always been the case that we may mis-align when the\nabbreviations ended up longer than the minimum. It's just that it didn't\nhappen all that often in most repos (but it probably did constantly in\nlinux.git).\n\nThe simplest band-aid fix would be to compute TRANSPORT_SUMMARY_WIDTH on\nthe fly, taking into account the minimum found by actually counting the\nobjects. That at least gets us back to where we were, with it mostly\nworking and occasionally ugly when there's an oddball collision (for\ngit.git anyway; it probably makes the kernel output much nicer).\n\nThe \"right\" fix is to queue up the list of ref updates to print, find\nthe abbreviations for each, and then print them all in one shot, knowing\nahead of time the size necessary to align them. This could also let us\nimprove the name-alignment.\n\n-Peff\n"},{"id":"304649","messageId":"xmqqa8dxbb9r.fsf@gitster.mtv.corp.google.com","threadId":"44348","inReplyTo":"20161021214218.u46qf3lch3wwiutp@sigill.intra.peff.net","subject":"Re: [BUG] fetch output is ugly in 'next'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-21T22:14:08Z","receivedAt":"2016-10-21T22:14:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The \"right\" fix is to queue up the list of ref updates to print, find\n> the abbreviations for each, and then print them all in one shot, knowing\n> ahead of time the size necessary to align them. This could also let us\n> improve the name-alignment.\n\nHeh, that was what I was starting to code while listening to\nJonathan and Stefan talk in a meeting ;-)\n\n"},{"id":"304650","messageId":"20161021223927.26364-3-gitster@pobox.com","threadId":"44348","inReplyTo":"20161021223927.26364-1-gitster@pobox.com","subject":"[PATCH 2/3] fetch: pass summary_width down the callchain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-21T22:39:26Z","receivedAt":"2016-10-21T22:39:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The leaf function on the \"fetch\" side that uses TRANSPORT_SUMMARY_WIDTH\nconstant is builtin/fetch.c::format_display() and it has two distinct\ncallchains.  The one that reports the primary result of fetch originates\nat store_updated_refs(); the other one that reports the pruning of\nthe remote-tracking refs originates at prune_refs().\n\nTeach these two places to pass summary_width down the callchain,\njust like we did for the \"push\" side in the previous commit.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/fetch.c | 37 +++++++++++++++++++++----------------\n 1 file changed, 21 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a9f12cc5cf..40696e5338 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -17,9 +17,6 @@\n #include \"argv-array.h\"\n #include \"utf8.h\"\n \n-#define TRANSPORT_SUMMARY(x) \\\n-\t(int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)\n-\n static const char * const builtin_fetch_usage[] = {\n \tN_(\"git fetch [<options>] [<repository> [<refspec>...]]\"),\n \tN_(\"git fetch [<options>] <group>\"),\n@@ -569,9 +566,12 @@ static void print_compact(struct strbuf *display,\n \n static void format_display(struct strbuf *display, char code,\n \t\t\t   const char *summary, const char *error,\n-\t\t\t   const char *remote, const char *local)\n+\t\t\t   const char *remote, const char *local,\n+\t\t\t   int summary_width)\n {\n-\tstrbuf_addf(display, \"%c %-*s \", code, TRANSPORT_SUMMARY(summary));\n+\tint width = (summary_width + strlen(summary) - gettext_width(summary));\n+\n+\tstrbuf_addf(display, \"%c %-*s \", code, width, summary);\n \tif (!compact_format)\n \t\tprint_remote_to_local(display, remote, local);\n \telse\n@@ -583,7 +583,8 @@ static void format_display(struct strbuf *display, char code,\n static int update_local_ref(struct ref *ref,\n \t\t\t    const char *remote,\n \t\t\t    const struct ref *remote_ref,\n-\t\t\t    struct strbuf *display)\n+\t\t\t    struct strbuf *display,\n+\t\t\t    int summary_width)\n {\n \tstruct commit *current = NULL, *updated;\n \tenum object_type type;\n@@ -597,7 +598,7 @@ static int update_local_ref(struct ref *ref,\n \tif (!oidcmp(&ref->old_oid, &ref->new_oid)) {\n \t\tif (verbosity > 0)\n \t\t\tformat_display(display, '=', _(\"[up to date]\"), NULL,\n-\t\t\t\t       remote, pretty_ref);\n+\t\t\t\t       remote, pretty_ref, summary_width);\n \t\treturn 0;\n \t}\n \n@@ -611,7 +612,7 @@ static int update_local_ref(struct ref *ref,\n \t\t */\n \t\tformat_display(display, '!', _(\"[rejected]\"),\n \t\t\t       _(\"can't fetch in current branch\"),\n-\t\t\t       remote, pretty_ref);\n+\t\t\t       remote, pretty_ref, summary_width);\n \t\treturn 1;\n \t}\n \n@@ -621,7 +622,7 @@ static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(\"updating tag\", ref, 0);\n \t\tformat_display(display, r ? '!' : 't', _(\"[tag update]\"),\n \t\t\t       r ? _(\"unable to update local ref\") : NULL,\n-\t\t\t       remote, pretty_ref);\n+\t\t\t       remote, pretty_ref, summary_width);\n \t\treturn r;\n \t}\n \n@@ -654,7 +655,7 @@ static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(msg, ref, 0);\n \t\tformat_display(display, r ? '!' : '*', what,\n \t\t\t       r ? _(\"unable to update local ref\") : NULL,\n-\t\t\t       remote, pretty_ref);\n+\t\t\t       remote, pretty_ref, summary_width);\n \t\treturn r;\n \t}\n \n@@ -670,7 +671,7 @@ static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(\"fast-forward\", ref, 1);\n \t\tformat_display(display, r ? '!' : ' ', quickref.buf,\n \t\t\t       r ? _(\"unable to update local ref\") : NULL,\n-\t\t\t       remote, pretty_ref);\n+\t\t\t       remote, pretty_ref, summary_width);\n \t\tstrbuf_release(&quickref);\n \t\treturn r;\n \t} else if (force || ref->force) {\n@@ -685,12 +686,12 @@ static int update_local_ref(struct ref *ref,\n \t\tr = s_update_ref(\"forced-update\", ref, 1);\n \t\tformat_display(display, r ? '!' : '+', quickref.buf,\n \t\t\t       r ? _(\"unable to update local ref\") : _(\"forced update\"),\n-\t\t\t       remote, pretty_ref);\n+\t\t\t       remote, pretty_ref, summary_width);\n \t\tstrbuf_release(&quickref);\n \t\treturn r;\n \t} else {\n \t\tformat_display(display, '!', _(\"[rejected]\"), _(\"non-fast-forward\"),\n-\t\t\t       remote, pretty_ref);\n+\t\t\t       remote, pretty_ref, summary_width);\n \t\treturn 1;\n \t}\n }\n@@ -721,6 +722,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tchar *url;\n \tconst char *filename = dry_run ? \"/dev/null\" : git_path_fetch_head();\n \tint want_status;\n+\tint summary_width = TRANSPORT_SUMMARY_WIDTH;\n \n \tfp = fopen(filename, \"a\");\n \tif (!fp)\n@@ -830,13 +832,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n \t\t\tstrbuf_reset(&note);\n \t\t\tif (ref) {\n-\t\t\t\trc |= update_local_ref(ref, what, rm, &note);\n+\t\t\t\trc |= update_local_ref(ref, what, rm, &note,\n+\t\t\t\t\t\t       summary_width);\n \t\t\t\tfree(ref);\n \t\t\t} else\n \t\t\t\tformat_display(&note, '*',\n \t\t\t\t\t       *kind ? kind : \"branch\", NULL,\n \t\t\t\t\t       *what ? what : \"HEAD\",\n-\t\t\t\t\t       \"FETCH_HEAD\");\n+\t\t\t\t\t       \"FETCH_HEAD\", summary_width);\n \t\t\tif (note.len) {\n \t\t\t\tif (verbosity >= 0 && !shown_url) {\n \t\t\t\t\tfprintf(stderr, _(\"From %.*s\\n\"),\n@@ -903,6 +906,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,\n \tint url_len, i, result = 0;\n \tstruct ref *ref, *stale_refs = get_stale_heads(refs, ref_count, ref_map);\n \tchar *url;\n+\tint summary_width = TRANSPORT_SUMMARY_WIDTH;\n \tconst char *dangling_msg = dry_run\n \t\t? _(\"   (%s will become dangling)\")\n \t\t: _(\"   (%s has become dangling)\");\n@@ -938,7 +942,8 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,\n \t\t\t\tshown_url = 1;\n \t\t\t}\n \t\t\tformat_display(&sb, '-', _(\"[deleted]\"), NULL,\n-\t\t\t\t       _(\"(none)\"), prettify_refname(ref->name));\n+\t\t\t\t       _(\"(none)\"), prettify_refname(ref->name),\n+\t\t\t\t       summary_width);\n \t\t\tfprintf(stderr, \" %s\\n\",sb.buf);\n \t\t\tstrbuf_release(&sb);\n \t\t\twarn_dangling_symref(stderr, dangling_msg, ref->name);\n-- \n2.10.1-723-g2384e83bc3\n\n"},{"id":"304651","messageId":"20161021223927.26364-1-gitster@pobox.com","threadId":"44348","inReplyTo":"xmqqa8dxbb9r.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 0/3] fetch output is ugly in 'next'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-21T22:39:24Z","receivedAt":"2016-10-21T22:39:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"It turns out that there are three codepaths, all of which have a set\nof refs that they want to show them using TRANSPORT_SUMMARY_WIDTH\nconstant.  The two preparatory steps turn the constant used at the\nleaf level into a parameter that is passed down through these\ncallchains, and the last step introduces a helper function that can\nbe used to compute the appropriate width to be fed to these\ncallchains.\n\nJunio C Hamano (3):\n  transport: pass summary_width down the callchain\n  fetch: pass summary_width down the callchain\n  transport: allow summary-width to be computed dynamically\n\n builtin/fetch.c | 37 +++++++++++++++++--------------\n transport.c     | 68 ++++++++++++++++++++++++++++++++++++---------------------\n transport.h     |  2 +-\n 3 files changed, 65 insertions(+), 42 deletions(-)\n\n-- \n2.10.1-723-g2384e83bc3\n\n"},{"id":"304652","messageId":"20161021223927.26364-2-gitster@pobox.com","threadId":"44348","inReplyTo":"20161021223927.26364-1-gitster@pobox.com","subject":"[PATCH 1/3] transport: pass summary_width down the callchain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-21T22:39:25Z","receivedAt":"2016-10-21T22:39:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The callchain that originates at transport_print_push_status()\neventually hits a single leaf function, print_ref_status(), that is\nused to show from what old object to what new object a ref got\nupdated, and the width of the part that shows old and new object\nnames used a constant TRANSPORT_SUMMARY_WIDTH.\n\nTeach the callchain to pass the width down from the top instead.\nThis allows a future enhancement to compute the necessary display\nwidth before calling down this callchain.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n transport.c | 63 +++++++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 38 insertions(+), 25 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 94d6dc3725..ec02b78924 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -295,7 +295,9 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v\n \t}\n }\n \n-static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg, int porcelain)\n+static void print_ref_status(char flag, const char *summary,\n+\t\t\t     struct ref *to, struct ref *from, const char *msg,\n+\t\t\t     int porcelain, int summary_width)\n {\n \tif (porcelain) {\n \t\tif (from)\n@@ -307,7 +309,7 @@ static void print_ref_status(char flag, const char *summary, struct ref *to, str\n \t\telse\n \t\t\tfprintf(stdout, \"%s\\n\", summary);\n \t} else {\n-\t\tfprintf(stderr, \" %c %-*s \", flag, TRANSPORT_SUMMARY_WIDTH, summary);\n+\t\tfprintf(stderr, \" %c %-*s \", flag, summary_width, summary);\n \t\tif (from)\n \t\t\tfprintf(stderr, \"%s -> %s\", prettify_refname(from->name), prettify_refname(to->name));\n \t\telse\n@@ -321,15 +323,16 @@ static void print_ref_status(char flag, const char *summary, struct ref *to, str\n \t}\n }\n \n-static void print_ok_ref_status(struct ref *ref, int porcelain)\n+static void print_ok_ref_status(struct ref *ref, int porcelain, int summary_width)\n {\n \tif (ref->deletion)\n-\t\tprint_ref_status('-', \"[deleted]\", ref, NULL, NULL, porcelain);\n+\t\tprint_ref_status('-', \"[deleted]\", ref, NULL, NULL,\n+\t\t\t\t porcelain, summary_width);\n \telse if (is_null_oid(&ref->old_oid))\n \t\tprint_ref_status('*',\n \t\t\t(starts_with(ref->name, \"refs/tags/\") ? \"[new tag]\" :\n \t\t\t\"[new branch]\"),\n-\t\t\tref, ref->peer_ref, NULL, porcelain);\n+\t\t\tref, ref->peer_ref, NULL, porcelain, summary_width);\n \telse {\n \t\tstruct strbuf quickref = STRBUF_INIT;\n \t\tchar type;\n@@ -349,12 +352,14 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)\n \t\tstrbuf_add_unique_abbrev(&quickref, ref->new_oid.hash,\n \t\t\t\t\t DEFAULT_ABBREV);\n \n-\t\tprint_ref_status(type, quickref.buf, ref, ref->peer_ref, msg, porcelain);\n+\t\tprint_ref_status(type, quickref.buf, ref, ref->peer_ref, msg,\n+\t\t\t\t porcelain, summary_width);\n \t\tstrbuf_release(&quickref);\n \t}\n }\n \n-static int print_one_push_status(struct ref *ref, const char *dest, int count, int porcelain)\n+static int print_one_push_status(struct ref *ref, const char *dest, int count,\n+\t\t\t\t int porcelain, int summary_width)\n {\n \tif (!count) {\n \t\tchar *url = transport_anonymize_url(dest);\n@@ -364,56 +369,60 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i\n \n \tswitch(ref->status) {\n \tcase REF_STATUS_NONE:\n-\t\tprint_ref_status('X', \"[no match]\", ref, NULL, NULL, porcelain);\n+\t\tprint_ref_status('X', \"[no match]\", ref, NULL, NULL,\n+\t\t\t\t porcelain, summary_width);\n \t\tbreak;\n \tcase REF_STATUS_REJECT_NODELETE:\n \t\tprint_ref_status('!', \"[rejected]\", ref, NULL,\n-\t\t\t\t\t\t \"remote does not support deleting refs\", porcelain);\n+\t\t\t\t \"remote does not support deleting refs\",\n+\t\t\t\t porcelain, summary_width);\n \t\tbreak;\n \tcase REF_STATUS_UPTODATE:\n \t\tprint_ref_status('=', \"[up to date]\", ref,\n-\t\t\t\t\t\t ref->peer_ref, NULL, porcelain);\n+\t\t\t\t ref->peer_ref, NULL, porcelain, summary_width);\n \t\tbreak;\n \tcase REF_STATUS_REJECT_NONFASTFORWARD:\n \t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n-\t\t\t\t\t\t \"non-fast-forward\", porcelain);\n+\t\t\t\t \"non-fast-forward\", porcelain, summary_width);\n \t\tbreak;\n \tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n \t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n-\t\t\t\t\t\t \"already exists\", porcelain);\n+\t\t\t\t \"already exists\", porcelain, summary_width);\n \t\tbreak;\n \tcase REF_STATUS_REJECT_FETCH_FIRST:\n \t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n-\t\t\t\t\t\t \"fetch first\", porcelain);\n+\t\t\t\t \"fetch first\", porcelain, summary_width);\n \t\tbreak;\n \tcase REF_STATUS_REJECT_NEEDS_FORCE:\n \t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n-\t\t\t\t\t\t \"needs force\", porcelain);\n+\t\t\t\t \"needs force\", porcelain, summary_width);\n \t\tbreak;\n \tcase REF_STATUS_REJECT_STALE:\n \t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n-\t\t\t\t\t\t \"stale info\", porcelain);\n+\t\t\t\t \"stale info\", porcelain, summary_width);\n \t\tbreak;\n \tcase REF_STATUS_REJECT_SHALLOW:\n \t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n-\t\t\t\t\t\t \"new shallow roots not allowed\", porcelain);\n+\t\t\t\t \"new shallow roots not allowed\",\n+\t\t\t\t porcelain, summary_width);\n \t\tbreak;\n \tcase REF_STATUS_REMOTE_REJECT:\n \t\tprint_ref_status('!', \"[remote rejected]\", ref,\n-\t\t\t\t\t\t ref->deletion ? NULL : ref->peer_ref,\n-\t\t\t\t\t\t ref->remote_status, porcelain);\n+\t\t\t\t ref->deletion ? NULL : ref->peer_ref,\n+\t\t\t\t ref->remote_status, porcelain, summary_width);\n \t\tbreak;\n \tcase REF_STATUS_EXPECTING_REPORT:\n \t\tprint_ref_status('!', \"[remote failure]\", ref,\n-\t\t\t\t\t\t ref->deletion ? NULL : ref->peer_ref,\n-\t\t\t\t\t\t \"remote failed to report status\", porcelain);\n+\t\t\t\t ref->deletion ? NULL : ref->peer_ref,\n+\t\t\t\t \"remote failed to report status\",\n+\t\t\t\t porcelain, summary_width);\n \t\tbreak;\n \tcase REF_STATUS_ATOMIC_PUSH_FAILED:\n \t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n-\t\t\t\t\t\t \"atomic push failed\", porcelain);\n+\t\t\t\t \"atomic push failed\", porcelain, summary_width);\n \t\tbreak;\n \tcase REF_STATUS_OK:\n-\t\tprint_ok_ref_status(ref, porcelain);\n+\t\tprint_ok_ref_status(ref, porcelain, summary_width);\n \t\tbreak;\n \t}\n \n@@ -427,25 +436,29 @@ void transport_print_push_status(const char *dest, struct ref *refs,\n \tint n = 0;\n \tunsigned char head_sha1[20];\n \tchar *head;\n+\tint summary_width = TRANSPORT_SUMMARY_WIDTH;\n \n \thead = resolve_refdup(\"HEAD\", RESOLVE_REF_READING, head_sha1, NULL);\n \n \tif (verbose) {\n \t\tfor (ref = refs; ref; ref = ref->next)\n \t\t\tif (ref->status == REF_STATUS_UPTODATE)\n-\t\t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n+\t\t\t\tn += print_one_push_status(ref, dest, n,\n+\t\t\t\t\t\t\t   porcelain, summary_width);\n \t}\n \n \tfor (ref = refs; ref; ref = ref->next)\n \t\tif (ref->status == REF_STATUS_OK)\n-\t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n+\t\t\tn += print_one_push_status(ref, dest, n,\n+\t\t\t\t\t\t   porcelain, summary_width);\n \n \t*reject_reasons = 0;\n \tfor (ref = refs; ref; ref = ref->next) {\n \t\tif (ref->status != REF_STATUS_NONE &&\n \t\t    ref->status != REF_STATUS_UPTODATE &&\n \t\t    ref->status != REF_STATUS_OK)\n-\t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n+\t\t\tn += print_one_push_status(ref, dest, n,\n+\t\t\t\t\t\t   porcelain, summary_width);\n \t\tif (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) {\n \t\t\tif (head != NULL && !strcmp(head, ref->name))\n \t\t\t\t*reject_reasons |= REJECT_NON_FF_HEAD;\n-- \n2.10.1-723-g2384e83bc3\n\n"},{"id":"304653","messageId":"20161021223927.26364-4-gitster@pobox.com","threadId":"44348","inReplyTo":"20161021223927.26364-1-gitster@pobox.com","subject":"[PATCH 3/3] transport: allow summary-width to be computed dynamically","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-21T22:39:27Z","receivedAt":"2016-10-21T22:39:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Now we have identified three callchains that have a set of refs that\nthey want to show their <old, new> object names in an aligned output,\nwe can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH\nwith a helper function call to transport_summary_width() that takes\nthe set of ref as a parameter.  This step does not yet iterate over\nthe refs and compute, which is left as an exercise to the readers.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/fetch.c | 4 ++--\n transport.c     | 7 ++++++-\n transport.h     | 2 +-\n 3 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 40696e5338..09813cd826 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -722,7 +722,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tchar *url;\n \tconst char *filename = dry_run ? \"/dev/null\" : git_path_fetch_head();\n \tint want_status;\n-\tint summary_width = TRANSPORT_SUMMARY_WIDTH;\n+\tint summary_width = transport_summary_width(ref_map);\n \n \tfp = fopen(filename, \"a\");\n \tif (!fp)\n@@ -906,7 +906,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,\n \tint url_len, i, result = 0;\n \tstruct ref *ref, *stale_refs = get_stale_heads(refs, ref_count, ref_map);\n \tchar *url;\n-\tint summary_width = TRANSPORT_SUMMARY_WIDTH;\n+\tint summary_width = transport_summary_width(stale_refs);\n \tconst char *dangling_msg = dry_run\n \t\t? _(\"   (%s will become dangling)\")\n \t\t: _(\"   (%s has become dangling)\");\ndiff --git a/transport.c b/transport.c\nindex ec02b78924..d4b8bf5f25 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -429,6 +429,11 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count,\n \treturn 1;\n }\n \n+int transport_summary_width(const struct ref *refs)\n+{\n+\treturn (2 * FALLBACK_DEFAULT_ABBREV + 3);\n+}\n+\n void transport_print_push_status(const char *dest, struct ref *refs,\n \t\t\t\t  int verbose, int porcelain, unsigned int *reject_reasons)\n {\n@@ -436,7 +441,7 @@ void transport_print_push_status(const char *dest, struct ref *refs,\n \tint n = 0;\n \tunsigned char head_sha1[20];\n \tchar *head;\n-\tint summary_width = TRANSPORT_SUMMARY_WIDTH;\n+\tint summary_width = transport_summary_width(refs);\n \n \thead = resolve_refdup(\"HEAD\", RESOLVE_REF_READING, head_sha1, NULL);\n \ndiff --git a/transport.h b/transport.h\nindex e783377e40..706d99e818 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -142,7 +142,7 @@ struct transport {\n #define TRANSPORT_PUSH_ATOMIC 8192\n #define TRANSPORT_PUSH_OPTIONS 16384\n \n-#define TRANSPORT_SUMMARY_WIDTH (2 * FALLBACK_DEFAULT_ABBREV + 3)\n+extern int transport_summary_width(const struct ref *refs);\n \n /* Returns a transport suitable for the url */\n struct transport *transport_get(struct remote *, const char *);\n-- \n2.10.1-723-g2384e83bc3\n\n"},{"id":"304665","messageId":"20161022041021.cpe7pooacl6tywqd@sigill.intra.peff.net","threadId":"44348","inReplyTo":"20161021223927.26364-4-gitster@pobox.com","subject":"Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-22T04:10:22Z","receivedAt":"2016-10-22T04:10:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2016 at 03:39:27PM -0700, Junio C Hamano wrote:\n\n> Now we have identified three callchains that have a set of refs that\n> they want to show their <old, new> object names in an aligned output,\n> we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH\n> with a helper function call to transport_summary_width() that takes\n> the set of ref as a parameter.  This step does not yet iterate over\n> the refs and compute, which is left as an exercise to the readers.\n\nThe final step could be something like this:\n\ndiff --git a/transport.c b/transport.c\nindex 4dac713063..c1eaa4a 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -443,7 +443,21 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count,\n \n int transport_summary_width(const struct ref *refs)\n {\n-\treturn (2 * FALLBACK_DEFAULT_ABBREV + 3);\n+\tint max_abbrev;\n+\n+\t/*\n+\t * Computing the complete set of abbreviated sha1s is expensive just to\n+\t * find their lengths, but we can at least find our real dynamic\n+\t * minimum by picking an arbitrary sha1.\n+\t */\n+\tif (refs)\n+\t\tmax_abbrev = strlen(find_unique_abbrev(refs->old_oid.hash,\n+\t\t\t\t\t\t       DEFAULT_ABBREV));\n+\telse\n+\t\tmax_abbrev = FALLBACK_DEFAULT_ABBREV;\n+\n+\t/* 2 abbreviated sha1s, plus \"...\" in between */\n+\treturn (2 * max_abbrev + 3);\n }\n \n void transport_print_push_status(const char *dest, struct ref *refs,\n\n\nwhich produces reasonable results for me. But if we really wanted the\ntrue value, I think we'd want to compute and store the abbreviated\nsha1s, and then the refactoring done by your series probably isn't the\nright direction.\n\nI think we'd instead want to replace \"struct strbuf *display\" passed\ndown to update_local_ref() with something more like:\n\n  struct ref_status_table {\n\tstruct ref_status_item {\n\t\tchar old_hash[GIT_SHA1_HEX + 1];\n\t\tchar new_hash[GIT_SHA1_HEX + 1];\n\t\tchar *remote_ref;\n\t\tchar *local_ref;\n\t\tchar *summary;\n\t\tchar code;\n\t} *items;\n\tsize_t alloc, nr;\n  };\n\nand then format_display() would just add to the list (including calling\nfind_unique_abbrev()), and then at the end we'd call a function to show\nthem all.\n\nThat would also get rid of prepare_format_display(), as we could easily\nwalk over the prepared list before printing any of them (as opposed to\nwhat that function does now, which is to walk over the ref map; that\nrequires that it know which refs are going to be printed).\n\n-Peff\n"},{"id":"304666","messageId":"xmqqoa2d9eum.fsf@gitster.mtv.corp.google.com","threadId":"44348","inReplyTo":"20161021223927.26364-4-gitster@pobox.com","subject":"Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-22T04:39:45Z","receivedAt":"2016-10-22T04:39:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Now we have identified three callchains that have a set of refs that\n> they want to show their <old, new> object names in an aligned output,\n> we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH\n> with a helper function call to transport_summary_width() that takes\n> the set of ref as a parameter.  This step does not yet iterate over\n> the refs and compute, which is left as an exercise to the readers.\n\nAnd this is the final one.\n\n-- >8 --\nFrom: Junio C Hamano <gitster@pobox.com>\nDate: Fri, 21 Oct 2016 21:33:06 -0700\nSubject: [PATCH] transport: compute summary-width dynamically\n\nNow all that is left to do is to actually iterate over the refs\nand measure the display width needed to show their abbreviation.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n transport.c | 18 +++++++++++++++++-\n 1 file changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/transport.c b/transport.c\nindex d4b8bf5f25..f1f95cf7c7 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -429,9 +429,25 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count,\n \treturn 1;\n }\n \n+static int measure_abbrev(const struct object_id *oid, int sofar)\n+{\n+\tchar hex[GIT_SHA1_HEXSZ + 1];\n+\tint w = find_unique_abbrev_r(hex, oid->hash, DEFAULT_ABBREV);\n+\n+\treturn (w < sofar) ? sofar : w;\n+}\n+\n int transport_summary_width(const struct ref *refs)\n {\n-\treturn (2 * FALLBACK_DEFAULT_ABBREV + 3);\n+\tint maxw;\n+\n+\tfor (maxw = -1; refs; refs = refs->next) {\n+\t\tmaxw = measure_abbrev(&refs->old_oid, maxw);\n+\t\tmaxw = measure_abbrev(&refs->new_oid, maxw);\n+\t}\n+\tif (maxw < 0)\n+\t\tmaxw = FALLBACK_DEFAULT_ABBREV;\n+\treturn (2 * maxw + 3);\n }\n \n void transport_print_push_status(const char *dest, struct ref *refs,\n-- \n2.10.1-723-g2384e83bc3\n\n"},{"id":"304669","messageId":"20161022050426.t2fifjqrldc6mebc@sigill.intra.peff.net","threadId":"44348","inReplyTo":"xmqqoa2d9eum.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-22T05:04:26Z","receivedAt":"2016-10-22T05:04:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2016 at 09:39:45PM -0700, Junio C Hamano wrote:\n\n> And this is the final one.\n> \n> -- >8 --\n> From: Junio C Hamano <gitster@pobox.com>\n> Date: Fri, 21 Oct 2016 21:33:06 -0700\n> Subject: [PATCH] transport: compute summary-width dynamically\n> \n> Now all that is left to do is to actually iterate over the refs\n> and measure the display width needed to show their abbreviation.\n\nI think we crossed emails. :) This is obviously correct, if we don't\nmind paying the find_unique_abbrev cost twice for each sha1.\n\n>  int transport_summary_width(const struct ref *refs)\n>  {\n> -\treturn (2 * FALLBACK_DEFAULT_ABBREV + 3);\n> +\tint maxw;\n> +\n> +\tfor (maxw = -1; refs; refs = refs->next) {\n> +\t\tmaxw = measure_abbrev(&refs->old_oid, maxw);\n> +\t\tmaxw = measure_abbrev(&refs->new_oid, maxw);\n> +\t}\n\nThis is a minor style nit, but I think it's better to avoid mixing\nunrelated bits between the initialization, condition, and iteration bits\nof a for loop. IOW:\n\n  int maxw = -1;\n\n  for (; refs; refs = refs->next) {\n\t...\n  }\n\nmakes the for-loop _just_ about iteration, and it is more clear that the\nmanipulation of \"maxw\" is a side effect of the loop that is valid after\nit finishes.\n\nThough in this particular case, the loop and the whole function are\nshort enough that I don't care that much. Just raising a philosophical\npoint. :)\n\n-Peff\n"},{"id":"304679","messageId":"xmqq4m449wqw.fsf@gitster.mtv.corp.google.com","threadId":"44348","inReplyTo":"20161022050426.t2fifjqrldc6mebc@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-22T16:25:27Z","receivedAt":"2016-10-22T16:25:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Oct 21, 2016 at 09:39:45PM -0700, Junio C Hamano wrote:\n>\n>> And this is the final one.\n>> \n>> -- >8 --\n>> From: Junio C Hamano <gitster@pobox.com>\n>> Date: Fri, 21 Oct 2016 21:33:06 -0700\n>> Subject: [PATCH] transport: compute summary-width dynamically\n>> \n>> Now all that is left to do is to actually iterate over the refs\n>> and measure the display width needed to show their abbreviation.\n>\n> I think we crossed emails. :) This is obviously correct, if we don't\n> mind paying the find_unique_abbrev cost twice for each sha1.\n\nIndeed we did.  I do not think the cost matters that much in the\ncodepath to produce the final summary output.\n\n> This is a minor style nit, but I think it's better to avoid mixing\n> unrelated bits between the initialization, condition, and iteration bits\n> of a for loop.\n\nYeah, you're right.\n"}]}