From: Eric Sunshine <sunshine@sunshineco.com>
To: Karthik Nayak <karthik.188@gmail.com>
Cc: Git List <git@vger.kernel.org>,
Christian Couder <christian.couder@gmail.com>,
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 02/10] branch: refactor width computation
Date: Mon, 10 Aug 2015 21:58:10 -0400 [thread overview]
Message-ID: <CAPig+cRHvkAEM1ayVc5nj6RCih52x1+Bn9YTQPV93ej=3arK2Q@mail.gmail.com> (raw)
In-Reply-To: <1438693282-15516-2-git-send-email-Karthik.188@gmail.com>
On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Remove unnecessary variables from ref_list and ref_item which were
> used for width computation. This is to make ref_item similar to
> ref-filter's ref_array_item. This will ensure a smooth port of
> branch.c to use ref-filter APIs in further patches.
>
> Previously the maxwidth was computed when inserting the refs into the
> ref_list. Now, we obtain the entire ref_list and then compute
> maxwidth.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 4fc8beb..b058b74 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -557,16 +552,21 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
> -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;
Nit: Unnecessary work. You compute the width and then immediately
throw it away when 'ignore' is true.
Also, you use 'it' elsewhere rather than 'refs->list[i]' but not this
line, which makes it seem out of place. I would have expected to see:
if (it->ignore)
continue;
(Despite the noisier diff, the end result will be more consistent.)
> - 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;
> }
On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Remove unnecessary variables from ref_list and ref_item which were
> used for width computation. This is to make ref_item similar to
> ref-filter's ref_array_item. This will ensure a smooth port of
> branch.c to use ref-filter APIs in further patches.
>
> Previously the maxwidth was computed when inserting the refs into the
> ref_list. Now, we obtain the entire ref_list and then compute
> maxwidth.
>
> Based-on-patch-by: Jeff King <peff@peff.net>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> builtin/branch.c | 61 +++++++++++++++++++++++++++++---------------------------
> 1 file changed, 32 insertions(+), 29 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 4fc8beb..b058b74 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -282,14 +282,14 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> struct ref_item {
> char *name;
> char *dest;
> - unsigned int kind, width;
> + unsigned int kind;
> struct commit *commit;
> int ignore;
> };
>
> struct ref_list {
> struct rev_info revs;
> - int index, alloc, maxwidth, verbose, abbrev;
> + int index, alloc, verbose, abbrev;
> struct ref_item *list;
> struct commit_list *with_commit;
> int kinds;
> @@ -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;
> }
> @@ -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;
> default:
> color = BRANCH_COLOR_PLAIN;
> @@ -557,16 +552,21 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
> strbuf_release(&out);
> }
>
> -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;
> }
>
> static char *get_head_description(void)
> @@ -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);
> }
> }
> @@ -624,6 +621,16 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
> int i;
> struct append_ref_cb cb;
> struct ref_list ref_list;
> + int maxwidth = 0;
> + const char *remote_prefix = "";
> +
> + /*
> + * If we are listing more than just remote branches,
> + * then remote branches will have a "remotes/" prefix.
> + * We need to account for this in the width.
> + */
> + if (kinds != REF_REMOTE_BRANCH)
> + remote_prefix = "remotes/";
>
> memset(&ref_list, 0, sizeof(ref_list));
> ref_list.kinds = kinds;
> @@ -667,26 +674,22 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
> clear_commit_marks(item->commit, ALL_REV_FLAGS);
> }
> clear_commit_marks(filter, ALL_REV_FLAGS);
> -
> - if (verbose)
> - ref_list.maxwidth = calc_maxwidth(&ref_list);
> }
> + 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);
>
> for (i = 0; i < ref_list.index; i++) {
> int current = !detached &&
> (ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
> !strcmp(ref_list.list[i].name, head);
> - char *prefix = (kinds != REF_REMOTE_BRANCH &&
> - ref_list.list[i].kind == REF_REMOTE_BRANCH)
> - ? "remotes/" : "";
> - print_ref_item(&ref_list.list[i], ref_list.maxwidth, verbose,
> - abbrev, current, prefix);
> + print_ref_item(&ref_list.list[i], maxwidth, verbose,
> + abbrev, current, remote_prefix);
> }
>
> free_ref_list(&ref_list);
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2015-08-11 1:58 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-04 12:59 [PATCH 0/10] Port branch.c to ref-filter Karthik Nayak
2015-08-04 13:01 ` [PATCH 01/10] ref-filter: add option to filter only branches Karthik Nayak
2015-08-11 17:33 ` Junio C Hamano
2015-08-13 10:51 ` Karthik Nayak
2015-08-13 11:35 ` Karthik Nayak
2015-08-13 15:13 ` Karthik Nayak
2015-08-14 15:56 ` Junio C Hamano
2015-08-14 18:45 ` Karthik Nayak
2015-08-13 16:52 ` Matthieu Moy
2015-08-13 20:13 ` Karthik Nayak
2015-08-04 13:01 ` [PATCH 02/10] branch: refactor width computation Karthik Nayak
2015-08-11 1:58 ` Eric Sunshine [this message]
2015-08-11 13:10 ` Karthik Nayak
2015-08-04 13:01 ` [PATCH 03/10] branch: bump get_head_description() to the top Karthik Nayak
2015-08-11 1:59 ` Eric Sunshine
2015-08-11 13:12 ` Karthik Nayak
2015-08-04 13:01 ` [PATCH 04/10] branch: roll show_detached HEAD into regular ref_list Karthik Nayak
2015-08-11 2:41 ` Eric Sunshine
2015-08-11 13:17 ` Karthik Nayak
2015-08-04 13:01 ` [PATCH 05/10] branch: move 'current' check down to the presentation layer Karthik Nayak
2015-08-04 13:01 ` [PATCH 06/10] branch: drop non-commit error reporting Karthik Nayak
2015-08-04 13:01 ` [PATCH 07/10] branch.c: use 'ref-filter' data structures Karthik Nayak
2015-08-04 13:01 ` [PATCH 08/10] branch.c: use 'ref-filter' APIs Karthik Nayak
2015-08-04 13:01 ` [PATCH 09/10] branch: add '--points-at' option Karthik Nayak
2015-08-04 13:01 ` [PATCH 0/10] Port branch.c to ref-filter Karthik Nayak
2015-08-05 21:35 ` Junio C Hamano
2015-08-07 15:22 ` Karthik Nayak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPig+cRHvkAEM1ayVc5nj6RCih52x1+Bn9YTQPV93ej=3arK2Q@mail.gmail.com' \
--to=sunshine@sunshineco.com \
--cc=Matthieu.Moy@grenoble-inp.fr \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=karthik.188@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).