builtin git-bundle - pack contains many more objects than required

builtin-bundle in version v1.5.0.3-271-g5ced057 (commit 5ced0572217f82f20c4a3460) (and probably before) includes far more objects in the pack than are actually necessary. Comparison to the original shell scripts I wrote shows the prerequisites and refs always agree between those (and that is what I tested before), so the problem is apparently in the arguments as passed to pack-objects. I confess to not understanding the revision walking code called from builtin-bundle.c, so I do not have a patch.

However, a simple example:

git>git-rev-list --objects HEAD~1..HEAD 5ced0572217f82f20c4a3460492768e07c08aeea 1d30eaabe4b6f7218e4e4cfff5670a493aa7358e 2a1d6a2be1511f65b601897f05962e5f673257d8 contrib cbd77b2f57114d4fa2f119f1b1ee17968d6d67d4 contrib/emacs 8554e3967cc692c6916e5aee35952074d07e8bf0 contrib/emacs/Makefile

shows that the bundle for this case should have five objects as shown. In this case, 916 objects are actually included in the pack when executing

git bundle create test.bdl HEAD~1..HEAD

Mark

The handcrafted built-in rev-list lookalike forgot to mark the trees and blobs contained in the boundary commits uninteresting, resulting in unnecessary objects in the pack.

---

* I'll have this in 'master' tonight. Also I am thinking about changing 'git bundle verify' to spit out heads and prereqs unconditionally, in addition to 'foo.bdl is ok'. Comments?

 builtin-bundle.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

index d41a413..279b8f8 100644
@@ -263,6 +263,11 @@ static void show_object(struct object_array_entry *p)
 	write_or_die(1, "\n", 1);
 }
 
+static void show_edge(struct commit *commit)
+{
+	; /* nothing to do */
+}
+
 static int create_bundle(struct bundle_header *header, const char *path,
 		int argc, const char **argv)
 {

@@ -341,6 +346,7 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	dup2(in, 1);
 	close(in);
 	prepare_revision_walk(&revs);
+	mark_edges_uninteresting(revs.commits, &revs, show_edge);
 	traverse_commit_list(&revs, show_commit, show_object);
 	close(1);
 	while (waitpid(pid, &status, 0) < 0)

1.5.0.3.862.g71037

This works for things like master~1..master, but fails on git bundle create t.bdl master --since=1.day.ago. Apparently the boundary commits marked with a date are not being honored in creating a pack.

Mark

That one is caused by the broken revision traversal in 'master' and being worked on in 'next'. Care to try the one from 'next' instead?

using next as just pulled from kernel.org (09890a9bce0bc27182bc1f74a34b53) ...

git>git bundle create test.bdl HEAD~1..HEAD
error: rev-list died 255

I have not found any rev-args set that avoids that error.

Mark

Hi,

Have you tried "make test"? In particular, t5510-fetch.sh?

If it passes, git-bundle works at least for one particular case, and I'd suspect then that the inbuilt GIT_EXEC_PATH bites you. To avoid that particular peculiarity, just "export GIT_EXEC_PATH=/path/to/next/", and try again.

Hiw,
Dscho

Yes, setting GIT_EXEC_PATH fixed the problem, thanks.

I just tried git-bundle in a repository where I just committed 1 file, the previous commit is several weeks old.

git-bundle create test.bdl HEAD --since=1.day.ago ==>> pack with 1531 objects
git-bundle create test.bdl HEAD ^HEAD~1 ==>> pack with 3 objects

But, both should only have 3 objects. So, I think the boundary marking with date limiting still has a problem. Apparently, every blob supporting the included commits are included in the pack, even if those blobs are also part of the pack's prerequisites.

Mark

Hi,

Wonderful!

Did you test with "--since=1.day.ago HEAD", i.e. with the correct order? I know you'd like the options to be interminglable, but "HEAD" really is not an option, but an argument.

Hth,
Dscho

Changing the order of arguments makes no difference, same result either way.

Mark

Hi,

We don't do thin packs. Should we? I guess that

	$ git ls-tree -r HEAD | wc

results in something close to 1500 in that repo. Which basically means that the 1531 objects (including trees and the commit) sounds correct.

Of course, we _could_ make the packs thin, but the disadvantage would be that we can never decide at a later stage to allow shallow fetches from that bundle.

The advantage of the thin packs would be that the bundles would be much smaller.

Ciao,
Dscho

We should do thin packs, but 1500+ objects is a different story.

	rev-list --objects <revspec>

means "show me the revs and its objects".

	rev-list --max-count=<n> <revspec>

means "show me the revs and its objects, but stop at <n> revs".

On the other hand,

	rev-list --objects ^<rev1> <rev2>

means something totally different. "never show me what is reachable from <rev1> but I am interested to see what are reachable from <rev2>".

So it is natural that "rev-list --objects --max-count=1 HEAD" shows the same set of tree and blob objects as "git-tar HEAD". "show me the revs and its objects" does not say "but exclude trees and blobs reachable from commits beyond the <n> revs boundary," which is "rev-list --objects HEAD^..HEAD".

In other words, "rev-list -1 HEAD" and "rev-list HEAD^..HEAD" are _different_.

We fixed (in 'next') the revision traversal so that --max-count and --boundary interact correctly. What's not fixed is that we still use "rev-list --objects --max-*" given by the user, which is not the semantics git-bundle wants. We should rewrite the revspec to drive pack-objects when generating the packfile part of the bundle to do "<revs> --not <boundaries>" internally in git-bundle.

When saying something like "--since=1.day.ago" or "--max-count=5", git-bundle finds the boundary commits which are recorded as prerequisites. However, it failed to tell pack-objects _not_ to pack the objects which are in these.

Fix that. And add a test for that.

---

On Wed, 7 Mar 2007, Junio C Hamano wrote:

> What's not fixed is that we still use "rev-list --objects > --max-*" given by the user, which is not the semantics > git-bundle wants. We should rewrite the revspec to drive > pack-objects when generating the packfile part of the bundle to > do "<revs> --not <boundaries>" internally in git-bundle.

Since we already calculate the prerequisites, it's easy to pass them as uninteresting, too. It does not hurt to keep the max* limiters, so they are not unset.

 builtin-bundle.c |   15 ++++++++++++---
 t/t5510-fetch.sh |   11 +++++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)

index 3b3bc25..70d4479 100644
@@ -305,6 +305,10 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	/* write signature */
 	write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
 
+	/* init revs to list objects for pack-objects later */
+	save_commit_buffer = 0;
+	init_revisions(&revs, NULL);
+
 	/* write prerequisites */
 	memcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *));
 	argv_boundary[0] = "rev-list";

@@ -316,8 +320,15 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	if (pid < 0)
 		return -1;
 	while ((i = read_string(out, buffer, sizeof(buffer))) > 0)
-		if (buffer[0] == '-')
+		if (buffer[0] == '-') {
+			unsigned char sha1[20];
 			write_or_die(bundle_fd, buffer, i);
+			if (!get_sha1_hex(buffer + 1, sha1)) {
+				struct object *object = parse_object(sha1);
+				object->flags |= UNINTERESTING;
+				add_pending_object(&revs, object, buffer);
+			}
+		}
 	while ((i = waitpid(pid, &status, 0)) < 0)
 		if (errno != EINTR)
 			return error("rev-list died");

@@ -325,8 +336,6 @@ static int create_bundle(struct bundle_header *header, const char *path,
 		return error("rev-list died %d", WEXITSTATUS(status));
 
 	/* write references */
-	save_commit_buffer = 0;
-	init_revisions(&revs, NULL);
 	revs.tag_objects = 1;
 	revs.tree_objects = 1;
 	revs.blob_objects = 1;

index ce96b4b..7be9793 100755
@@ -121,4 +121,15 @@ test_expect_success 'unbundle 2' '
 	test "tip" = "$(git log -1 --pretty=oneline master | cut -b42-)"
 '
 
+test_expect_success 'bundle does not prerequisite objects' '
+	cd "$D" &&
+	touch file2 &&
+	git add file2 &&
+	git commit -m add.file2 file2 &&
+	git bundle create bundle3 -1 HEAD &&
+	sed "1,4d" < bundle3 > bundle.pack &&
+	git index-pack bundle.pack &&
+	test 4 = $(git verify-pack -v bundle.pack | wc -l)
+'
+
 test_done

1.5.0.3.2562.gfcfe0

Johannes Schindelin wrote:

Using next (2432cbc253e8ef5cf09062abb8f7325813476d1d) that has your patches for creating a thin-pack and limiting objects, I generally find the bundle file created cannot be used.

Specific example you can repeat in a current git repo:

git>git branch foo 2432cbc253e8ef5c
git>git bundle create t.bdl foo~1..foo
Generating pack...
Done counting 20 objects.
Result has 14 objects.
Deltifying 14 objects.
100% (14/14) done
Writing 14 objects.
100% (14/14) done
Total 14 (delta 10), reused 10 (delta 6)
git>git fetch t.bdl
fatal: pack has junk at the end
error: index-pack exited with status 128
fatal: Fetch failure: t.bdl
git>

Also, we get an interesting failure if the references are not touched in a given date range:
git>git bundle create t.bdl --since=5.minutes.ago foo
warning: ref 'foo' is excluded by the rev-list options
Generating pack...
Done counting 38321 objects.
Deltifying 38321 objects.
100% (38321/38321) done
Writing 38321 objects.
100% (38321/38321) done
Total 38321 (delta 26678), reused 38180 (delta 26551)
git>

No references requies 38000+ objects?? In this case, the bundle includes every object in the repository, the exact opposite of what should happen (zero objects).

Mark

Since a bundle doesn't make any sense *anyway* unless you have the prerequisites at the other end, I think you might as well do thin packs. That will cut down on the bundle size a *lot* for the common cases.

Linus

Thin packs are way smaller, but they rely on the receiving end to have the base objects. However, Git's pack protocol also uses thin packs by default. So make the packs contained in bundles thin, since bundles are just another transport.

The patch looks a bit bigger than intended, mainly because --thin _implies_ that pack-objects should run its own rev-list. Therefore, this patch removes all the stuff we used to roll rev-list ourselves.

This commit also changes behaviour slightly: since we now know early enough if a specified ref is _not_ contained in the pack, we can avoid putting that ref into the pack. So, we don't die() here, but warn() instead, and skip that ref.

---

It came to me as a surprise that --thin implies --revs, and I have to admit that I still don't understand it.

Anyway, this commit deletes more than double the lines as it adds, so I'm not complaining.

On Wed, 7 Mar 2007, Linus Torvalds wrote:

> On Wed, 7 Mar 2007, Johannes Schindelin wrote:
>
> > We don't do thin packs. Should we?
>
> Since a bundle doesn't make any sense *anyway* unless you have > the prerequisites at the other end, I think you might as well do > thin packs. That will cut down on the bundle size a *lot* for > the common cases.

Well, I disagree on the blanket "*anyway*". Shallow fetches are no longer possible from these bundles (at least after this commit _and_ ":/git-bundle: avoid packing" which I just sent out).

But then, people who want shallow fetches via bundles are more likely to want tar balls _anyway_.

 builtin-bundle.c |   85 ++++++++++++++++-------------------------------------
 1 files changed, 26 insertions(+), 59 deletions(-)

index 70d4479..6163358 100644
@@ -257,45 +257,15 @@ static int list_heads(struct bundle_header *header, int argc, const char **argv)
 	return list_refs(&header->references, argc, argv);
 }
 
-static void show_commit(struct commit *commit)
-{
-	write_or_die(1, sha1_to_hex(commit->object.sha1), 40);
-	write_or_die(1, "\n", 1);
-	if (commit->parents) {
-		free_commit_list(commit->parents);
-		commit->parents = NULL;
-	}
-}
-
-static void show_object(struct object_array_entry *p)
-{
-	/* An object with name "foo\n0000000..." can be used to
-	 * confuse downstream git-pack-objects very badly.
-	 */
-	const char *ep = strchr(p->name, '\n');
-	int len = ep ? ep - p->name : strlen(p->name);
-	write_or_die(1, sha1_to_hex(p->item->sha1), 40);
-	write_or_die(1, " ", 1);
-	if (len)
-		write_or_die(1, p->name, len);
-	write_or_die(1, "\n", 1);
-}
-
-static void show_edge(struct commit *commit)
-{
-	; /* nothing to do */
-}
-
 static int create_bundle(struct bundle_header *header, const char *path,
 		int argc, const char **argv)
 {
 	int bundle_fd = -1;
 	const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
-	const char **argv_pack = xmalloc(4 * sizeof(const char *));
+	const char **argv_pack = xmalloc(5 * sizeof(const char *));
 	int pid, in, out, i, status;
 	char buffer[1024];
 	struct rev_info revs;
-	struct object_array tips;
 
 	bundle_fd = (!strcmp(path, "-") ? 1 :
 			open(path, O_CREAT | O_WRONLY, 0666));

@@ -319,16 +289,20 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	pid = fork_with_pipe(argv_boundary, NULL, &out);
 	if (pid < 0)
 		return -1;
-	while ((i = read_string(out, buffer, sizeof(buffer))) > 0)
+	while ((i = read_string(out, buffer, sizeof(buffer))) > 0) {
+		unsigned char sha1[20];
 		if (buffer[0] == '-') {
-			unsigned char sha1[20];
 			write_or_die(bundle_fd, buffer, i);
 			if (!get_sha1_hex(buffer + 1, sha1)) {
 				struct object *object = parse_object(sha1);
 				object->flags |= UNINTERESTING;
 				add_pending_object(&revs, object, buffer);
 			}
+		} else if (!get_sha1_hex(buffer, sha1)) {
+			struct object *object = parse_object(sha1);
+			object->flags |= SHOWN;
 		}
+	}
 	while ((i = waitpid(pid, &status, 0)) < 0)
 		if (errno != EINTR)
 			return error("rev-list died");

@@ -336,14 +310,10 @@ static int create_bundle(struct bundle_header *header, const char *path,
 		return error("rev-list died %d", WEXITSTATUS(status));
 
 	/* write references */
-	revs.tag_objects = 1;
-	revs.tree_objects = 1;
-	revs.blob_objects = 1;
 	argc = setup_revisions(argc, argv, &revs, NULL);
 	if (argc > 1)
 		return error("unrecognized argument: %s'", argv[1]);
 
-	memset(&tips, 0, sizeof(tips));
 	for (i = 0; i < revs.pending.nr; i++) {
 		struct object_array_entry *e = revs.pending.objects + i;
 		unsigned char sha1[20];

@@ -353,11 +323,20 @@ static int create_bundle(struct bundle_header *header, const char *path,
 			continue;
 		if (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)
 			continue;
+		/*
+		 * Make sure the refs we wrote out is correct; --max-count and
+		 * other limiting options could have prevented all the tips
+		 * from getting output.
+		 */
+		if (!(e->item->flags & SHOWN)) {
+			warn("ref '%s' is excluded by the rev-list options",
+				e->name);
+			continue;
+		}
 		write_or_die(bundle_fd, sha1_to_hex(e->item->sha1), 40);
 		write_or_die(bundle_fd, " ", 1);
 		write_or_die(bundle_fd, ref, strlen(ref));
 		write_or_die(bundle_fd, "\n", 1);
-		add_object_array(e->item, e->name, &tips);
 		free(ref);
 	}
 

@@ -368,39 +347,27 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	argv_pack[0] = "pack-objects";
 	argv_pack[1] = "--all-progress";
 	argv_pack[2] = "--stdout";
-	argv_pack[3] = NULL;
+	argv_pack[3] = "--thin";
+	argv_pack[4] = NULL;
 	in = -1;
 	out = bundle_fd;
 	pid = fork_with_pipe(argv_pack, &in, &out);
 	if (pid < 0)
 		return error("Could not spawn pack-objects");
-	close(1);
-	dup2(in, 1);
+	for (i = 0; i < revs.pending.nr; i++) {
+		struct object *object = revs.pending.objects[i].item;
+		if (object->flags & UNINTERESTING)
+			write(in, "^", 1);
+		write(in, sha1_to_hex(object->sha1), 40);
+		write(in, "\n", 1);
+	}
 	close(in);
-	prepare_revision_walk(&revs);
-	mark_edges_uninteresting(revs.commits, &revs, show_edge);
-	traverse_commit_list(&revs, show_commit, show_object);
-	close(1);
 	while (waitpid(pid, &status, 0) < 0)
 		if (errno != EINTR)
 			return -1;
 	if (!WIFEXITED(status) || WEXITSTATUS(status))
 		return error ("pack-objects died");
 
-	/*
-	 * Make sure the refs we wrote out is correct; --max-count and
-	 * other limiting options could have prevented all the tips
-	 * from getting output.
-	 */
-	status = 0;
-	for (i = 0; i < tips.nr; i++) {
-		if (!(tips.objects[i].item->flags & SHOWN)) {
-			status = 1;
-			error("%s: not included in the resulting pack",
-			      tips.objects[i].name);
-		}
-	}
-
 	return status;
 }
 

1.5.0.3.2562.gfcfe0

I am a bit worried about that part.

If somebody says "bundle create foo.bdl --since=1.day maint" and maint's tip hasn't changed for a month, we would end up having no refs and no pack in the bundle with just a warning.

"bundle create foo.bdl -20 master next" does _not_ mean "20 revs but at least have master and next tip", but it may surprise an uninitiated to find out that running "bundle list foo.bdl" on the resulting bundle did not talk about master at all.

I have a feeling that it would avoid confusion if we did not even start the pack generation and die early when we find the counting caused us not to include all the positive refs specified on the command line.

Hi,

Fair enough.

Ciao,
Dscho

Does anybody actually use shallow clones in real life?

When I did the numbers a long time ago, the shallow clone didn't actually help much, because it meant that there were no deltas. Which meant that you got 1% of the history for 60% of the price of all history, and the shallow thing didn't really seem to make much sense.

I guess that for something with a really long history, you'd get 0.001% of the history for 10% of the price, and maybe it makes sense then.

Linus

Hi,

I don't. That's why I work on push/fetch from/via/into shallow repos.

You -- being blessed by not having to work with anything closed -- miss an important fact of commercial software development. Most, if not all, projects in a commercial setting contain binary blobs. For example, a DLL, or a PNG, or a Firmware blob. These are updated regularly. And they delta really awfully bad.

Also, for something as OpenOffice or Mozilla, I guess that if you really only want to work on the newest revision, the _initial_ fetch will be way cheaper than a full clone. Of course I have no numbers here, but that is what my gut feeling says.

Naturally, over time, the shallow clone will fetch in more and more objects from the upstream, eventually being almost as large as if having the complete history, but the user's experience is different: the amount of time needed to get that amount of data is much more widely spread.

Ciao,
Dscho

That is probably true but I do not think we have ever had date limiter and --objects interact correctly. It is understandable as traditionally packs were to generate needed objects between revisions, and unlimited but counted (--max-count) or timed (--max-age) are unexpected uses for "rev-list --objects" (in other words, nobody had done so and I am not surprised it does not work as you expect -- rather I would be surprised if anybody has even expectd the current code to work that way without modification).

I am a bit too tired tonight so anybody is welcome to beat me to fix this one, preferrably on top of 'next'.

On 3/6/07, *Junio C Hamano* <[email protected] <mailto:[email protected]>> wrote:

* I'll have this in 'master' tonight. Also I am thinking about changing 'git bundle verify' to spit out heads and prereqs unconditionally, in addition to 'foo.bdl is ok'. Comments?

That is a good feature, much better than trying to list the bundle, and I have no issue with this suggestion.

We could also define "git bundle [depends|show-refs] bundle-file" to be more suggestive of the actual result. As the prerequisites have a common conceptual definition with those of a shallow clone, perhaps there should be a common way to get those. In this vein, git-ls-remote bundle already shows the references in git-like fashion.

Mark

Indeed. The internal rev-list reimplemented in builtin-bundle.c lacks the code to mark trees and blobs in commits that are on the other side of the edge uninteresting.

This is on top of the patches I send to the list and Dscho earlier.

--

 builtin-bundle.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

index 4fe74a7..199a30d 100644
@@ -278,6 +278,11 @@ static void show_object(struct object_array_entry *p)
 	write_or_die(1, "\n", 1);
 }
 
+static void show_edge(struct commit *commit)
+{
+	; /* nothing to do */
+}
+
 static int create_bundle(struct bundle_header *header, const char *path,
 		int argc, const char **argv)
 {

@@ -361,6 +366,7 @@ static int create_bundle(struct bundle_header *header, const char *path,
 	dup2(in, 1);
 	close(in);
 	prepare_revision_walk(&revs);
+	mark_edges_uninteresting(revs.commits, &revs, show_edge);
 	traverse_commit_list(&revs, show_commit, show_object);
 	close(1);
 	while (waitpid(pid, &status, 0) < 0)