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

Re: [RFC/PATCH 05/11] branch: fix width computation

From
Karthik Nayak <karthik.188@gmail.com>
Date
Jul 28, 2015, 18:16 UTC
Message-ID
<CAOLa=ZTsr3rFanRFwDW7YUE=VmcTgpaOHEKX2cJpfKOhW1FZGA@mail.gmail.com>
In-Reply-To
<vpq6154zk9o.fsf@anie.imag.fr>

On Tue, Jul 28, 2015 at 3:17 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 7 quoted lines
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>
> Why did send-email add this From: header? Strange, it has the same
> content as your actual From: field.
>
Not sure why, everything else came out fine. Idk what happened here.
Show 9 quoted lines
>> Remove unnecessary variables from ref_list and ref_item
>> which were used for width computation. Make other changes
>> accordingly. This patch is a precursor for porting branch.c
>> to use ref-filter APIs.
>
> You can explain better why this is needed. I guess something like "we're
> making struct ref_item similar to ref-filter's ref_array_item", but the
> reader shouldn't have to guess.
>
Will explain like you suggested.
> You should adujst the subject like BTW, I don't think you are "fixing"
> anything.
>
I guess refactor would be a better word here.
Show 7 quoted lines
> On a side note: on the "tag" series, see how explaining better and
> splitting patches led not only to a better history, but also to better
> final code, and to finding a actual bugs (the %(color) thing and the
> absence of reset on the state variable) even after sereval rounds of
> review? I'm being picky and demanding, but don't see that as a complain
> from me, just hints on getting even better ;-).
>

Haha, I look forward to reviews, they show things I usually miss out on, and help me get better. so keep them coming, I'll keep improving :) Thanks

Show 22 quoted lines
>> @@ -386,15 +386,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
>>       newitem->name = xstrdup(refname);
>>       newitem->kind = kind;
>>       newitem->commit = commit;
>> -     newitem->width = utf8_strwidth(refname);
>>       newitem->dest = resolve_symref(orig_refname, prefix);
>>       newitem->ignore = 0;
>> -     /* adjust for "remotes/" */
>> -     if (newitem->kind == REF_REMOTE_BRANCH &&
>> -         ref_list->kinds != REF_REMOTE_BRANCH)
>> -             newitem->width += 8;
>> -     if (newitem->width > ref_list->maxwidth)
>> -             ref_list->maxwidth = newitem->width;
>>
>>       return 0;
>>  }
>
> OK, in the old code, the width computation is done when inserting the
> ref into the array. In the new code, you build the array and then do the
> width computation. You can explain this better in the commit message I
> think (instead of "Make other changes accordingly" which doesn't bring
> much).

Okay I guess will do, just didn't want to explain the whole thing in the commit message.

Show 26 quoted lines
>
>> @@ -505,11 +498,12 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
>>  }
>>
>>  static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>> -                        int abbrev, int current, char *prefix)
>> +                        int abbrev, int current, const char *remote_prefix)
>>  {
>>       char c;
>>       int color;
>>       struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
>> +     const char *prefix = "";
>>
>>       if (item->ignore)
>>               return;
>> @@ -520,6 +514,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>>               break;
>>       case REF_REMOTE_BRANCH:
>>               color = BRANCH_COLOR_REMOTE;
>> +             prefix = remote_prefix;
>>               break;
>
> Why do you need these two hunks? I did not investigate in details, but
> it seems you're calling print_ref_item either with prefix="" or with
> prefix=remote_prefix so it would seem that keeping "prefix" as argument
> would work. I guess I missed something.

This is needed as whenever we do "git branch", show_detached() calls print_ref_item() with remote_prefix="". But print_ref_list() calls print_ref_item() with remote_prefix="remotes" (only when `git branch -a` is called remote_prefix=""). But only remote branches should be given the remotes prefix.

Show 31 quoted lines
>
>> -static int calc_maxwidth(struct ref_list *refs)
>> +static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
>>  {
>> -     int i, w = 0;
>> +     int i, max = 0;
>>       for (i = 0; i < refs->index; i++) {
>> +             struct ref_item *it = &refs->list[i];
>> +             int w = utf8_strwidth(it->name);
>> +
>>               if (refs->list[i].ignore)
>>                       continue;
>> -             if (refs->list[i].width > w)
>> -                     w = refs->list[i].width;
>> +             if (it->kind == REF_REMOTE_BRANCH)
>> +                     w += remote_bonus;
>> +             if (w > max)
>> +                     max = w;
>>       }
>> -     return w;
>> +     return max;
>>  }
>
> The old code was using 'w' for the max and no variable for the value at
> the current iteration. You're using 'max' for the max and 'w' at the
> current iteration. Using the same name 'w' for different things in the
> pre- and post-image of the patch distracts the reader.
>
> It may make sense to s/w/max/ in a separate preparatory patch. Or maybe
> it's overkill.
>

Since the change was minimal and easily understandable I think it's ok. But if you still feel otherwise, I'll be happy to introduce a preparatory patch.

Show 54 quoted lines
>> @@ -600,21 +600,18 @@ static char *get_head_description(void)
>>       return strbuf_detach(&desc, NULL);
>>  }
>>
>> -static void show_detached(struct ref_list *ref_list)
>> +static void show_detached(struct ref_list *ref_list, int maxwidth)
>>  {
>>       struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
>>
>>       if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
>>               struct ref_item item;
>>               item.name = get_head_description();
>> -             item.width = utf8_strwidth(item.name);
>>               item.kind = REF_LOCAL_BRANCH;
>>               item.dest = NULL;
>>               item.commit = head_commit;
>>               item.ignore = 0;
>> -             if (item.width > ref_list->maxwidth)
>> -                     ref_list->maxwidth = item.width;
>> -             print_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
>> +             print_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
>>               free(item.name);
>>       }
>>  }
> ...
>> +     int maxwidth = 0;
> ...
>> +     if (verbose)
>> +             maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
>>
>>       qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
>>
>>       detached = (detached && (kinds & REF_LOCAL_BRANCH));
>>       if (detached && match_patterns(pattern, "HEAD"))
>> -             show_detached(&ref_list);
>> +             show_detached(&ref_list, maxwidth);
>
> This hunks could ideally go in a preparatory patch that would just move
> the place where maxwidth is computed. This preparatory patch would just
> say
>
>         maxwidth = ref_list->maxwidth;
>
> and then you would do the actual change to
>
>         if (verbose)
>                 maxwidth = calc_maxwidth(...)
>
> without the distracting changes in the function's argument list.
>
> I won't insist on that, again it may be overkill. But reading the patch,
> I found it both rather trivial and hard to read, so there's room for
> improvement.
>

I find it too small to make a preparatory patch again, but if you really feel so, like I said, I'll change :)

-- 
Regards,
Karthik Nayak
Previous: Matthieu MoyNext: Karthik Nayak
Message 32 of 88 in “Port branch.c to use ref-filter APIs”
  1. Port branch.c to use ref-filter APIsKarthik Nayak, Jul 28, 2015
  2. 01/11 ref-filter: add "%(objectname:size=X)" optionKarthik Nayak, Jul 28, 2015
  3. 02/11 ref-filter: add 'colornext' atomKarthik Nayak, Jul 28, 2015
  4. Matthieu MoyJul 28, 2015
  5. Karthik NayakJul 28, 2015
  6. Christian CouderJul 28, 2015
  7. Karthik NayakJul 28, 2015
  8. Eric SunshineJul 29, 2015
  9. Matthieu MoyJul 29, 2015
  10. Jacob KellerJul 30, 2015
  11. Junio C HamanoJul 30, 2015
  12. Karthik NayakAug 1, 2015
  13. 03/11 ref-filter: add option to filter only branchesKarthik Nayak, Jul 28, 2015
  14. Matthieu MoyJul 28, 2015
  15. Karthik NayakJul 28, 2015
  16. 04/11 ref-filter: add 'ifexists' atomKarthik Nayak, Jul 28, 2015
  17. Jacob KellerJul 28, 2015
  18. Karthik NayakJul 28, 2015
  19. Matthieu MoyJul 28, 2015
  20. Karthik NayakJul 28, 2015
  21. Junio C HamanoJul 28, 2015
  22. Karthik NayakJul 29, 2015
  23. Junio C HamanoJul 29, 2015
  24. Junio C HamanoJul 29, 2015
  25. Matthieu MoyJul 29, 2015
  26. Junio C HamanoJul 29, 2015
  27. Karthik NayakAug 1, 2015
  28. Jacob KellerAug 1, 2015
  29. Karthik NayakAug 1, 2015
  30. 05/11 branch: fix width computationKarthik Nayak, Jul 28, 2015
  31. Matthieu MoyJul 28, 2015
  32. Karthik NayakJul 28, 2015
  33. 06/11 branch: roll show_detached HEAD into regular ref_listKarthik Nayak, Jul 28, 2015
  34. Matthieu MoyJul 28, 2015
  35. Karthik NayakJul 28, 2015
  36. Matthieu MoyJul 29, 2015
  37. Karthik NayakJul 29, 2015
  38. 07/11 branch: move 'current' check down to the presentation layerKarthik Nayak, Jul 28, 2015
  39. Matthieu MoyJul 28, 2015
  40. Karthik NayakJul 28, 2015
  41. Jacob KellerJul 29, 2015
  42. Karthik NayakJul 29, 2015
  43. Matthieu MoyJul 29, 2015
  44. Karthik NayakJul 29, 2015
  45. Matthieu MoyJul 29, 2015
  46. Karthik NayakAug 1, 2015
  47. Jacob KellerAug 1, 2015
  48. Eric SunshineAug 1, 2015
  49. Karthik NayakAug 2, 2015
  50. Junio C HamanoAug 2, 2015
  51. 08/11 branch: drop non-commit error reportingKarthik Nayak, Jul 28, 2015
  52. 09/11 branch.c: use 'ref-filter' data structuresKarthik Nayak, Jul 28, 2015
  53. Christian CouderJul 28, 2015
  54. Matthieu MoyJul 28, 2015
  55. Karthik NayakJul 28, 2015
  56. Matthieu MoyJul 29, 2015
  57. Karthik NayakJul 29, 2015
  58. Karthik NayakJul 28, 2015
  59. Christian CouderJul 28, 2015
  60. Karthik NayakJul 28, 2015
  61. Matthieu MoyJul 28, 2015
  62. Karthik NayakJul 28, 2015
  63. Junio C HamanoJul 28, 2015
  64. Karthik NayakJul 28, 2015
  65. Junio C HamanoJul 28, 2015
  66. Karthik NayakJul 29, 2015
  67. 10/11 branch.c: use 'ref-filter' APIsKarthik Nayak, Jul 28, 2015
  68. Jacob KellerJul 28, 2015
  69. Karthik NayakJul 29, 2015
  70. Christian CouderJul 28, 2015
  71. Karthik NayakJul 29, 2015
  72. Matthieu MoyJul 28, 2015
  73. Karthik NayakJul 29, 2015
  74. Matthieu MoyJul 29, 2015
  75. Karthik NayakJul 29, 2015
  76. Matthieu MoyJul 29, 2015
  77. Karthik NayakJul 30, 2015
  78. Matthieu MoyJul 30, 2015
  79. Karthik NayakAug 3, 2015
  80. Matthieu MoyAug 19, 2015
  81. Karthik NayakAug 19, 2015
  82. 11/11 branch: add '--points-at' optionKarthik Nayak, Jul 28, 2015
  83. Jacob KellerJul 28, 2015
  84. Karthik NayakJul 29, 2015
  85. Matthieu MoyJul 28, 2015
  86. Karthik NayakJul 28, 2015
  87. Matthieu MoyJul 28, 2015
  88. Karthik NayakJul 29, 2015

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

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