Re: [RFC] Build in clone

[Johan Herland <johan@herland.net>]

On Monday 25 February 2008, Daniel Barkalow wrote:
> This version is still a mess, but it passes all of the tests.

Not for me:
*** t5700-clone-reference.sh ***
*   ok 1: preparing first repository
*   ok 2: preparing second repository
* FAIL 3: cloning with reference (-l -s)
        git clone -l -s --reference B A C
*   ok 4: existence of info/alternates
*   ok 5: pulling from reference
*   ok 6: that reference gets used
* FAIL 7: cloning with reference (no -l -s)
        git clone --reference B file://`pwd`/A D
*   ok 8: existence of info/alternates
*   ok 9: pulling from reference
*   ok 10: that reference gets used
*   ok 11: updating origin
*   ok 12: pulling changes from origin
*   ok 13: that alternate to origin gets used
*   ok 14: pulling changes from origin
*   ok 15: check objects expected to exist locally
* failed 2 among 15 test(s)
make[1]: *** [t5700-clone-reference.sh] Error 1

> I'm somewhat unconvinced by the test ccoverage for clone, however; the
> last failure I found was actually for which heads get created in a
> bare repository, and it was only failing when there was an extra one
> in a non-bare clone in a test for something entirely different.
> 
> This is largely based on Kristian Høgsberg's version from December, but 
> the introduced warnings and two whitespace errors I haven't located are 
> mine.
> 
> I'm still working on getting it cleaned up, but I thought it would be good 
> to get it some exposure and testing, since people have been talking about 
> builtin-clone today.

Other than the failing tests, it seems to work fairly well. I've been
playing around with it for a few minutes, and on a test repo I have with
1001 branches and 10000 tags, it cuts down the runtime of a local git-clone
from 25 seconds to ~1.5 seconds. (simply by eliminating the overhead of
invoking git-update-ref for every single ref) :)

I've tried to test this by diffing a cloned repo against an equivalent
clone done by the old script. Below I pasted in a few immediate fixes I
found. With these fixes, the only remaining diff between the clones is
that refs/remotes/origin/HEAD used to be a symbolic ref (with no reflog),
but is now a "regular" ref (with reflog).
The fixes are, in order of importance:
- Call git_config(git_default_config) in order to properly set up
  user.name and user.email for reflogs (This BREAKS test #9 in
  t1020-subdirectory.sh. Have yet to figure out why)
- Fix "clone from $repo" reflog messages (using strbufs; something tells
  me more of this code would benefit from using strbufs)
- Høgsberg's name should be in UTF-8 (not sure if this will survive this
  mail)
- The two whitespace errors you mentioned

I'm sorry that my patch below sucks from a style POV. Feel free to ignore.
Will redo when it's not in the middle of the night.


Have fun! :)

...Johan

-8<----------------8<---------------------8<-
[PATCH] WIP: Minor fixes on top of builtin-clone

Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin-clone.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 5aa75e1..7eed340 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -1,7 +1,7 @@
 /*
  * Builtin "git clone"
  *
- * Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
+ * Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
  * Based on git-commit.sh by Junio C Hamano and Linus Torvalds
  *
  * Clone a repository into a different directory that does not yet exist.
@@ -79,7 +79,7 @@ static char *get_repo_path(const char *repo)
 
 	if (!stat(repo, &buf) && S_ISDIR(buf.st_mode))
 		return xstrdup(make_absolute_path(repo));
-	
+
 	return NULL;
 }
 
@@ -347,6 +347,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	char *path, *dir, *head, *ref_temp;
 	struct ref *refs, *r, *remote_head, *head_points_at, *remote_master;
 	char branch_top[256], key[256], refname[256], value[256];
+	struct strbuf reflog_msg;
+
+	git_config(git_default_config);
 
 	clone_pid = getpid();
 
@@ -459,6 +462,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		snprintf(branch_top, sizeof branch_top,
 			 "refs/remotes/%s", option_origin);
 
+	strbuf_init(&reflog_msg, strlen(repo) + 12);
+	strbuf_addf(&reflog_msg, "clone: from %s", repo);
+
 	printf("%p\n", refs);
 	remote_head = NULL;
 	remote_master = NULL;
@@ -487,7 +493,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 				continue;
 		}
 
-		update_ref("clone from $repo",
+		update_ref(reflog_msg.buf,
 			   refname, r->old_sha1, NULL, 0, DIE_ON_ERR);
 	}
 
@@ -495,7 +501,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (!remote_head) {
 		/* If there isn't one, oh well. */
 	} else if (remote_master && !hashcmp(remote_master->old_sha1,
-				      remote_head->old_sha1)) {
+					     remote_head->old_sha1)) {
 		/* If refs/heads/master could be right, it is. */
 		head_points_at = remote_master;
 	} else
@@ -552,7 +558,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		git_config_set(key, head_points_at->name);
 	} else if (remote_head) {
 		/* Source had detached HEAD pointing somewhere. */
-		update_ref("clone from $repo", "HEAD", remote_head->old_sha1,
+		update_ref(reflog_msg.buf, "HEAD", remote_head->old_sha1,
 			   NULL, REF_NODEREF, DIE_ON_ERR);
 	} else {
 		/* Nothing to checkout out */
@@ -591,7 +597,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		    commit_locked_index(&lock_file))
 			die("unable to write new index file");
 	}
-	
+
+	strbuf_release(&reflog_msg);
 	junk_work_tree = NULL;
 	junk_git_dir = NULL;
 	return 0;
-- 
1.5.4.3.328.gcaed