| Message ID | 56403dd377b996d21a4da1e01ffd6e691ac120bd.1589407014.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: drop CHECK_OIDS, peel in callers | expand |
On Wed, May 13, 2020 at 03:59:44PM -0600, Taylor Blau wrote: > -static int read_one_commit(struct oidset *commits, const char *hash) > +static int read_one_commit(struct oidset *commits, struct progress *progress, > + const char *hash) > { > + struct commit *result; > struct object_id oid; > const char *end; > > if (parse_oid_hex(hash, &oid, &end)) > return error(_("unexpected non-hex object ID: %s"), hash); > > - oidset_insert(commits, &oid); > + result = lookup_commit_reference_gently(the_repository, &oid, 1); > + if (result) > + oidset_insert(commits, &result->object.oid); > + else > + return error(_("invalid commit object id: %s"), hash); > + > + display_progress(progress, oidset_size(commits)); > + I expected this to switch to deref_tag() here, but it looks like you do it in the final commit. That makes sense, since this step is really just copying the existing logic. > @@ -249,6 +265,8 @@ static int graph_write(int argc, const char **argv) > cleanup: > UNLEAK(pack_indexes); > strbuf_release(&buf); > + if (progress) > + stop_progress(&progress); > return result; Really minor nit, but stop_progress(), like display_progress(), handles NULL already. So you can lose the "if" here. -Peff
On Thu, May 14, 2020 at 02:01:50PM -0400, Jeff King wrote: > On Wed, May 13, 2020 at 03:59:44PM -0600, Taylor Blau wrote: > > > -static int read_one_commit(struct oidset *commits, const char *hash) > > +static int read_one_commit(struct oidset *commits, struct progress *progress, > > + const char *hash) > > { > > + struct commit *result; > > struct object_id oid; > > const char *end; > > > > if (parse_oid_hex(hash, &oid, &end)) > > return error(_("unexpected non-hex object ID: %s"), hash); > > > > - oidset_insert(commits, &oid); > > + result = lookup_commit_reference_gently(the_repository, &oid, 1); > > + if (result) > > + oidset_insert(commits, &result->object.oid); > > + else > > + return error(_("invalid commit object id: %s"), hash); > > + > > + display_progress(progress, oidset_size(commits)); > > + > > I expected this to switch to deref_tag() here, but it looks like you do > it in the final commit. That makes sense, since this step is really just > copying the existing logic. I went back and forth on this. I figure that the behavior change should come in the final commit, so this is keeping things consistent at this point in the series. > > @@ -249,6 +265,8 @@ static int graph_write(int argc, const char **argv) > > cleanup: > > UNLEAK(pack_indexes); > > strbuf_release(&buf); > > + if (progress) > > + stop_progress(&progress); > > return result; > > Really minor nit, but stop_progress(), like display_progress(), handles > NULL already. So you can lose the "if" here. Mm. If there's nothing else (besides the review of 4/8 which I already sent a replacement patch for), I'd prefer to leave this as-is to avoid another reroll. Hopefully since this isn't hurting anything with the extra if statement, it shouldn't matter too much. If you feel strongly, I'm happy to re-send this series, or this patch. > -Peff Thanks, Taylor
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index f6647449ed..83c55d9227 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -6,6 +6,7 @@ #include "repository.h" #include "commit-graph.h" #include "object-store.h" +#include "progress.h" static char const * const builtin_commit_graph_usage[] = { N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"), @@ -138,15 +139,24 @@ static int write_option_parse_split(const struct option *opt, const char *arg, return 0; } -static int read_one_commit(struct oidset *commits, const char *hash) +static int read_one_commit(struct oidset *commits, struct progress *progress, + const char *hash) { + struct commit *result; struct object_id oid; const char *end; if (parse_oid_hex(hash, &oid, &end)) return error(_("unexpected non-hex object ID: %s"), hash); - oidset_insert(commits, &oid); + result = lookup_commit_reference_gently(the_repository, &oid, 1); + if (result) + oidset_insert(commits, &result->object.oid); + else + return error(_("invalid commit object id: %s"), hash); + + display_progress(progress, oidset_size(commits)); + return 0; } @@ -157,6 +167,7 @@ static int graph_write(int argc, const char **argv) struct object_directory *odb = NULL; int result = 0; enum commit_graph_write_flags flags = 0; + struct progress *progress = NULL; static struct option builtin_commit_graph_write_options[] = { OPT_STRING(0, "object-dir", &opts.obj_dir, @@ -230,13 +241,18 @@ static int graph_write(int argc, const char **argv) } else if (opts.stdin_commits) { oidset_init(&commits, 0); flags |= COMMIT_GRAPH_WRITE_CHECK_OIDS; + if (opts.progress) + progress = start_delayed_progress( + _("Collecting commits from input"), 0); while (strbuf_getline(&buf, stdin) != EOF) { - if (read_one_commit(&commits, buf.buf)) { + if (read_one_commit(&commits, progress, buf.buf)) { result = 1; goto cleanup; } } + + } if (write_commit_graph(odb, @@ -249,6 +265,8 @@ static int graph_write(int argc, const char **argv) cleanup: UNLEAK(pack_indexes); strbuf_release(&buf); + if (progress) + stop_progress(&progress); return result; }
When given a list of commits, the commit-graph machinery calls 'lookup_commit_reference_gently()' on each element in the set and treats the resulting set of OIDs as the base over which to close for reachability. In an earlier collection of commits, the 'git commit-graph write --reachable' case made the inner-most call to 'lookup_commit_reference_gently()' by peeling references before they were passed over to the commit-graph internals. Do the analog for 'git commit-graph write --stdin-commits' by calling 'lookup_commit_reference_gently()' outside of the commit-graph machinery, making the inner-most call a noop. Since this may incur additional processing time, surround 'read_one_commit' with a progress meter to provide output to the caller. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- builtin/commit-graph.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)