Re: Why repository grows after "git gc"? / Purpose of *.keep files?

Nicolas Pitre [off-list ref] writes:

Let's see what happens here even before any packing is attempted

|$ git rev-list --objects 492c2e4..9404ef0
|362
|
|$ git rev-list --objects --all \
|   --unpacked=pack-6a3438b2702be06697023d80b77e67a73a0b0b5c.pack |
|	wc -l
|26559

So this --unpacked= argument (which undocumented semantics I still have 
issues with) is certainly not doing what is expected.

The output from rev-list is not surprising.  --unpacked=$this.pack implies
the usual --unpacked behaviour (i.e. only show unpacked objects by not
traversing into commits that are packed) and at the same time pretends
that objects in $this.pack are loose.

It was meant to be used for a partial incremental repacking.  If you have
a pack to be kept (perhaps a highly packed deep pack that holds the
earlier parts of the history), marked with .keep, and a handful young
packs, you would give these young ones with --unpacked, so that the
resulting single pack contains all that are loose or in these young
packs.  After that, you can remove all the young packs and loose objects.

At least that is the idea.

I am not sure where that rev-list experiment you showed fits in the bigger
picture, but if that is used for repacking the young packs, perhaps the
issue is that after the repacking the code forgets to remove the young
ones whose objects are now moved into the new pack?

Junio C Hamano writes:
 > The output from rev-list is not surprising.  --unpacked=$this.pack implies
 > the usual --unpacked behaviour (i.e. only show unpacked objects by not
 > traversing into commits that are packed)

The problem is unconditional traversing into commits that are
unpacked. This behavior is immediately obvious if the packed blob in
the .keep pack is large. I've been using the following since the large
object discussion with Dana, but it might be completely broken (though
the test case is probably correct).

--

Previously --unpacked would filter on the commit level, ignoring whether the
objects comprising the commit actually were packed or unpacked.

This makes it impossible to store e.g. excessively large blobs in
different packs from the commits referencing them, since the next repack of
such a commit will suck all referenced blobs into the same pack.

This change moves the unpacked check to the output stage and no longer checks
the flag during commit traversal and adds a trivial test demonstrating the
problem.
---
 Note that t6009 is already taken, so it might be better to merge the test
 into one of the other rev-list tests.

 list-objects.c               |    6 ++++--
 revision.c                   |    2 --
 t/t6009-rev-list-unpacked.sh |   32 ++++++++++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 4 deletions(-)
 create mode 100644 t/t6009-rev-list-unpacked.sh

diff --git a/list-objects.c b/list-objects.c
index c8b8375..b378c0f 100644
--- a/list-objects.c
+++ b/list-objects.c

@@ -146,7 +146,8 @@ void traverse_commit_list(struct rev_info *revs,
 
 	while ((commit = get_revision(revs)) != NULL) {
 		process_tree(revs, commit->tree, &objects, NULL, "");
-		show_commit(commit);
+		if (!revs->unpacked || !has_sha1_pack(commit->object.sha1, revs->ignore_packed))
+			show_commit(commit);
 	}
 	for (i = 0; i < revs->pending.nr; i++) {
 		struct object_array_entry *pending = revs->pending.objects + i;

@@ -173,7 +174,8 @@ void traverse_commit_list(struct rev_info *revs,
 		    sha1_to_hex(obj->sha1), name);
 	}
 	for (i = 0; i < objects.nr; i++)
-		show_object(&objects.objects[i]);
+		if (!revs->unpacked || !has_sha1_pack(objects.objects[i].item->sha1, revs->ignore_packed))
+			show_object(&objects.objects[i]);
 	free(objects.objects);
 	if (revs->pending.nr) {
 		free(revs->pending.objects);

diff --git a/revision.c b/revision.c
index 4231ea2..0e90d3b 100644
--- a/revision.c
+++ b/revision.c

@@ -1508,8 +1508,6 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)
 {
 	if (commit->object.flags & SHOWN)
 		return commit_ignore;
-	if (revs->unpacked && has_sha1_pack(commit->object.sha1, revs->ignore_packed))
-		return commit_ignore;
 	if (revs->show_all)
 		return commit_show;
 	if (commit->object.flags & UNINTERESTING)

diff --git a/t/t6009-rev-list-unpacked.sh b/t/t6009-rev-list-unpacked.sh
new file mode 100644
index 0000000..6b65e83
--- /dev/null
+++ b/t/t6009-rev-list-unpacked.sh

@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='test git rev-list --unpacked --objects'
+
+. ./test-lib.sh
+
+# Create an unpacked commit that references a packed object.
+
+test_expect_success setup '
+	echo Hallo > foo &&
+	git add foo &&
+	test_tick &&
+	git commit -m "A" &&
+        git gc &&
+	echo Cello > bar &&
+	git add bar &&
+	test_tick &&
+	git commit -m "B"
+'
+
+test_expect_success \
+    'object list should contain foo' '
+    git rev-list --all --objects | grep -q "foo"
+'
+
+test_expect_success \
+    'unpacked object list should not contain foo' '
+    test_must_fail "git rev-list --all --unpacked --objects | grep -q \"foo\""
+'
+
+
+test_done

-- 
1.5.5.1.382.g7d84c

On Wed, 14 May 2008, Juergen Ruehle wrote:

Junio C Hamano writes:
 > The output from rev-list is not surprising.  --unpacked=$this.pack implies
 > the usual --unpacked behaviour (i.e. only show unpacked objects by not
 > traversing into commits that are packed)

The problem is unconditional traversing into commits that are
unpacked. This behavior is immediately obvious if the packed blob in
the .keep pack is large. 

That's what I was suspecting too.  And because the Linux repo contains 
many files, then a single commit will fetch a large bunch of objects 
indeed.

I've been using the following since the large
object discussion with Dana, but it might be completely broken (though
the test case is probably correct).

This is not some part of git code I'm familiar with, so I can't tell if 
the patch is broken or not.  What I can do is repeat my simple test 
which produces the following results with your patch:

|$ git rev-list --objects 492c2e4..9404ef0
|362
|
|$ git rev-list --objects --all \
|   --unpacked=pack-6a3438b2702be06697023d80b77e67a73a0b0b5c.pack |
|       wc -l
|362

That's exactly what is expected.


Nicolas

On Wed, 14 May 2008, Juergen Ruehle wrote:

Previously --unpacked would filter on the commit level, ignoring whether the
objects comprising the commit actually were packed or unpacked.

I think this patch is correct, but I wonder why you removed the pruning 
from revision.c? Why do we want to process trees for commits that aren't 
going to be shown? This is going to slow down things a lot, and we've long 
had the rule that commits have to be complete in the packs that are kept 
(ie you should never have a pack-file that points to an unpacked object).

So I'd suggest a slightly less intrusive patch (untested!!) instead, which 
leaves the commit object logic alone.

(Your test-case should obviously be merged regardless)

		Linus

---
 list-objects.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/list-objects.c b/list-objects.c
index c8b8375..8cb05ca 100644
--- a/list-objects.c
+++ b/list-objects.c

@@ -172,8 +172,12 @@ void traverse_commit_list(struct rev_info *revs,
 		die("unknown pending object %s (%s)",
 		    sha1_to_hex(obj->sha1), name);
 	}
-	for (i = 0; i < objects.nr; i++)
-		show_object(&objects.objects[i]);
+	for (i = 0; i < objects.nr; i++) {
+		struct object_array_entry *entry = &objects.objects[i];
+		if (revs->unpacked && has_sha1_pack(entry->item->sha1, revs->ignore_packed))
+			continue;
+		show_object(entry);
+	}
 	free(objects.objects);
 	if (revs->pending.nr) {
 		free(revs->pending.objects);

On Wed, 14 May 2008, Linus Torvalds wrote:

I think this patch is correct, but I wonder why you removed the pruning 
from revision.c?

In fact, it might be a good idea to not just keep it in revision.c, but 
move it up a bit, so that a commit that is packed and should be ignored 
won't even have its parents put on the list (which means that we not only 
ignore the trees in that commit, but also all parents).

Of course, the more aggressively we prune, the more we end up having to 
depend on the fact that a commit that is in a pack that is marked "keep" 
must *always* have everything that leads to it in that pack or others also 
marked "keep". We effectively have that already (because we've always 
pruned away the commits early), but it's a thing to keep in mind whenever 
we prune even more aggressively.

		Linus

On Wed, 14 May 2008, Linus Torvalds wrote:

Of course, the more aggressively we prune, the more we end up having to 
depend on the fact that a commit that is in a pack that is marked "keep" 
must *always* have everything that leads to it in that pack or others also 
marked "keep". We effectively have that already (because we've always 
pruned away the commits early), but it's a thing to keep in mind whenever 
we prune even more aggressively.

I wonder if this is a good thing.  Such a rule would effectively put 
restrictions on how objects like big blobs could be distributed amongst 
many .keep packs.  I just wish we're not painting ourselves in a corner.


Nicolas

On Wed, 14 May 2008, Nicolas Pitre wrote:

On Wed, 14 May 2008, Linus Torvalds wrote:

quoted

Of course, the more aggressively we prune, the more we end up having to 
depend on the fact that a commit that is in a pack that is marked "keep" 
must *always* have everything that leads to it in that pack or others also 
marked "keep". We effectively have that already (because we've always 
pruned away the commits early), but it's a thing to keep in mind whenever 
we prune even more aggressively.

I wonder if this is a good thing.  Such a rule would effectively put 
restrictions on how objects like big blobs could be distributed amongst 
many .keep packs.  I just wish we're not painting ourselves in a corner.

You can distribute big objects arbitrarily among many .keep packs, but 
what you can *NOT* do (and which has _always_ been a bug to do) is to have 
a *.keep pack that refers to objects that are not in a .keep pack!

So keep<->keep you can do anything you want, and distribute objects any 
way.

But a keep pack must only refer to objects in itself or in other keep 
packs.

Because otherwise, if we ever hit an object in a keep pack, we'll stop 
even looking further when we use --unpacked. And that has always been true 
(admittedly only for "commit" objects, but those are the ones that most 
commonly refer to other objects, so ..)

			Linus

Linus Torvalds wrote:

On Wed, 14 May 2008, Nicolas Pitre wrote:

quoted

On Wed, 14 May 2008, Linus Torvalds wrote:

quoted

Of course, the more aggressively we prune, the more we end up having to 
depend on the fact that a commit that is in a pack that is marked "keep" 
must *always* have everything that leads to it in that pack or others also 
marked "keep". We effectively have that already (because we've always 
pruned away the commits early), but it's a thing to keep in mind whenever 
we prune even more aggressively.

I wonder if this is a good thing.  Such a rule would effectively put 
restrictions on how objects like big blobs could be distributed amongst 
many .keep packs.  I just wish we're not painting ourselves in a corner.

You can distribute big objects arbitrarily among many .keep packs, but 
what you can *NOT* do (and which has _always_ been a bug to do) is to have 
a *.keep pack that refers to objects that are not in a .keep pack!

So keep<->keep you can do anything you want, and distribute objects any 
way.

But a keep pack must only refer to objects in itself or in other keep 
packs.

Because otherwise, if we ever hit an object in a keep pack, we'll stop 
even looking further when we use --unpacked. And that has always been true 
(admittedly only for "commit" objects, but those are the ones that most 
commonly refer to other objects, so ..)

Sounds like git-fsck needs to start checking for this.