[PATCH] git-bundle: fix pack generation.

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.

Signed-off-by: Junio C Hamano <redacted>
---

 * 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(-)

diff --git a/builtin-bundle.c b/builtin-bundle.c
index d41a413..279b8f8 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c

@@ -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

Junio C Hamano wrote:

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.

Signed-off-by: Junio C Hamano <redacted>
  

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

On 3/6/07, *Junio C Hamano* <junkio@cox.net <mailto:junkio@cox.net>> 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

Mark Levedahl [off-list ref] writes:

Junio C Hamano wrote:

quoted

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.

Signed-off-by: Junio C Hamano <redacted>

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.

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?

Junio C Hamano wrote:

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,

On Tue, 6 Mar 2007, Mark Levedahl wrote:

Junio C Hamano wrote:

quoted

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.

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

Johannes Schindelin wrote:

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,

On Tue, 6 Mar 2007, Mark Levedahl wrote:

Yes, setting GIT_EXEC_PATH fixed the problem, thanks.

Wonderful!

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

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

On 3/6/07, Johannes Schindelin [off-list ref] wrote:

Hi,

quoted

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

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.

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

Mark

Hi,

On Wed, 7 Mar 2007, Mark Levedahl wrote:

On 3/6/07, Johannes Schindelin [off-list ref] wrote:

quoted

quoted

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

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.

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

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

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.

		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.

Signed-off-by: Johannes Schindelin <redacted>
---

	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(-)

diff --git a/builtin-bundle.c b/builtin-bundle.c
index 70d4479..6163358 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c

@@ -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

On Thu, 8 Mar 2007, Johannes Schindelin wrote:

	> 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).

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,

On Wed, 7 Mar 2007, Linus Torvalds wrote:

Does anybody actually use shallow clones in real life?

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

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.

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