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

[Taylor Blau <me@ttaylorr.com>]

On Thu, May 07, 2020 at 03:54:41PM -0400, Jeff King wrote:
> 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.

Makes sense, I think this version is the better of the two that you
suggested here. I noticed one small thing which is that since peeled is
only on the stack, I think we actually want 'oid = &peeled', but
otherwise I took this as-is.

> > +	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.

Yeah, I did it here to avoid having to add a seemingly-unrelated change
later on. I agree it doesn't matter much, so in the interest of leaving
the series alone, I'll leave it where it is here.

> -Peff

Thanks,
Taylor