* Re: rev-list does not output commit metadata (nor honor --format) when --use-bitmap-index is on
2024-07-11 18:12 rev-list does not output commit metadata (nor honor --format) when --use-bitmap-index is on Jullyana Ramos
@ 2024-07-17 7:44 ` Jeff King
2024-07-17 15:46 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2024-07-17 7:44 UTC (permalink / raw)
To: Jullyana Ramos; +Cc: git@vger.kernel.org
On Thu, Jul 11, 2024 at 06:12:16PM +0000, Jullyana Ramos wrote:
> What did you do before the bug happened? (Steps to reproduce your issue)
> I ran "git rev-list --use-bitmap-index HEAD" to see history while taking advantage of bitmap index.
>
> What did you expect to happen? (Expected behavior)
> A list of commits to be output, including their metadata (message, author, etc.)
>
> What happened instead? (Actual behavior)
> Only the commit SHAs are being output, no matter what I pass to --format.
> Note that this only reproduces if bitmap index has been created for the repository.
>
> What's different between what you expected and what actually happened?
> The commit metadata is missing.
This is the expected output, but perhaps not by anybody who isn't
familiar with the rev-list and bitmap internals. ;) The --use-bitmap-index
option was introduced to produce answers as quickly as possible, and
typically for "--objects" (where the graph traversal is very expensive).
So it uses its own code paths for printing out the bitmap result. And
notably it will not show the results in any defined order (though I
think in practice it will often follow the order of objects in the pack,
which roughly corresponds with traversal order).
So I don't think anybody gave much thought to what it would do with
--format, or many other options. And then to make things doubly
confusing, --use-bitmap-index is best-effort: if there are no bitmaps,
or we can't load them for some reason, we'll fall back to the existing
code paths. So you would get the formatted output in that case!
So I'd mostly consider this to be a documentation bug (which does
mention one of the caveats, but definitely not the ordering or format
issues). That could certainly be improved.
Alternatively, or in addition, we could perhaps be more careful about
rejecting certain options used with --use-bitmap-index (but enumerating
the complete set feels like a maintenance nightmare).
And finally, we might consider giving more guidance in the documentation
on when to use --use-bitmap-index. If you're going to use --format
anyway, then bitmaps are not buying you much in the first place; you
still have to open every commit object to show it! They might help if
your query requires a lot of traversing but only outputs a few commits
(so "HEAD~3..HEAD" or similar). But I'd actually expect commit graphs to
do just as good a job (or better) these days.
All that said, I think you _could_ make it work as you expect with
something like the patch below. I'm hesitant to pursue that, though, as
it is probably just opening up even more opportunities for confusion
(e.g., --graph will be nonsense in this case due to the ordering issue).
> I do not know if this change was intentional, but I can reproduce the
> issue with version 2.34.1 (Ubuntu) and 2.45.2 (Windows) and I could
> not reproduce it with version 2.3.5 (Windows). (I know, huge gap, my
> apologies) I did make sure this was a Git issue, not a Git for Windows
> issue. I can reproduce it with a brand new repository. Create a
> commit, run rev-list, commit metadata is there. Generate bitmap index
> and metadata goes missing.
I was surprised that it would have ever worked. But it is just that
2.3.5 predates 4eb707ebd6 (rev-list: allow commit-only bitmap
traversals, 2020-02-14). Before then we skipped bitmap traversal unless
--objects was specified.
But even back then, running "rev-list --use-bitmap-index --objects
--format=medium" would ignore "--format".
-Peff
---
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 97d077a994..ec1dfd8722 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -369,15 +369,32 @@ static int show_bisect_vars(struct rev_list_info *info, int reaches, int all)
return 0;
}
+/*
+ * Gross hack. The show_reachable_fn type should really take a
+ * void pointer through which we'd pass this.
+ */
+static struct rev_list_info *fast_info;
+
static int show_object_fast(
const struct object_id *oid,
- enum object_type type UNUSED,
+ enum object_type type,
int exclude UNUSED,
uint32_t name_hash UNUSED,
struct packed_git *found_pack UNUSED,
off_t found_offset UNUSED)
{
- fprintf(stdout, "%s\n", oid_to_hex(oid));
+ if (fast_info->revs->commit_format == CMIT_FMT_UNSPECIFIED) {
+ fprintf(stdout, "%s\n", oid_to_hex(oid));
+ } else if (type == OBJ_COMMIT) {
+ struct commit *c = lookup_commit(the_repository, oid);
+ parse_commit_or_die(c);
+ show_commit(c, fast_info);
+ return 1;
+ } else {
+ struct object *obj =
+ lookup_object_by_type(the_repository, oid, type);
+ show_object(obj, "", fast_info);
+ }
return 1;
}
@@ -472,9 +489,10 @@ static int try_bitmap_count(struct rev_info *revs,
return 0;
}
-static int try_bitmap_traversal(struct rev_info *revs,
+static int try_bitmap_traversal(struct rev_list_info *info,
int filter_provided_objects)
{
+ struct rev_info *revs = info->revs;
struct bitmap_index *bitmap_git;
/*
@@ -488,6 +506,7 @@ static int try_bitmap_traversal(struct rev_info *revs,
if (!bitmap_git)
return -1;
+ fast_info = info;
traverse_bitmap_commit_list(bitmap_git, revs, &show_object_fast);
free_bitmap_index(bitmap_git);
return 0;
@@ -729,7 +748,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
goto cleanup;
if (!try_bitmap_disk_usage(&revs, filter_provided_objects))
goto cleanup;
- if (!try_bitmap_traversal(&revs, filter_provided_objects))
+ if (!try_bitmap_traversal(&info, filter_provided_objects))
goto cleanup;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread