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

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:

quoted

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

quoted

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.

quoted

+	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