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
