| Message ID | 5ff56feab55b005b4a4d9559909ce7a08e5fa81e.1588641176.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: drop CHECK_OIDS, peel in callers | expand |
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
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
On Wed, May 13, 2020 at 01:48:48PM -0600, Taylor Blau wrote: > > 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. Yes, definitely. That's what I get for writing code in my email editor. :) -Peff
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; + if (oid_object_info(the_repository, &peeled, NULL) == OBJ_COMMIT) + oidset_insert(data->commits, &peeled); return 0; }
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. While we're at it, discard non-commit objects reachable from ref tips. This would be done automatically by 'fill_oids_from_commits()', but such functionality will be removed in a subsequent patch after the call to 'lookup_commit_reference_gently' is dropped (at which point a non-commit object in the commits oidset will become an error). Suggested-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- commit-graph.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)