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

[Jeff King <peff@peff.net>]

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

> 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. :)

> 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