Re: [PATCH v2 1/6] commit-graph: refactor preparing commit graph

From: Stefan Beller <sbeller@google.com>
Date: Jul 9, 2018, 21:41 UTC
Message-ID: <CAGZ79kaE_42A2B=VjRcNdJzueeVZmrsj6iNgk=6u9bi6pA4hQA@mail.gmail.com>
In-Reply-To: <9769e2a7471c2b32185896e833cc74824ac5437e.1531168854.git.jonathantanmy@google.com>

Hi Jonathan, On Mon, Jul 9, 2018 at 1:44 PM Jonathan Tan <jonathantanmy@google.com> wrote:

Show 7 quoted lines

>
> Two functions in the code (1) check if the repository is configured for
> commit graphs, (2) call prepare_commit_graph(), and (3) check if the
> graph exists. Move (1) and (3) into prepare_commit_graph(), reducing
> duplication of code.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>

Show 8 quoted lines

>  static int prepare_commit_graph_run_once = 0;
> -static void prepare_commit_graph(void)
> +
> +/*
> + * Return 1 if commit_graph is non-NULL, and 0 otherwise.
> + *
> + * On the first invocation, this function attemps to load the commit
> + * graph if the_repository is configured to have one.

and as we talk about in-memory commit graph (and not some stale file that may still be around on the fs), we can assertly return 0 when core_commit_graph is false.

Makes sense!

Show 13 quoted lines

> @@ -337,22 +348,17 @@ static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item
>
>  int parse_commit_in_graph(struct commit *item)
>  {
> -       if (!core_commit_graph)
> +       if (!prepare_commit_graph())
>                 return 0;
> -
> -       prepare_commit_graph();
> -       if (commit_graph)
> -               return parse_commit_in_graph_one(commit_graph, item);
> -       return 0;
> +       return parse_commit_in_graph_one(commit_graph, item);

Makes sense.

Show 12 quoted lines

>  }
>
>  void load_commit_graph_info(struct commit *item)
>  {
>         uint32_t pos;
> -       if (!core_commit_graph)
> +       if (!prepare_commit_graph())
>                 return;
> -       prepare_commit_graph();
> -       if (commit_graph && find_commit_in_graph(item, commit_graph, &pos))
> +       if (find_commit_in_graph(item, commit_graph, &pos))
>                 fill_commit_graph_info(item, commit_graph, pos);

here too,

This is
Reviewed-by: Stefan Beller <sbeller@google.com>