Re: [PATCH] Add list-all-objects command

Jeff King [off-list ref] writes:

On Mon, Jun 22, 2015 at 04:38:22AM -0400, Jeff King wrote:

quoted

quoted

+	prepare_packed_git();
+	for (p = packed_git; p; p = p->next) {
+		open_pack_index(p);
+	}

Yikes. The fact that you need to do this means that
for_each_packed_object is buggy, IMHO. I'll send a patch.

Here's that patch. And since I did not want to pile work on Charles, I
went ahead and just implemented the patches I suggested in the other
email.

We may want to take patch 1 separately for the maint-track, as it is
really a bug-fix (albeit one that I do not think actually affects anyone
in practice right now).

Hmph, add_unseen_recent_objects_to_traversal() is the only existing
user, and before d3038d22 (prune: keep objects reachable from recent
objects, 2014-10-15) added that function, for-each-packed-object
existed but had no callers.

And the objects not beeing seen by that function (due to lack of
"open") would matter only for pruning purposes, which would mean
you have to be calling into the codepath when running a full repack,
so you would have opened all the packs that matter anyway (if you
have a "old cruft archive" pack that contains only objects that
are unreachable, you may not have opened that pack, though, and you
may prune the thing away prematurely).

So, I think I can agree that this would unlikely affect anybody in
practice.

Patches 2-5 are useful even if we go with Charles' command, as they make
cat-file better (cleanups and he new buffer option).

Patches 6-7 implement the cat-file option that would be redundant with
list-all-objects.

By the way, in addition to not showing objects in order,
list-all-objects (and my cat-file option) may show duplicates. Do we
want to "sort -u" for the user? It might be nice for them to always get
a de-duped and sorted list. Aside from the CPU cost of sorting, it does
mean we'll allocate ~80MB for the kernel to store the sha1s. I guess
that's not too much when you are talking about the kernel repo. I took
the coward's way out and just mentioned the limitation in the
documentation, but I'm happy to be persuaded.

  [1/7]: for_each_packed_object: automatically open pack index
  [2/7]: cat-file: minor style fix in options list
  [3/7]: cat-file: move batch_options definition to top of file
  [4/7]: cat-file: add --buffer option
  [5/7]: cat-file: stop returning value from batch_one_object
  [6/7]: cat-file: split batch_one_object into two stages
  [7/7]: cat-file: add --batch-all-objects option

-Peff

On Mon, Jun 22, 2015 at 02:50:10PM -0700, Junio C Hamano wrote:

quoted

We may want to take patch 1 separately for the maint-track, as it is
really a bug-fix (albeit one that I do not think actually affects anyone
in practice right now).

Hmph, add_unseen_recent_objects_to_traversal() is the only existing
user, and before d3038d22 (prune: keep objects reachable from recent
objects, 2014-10-15) added that function, for-each-packed-object
existed but had no callers.

I think that is because it was added by d3038d22^. :)

And the objects not beeing seen by that function (due to lack of
"open") would matter only for pruning purposes, which would mean
you have to be calling into the codepath when running a full repack,
so you would have opened all the packs that matter anyway (if you
have a "old cruft archive" pack that contains only objects that
are unreachable, you may not have opened that pack, though, and you
may prune the thing away prematurely).

Exactly, that matches my analysis.

So, I think I can agree that this would unlikely affect anybody in
practice.

Yep. I am OK if we do not even worry about it for "maint", then.

-Peff