Re: [PATCH 3/8] commit-graph.c: peel refs in 'add_ref_to_set'

From: Jeff King <peff@peff.net>
Date: May 7, 2020, 19:54 UTC
Message-ID: <20200507195441.GA29683@coredump.intra.peff.net>
In-Reply-To: <5ff56feab55b005b4a4d9559909ce7a08e5fa81e.1588641176.git.me@ttaylorr.com>

On Mon, May 04, 2020 at 07:13:43PM -0600, Taylor Blau wrote:

Show 9 quoted lines

> While iterating references (to discover the set of commits to write to
> the commit-graph with 'git commit-graph write --reachable'),
> 'add_ref_to_set' can save 'fill_oids_from_commits()' some time by
> peeling the references beforehand.
> 
> Move peeling out of 'fill_oids_from_commits()' and into
> 'add_ref_to_set()' to use 'peel_ref()' instead of 'deref_tag()'. Doing
> so allows the commit-graph machinery to use the peeled value from
> '$GIT_DIR/packed-refs' instead of having to load and parse tags.

Or having to load and parse commits only to find out that they're not tags. :)

Show 16 quoted lines

> diff --git a/commit-graph.c b/commit-graph.c
> index 8f61256b0a..5c3fad0dd7 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1327,11 +1327,15 @@ static int add_ref_to_set(const char *refname,
>  			  const struct object_id *oid,
>  			  int flags, void *cb_data)
>  {
> +	struct object_id peeled;
>  	struct refs_cb_data *data = (struct refs_cb_data *)cb_data;
>  
>  	display_progress(data->progress, oidset_size(data->commits) + 1);
>  
> -	oidset_insert(data->commits, oid);
> +	if (peel_ref(refname, &peeled))
> +		peeled = *oid;

It may be the old-timer C programmer in me, but I always look slightly suspicious at struct assignments. We know that object_id doesn't need a deep copy, so it's obviously OK here. But should we use oidcpy() as a style thing?

Alternatively, you could do this without a struct copy at all with:

  if (!peel_ref(...))
         oid = peeled;
  oidset_insert(..., oid);

which is actually a bit cheaper.

> +	if (oid_object_info(the_repository, &peeled, NULL) == OBJ_COMMIT)
> +		oidset_insert(data->commits, &peeled);

I probably would have left adding this "if" until a later step, but I think it's OK here.

-Peff