Re: [RFC PATCH 3/4] name-rev: check if a commit should be named before naming it

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 1, 2019, 18:05 UTC
Message-ID: <CAPig+cQmZOWUZuj2+z+qrrCO-YBMNiEnGic1_FwAcaaj5gA5-w@mail.gmail.com>
In-Reply-To: <20190301175024.17337-4-alban.gruin@gmail.com>

On Fri, Mar 1, 2019 at 12:50 PM Alban Gruin <alban.gruin@gmail.com> wrote:

> Until now, name_rev() named every commit it found, even it the name

s/even it/even if/

Show 10 quoted lines

> would ultimately be unused.
> [...]
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> @@ -107,12 +107,18 @@ static int name_rev(struct commit *commit,
>  copy_data:
> -               name->tip_name = tip_name;
> +               if (commit_list_contains(commits, commit) ||
> +                   commit_list_count(commits) == 0) {

Minor: It would probably be more efficient to check if the count is 0
first, and only search the list if not.

By the way, how big is 'commits' likely to get? Will the linear scan done by commit_list_contains() become an issue? Should it be using a hash table instead?

Show 5 quoted lines

> +                       name->tip_name = tip_name;
> +                       free_alloc = 0;
> +               } else {
> +                       name->tip_name = NULL;
> +               }