[RFC] Build in clone

[RFC] Build in clone

[Daniel Barkalow]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 27489 bytes --]

This version is still a mess, but it passes all of the tests. 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.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 Makefile                                      |    2 +-
 builtin-clone.c                               |  598 +++++++++++++++++++++++++
 builtin-init-db.c                             |  163 ++++---
 builtin.h                                     |    1 +
 cache.h                                       |    5 +
 git-clone.sh => contrib/examples/git-clone.sh |    0 
 environment.c                                 |    6 +
 git.c                                         |    1 +
 8 files changed, 704 insertions(+), 72 deletions(-)
 create mode 100644 builtin-clone.c
 rename git-clone.sh => contrib/examples/git-clone.sh (100%)

diff --git a/Makefile b/Makefile
index 149343c..c56d9da 100644
--- a/Makefile
+++ b/Makefile
@@ -227,7 +227,6 @@ BASIC_LDFLAGS =
 
 SCRIPT_SH = \
 	git-bisect.sh \
-	git-clone.sh \
 	git-merge-one-file.sh git-mergetool.sh git-parse-remote.sh \
 	git-pull.sh git-rebase.sh git-rebase--interactive.sh \
 	git-repack.sh git-request-pull.sh \
@@ -344,6 +343,7 @@ BUILTIN_OBJS = \
 	builtin-checkout-index.o \
 	builtin-check-ref-format.o \
 	builtin-clean.o \
+	builtin-clone.o \
 	builtin-commit.o \
 	builtin-commit-tree.o \
 	builtin-count-objects.o \
diff --git a/builtin-clone.c b/builtin-clone.c
new file mode 100644
index 0000000..da278f9
--- /dev/null
+++ b/builtin-clone.c
@@ -0,0 +1,598 @@
+/*
+ * Builtin "git clone"
+ *
+ * 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.
+ */
+
+#include "cache.h"
+#include "parse-options.h"
+#include "fetch-pack.h"
+#include "refs.h"
+#include "tree.h"
+#include "tree-walk.h"
+#include "unpack-trees.h"
+#include "transport.h"
+#include "strbuf.h"
+#include "dir.h"
+
+/*
+ * Overall FIXMEs:
+ *  - respect DB_ENVIRONMENT for .git/objects.
+ *  - error path cleanup of dirs+files.
+ *
+ * Implementation notes:
+ *  - dropping use-separate-remote and no-separate-remote compatibility
+ *
+ */
+static const char * const builtin_clone_usage[] = {
+	"git-clone [options] [--] <repo> [<dir>]",
+	NULL
+};
+
+static int option_quiet, option_no_checkout, option_bare;
+static int option_local, option_no_hardlinks, option_shared;
+static char *option_template, *option_reference, *option_depth;
+static char *option_origin = NULL;
+static char *option_upload_pack = "git-upload-pack";
+
+static struct option builtin_clone_options[] = {
+	OPT__QUIET(&option_quiet),
+	OPT_BOOLEAN('n', "no-checkout", &option_no_checkout,
+		    "don't create a checkout"),
+	OPT_BOOLEAN(0, "bare", &option_bare, "create a bare repository"),
+	OPT_BOOLEAN(0, "naked", &option_bare, "create a bare repository"),
+	OPT_BOOLEAN('l', "local", &option_local,
+		    "to clone from a local repository"),
+	OPT_BOOLEAN(0, "no-hardlinks", &option_no_hardlinks,
+		    "don't use local hardlinks, always copy"),
+	OPT_BOOLEAN('s', "shared", &option_shared,
+		    "setup as shared repository"),
+	OPT_STRING(0, "template", &option_template, "path",
+		   "path the template repository"),
+	OPT_STRING(0, "reference", &option_reference, "repo",
+		   "reference repository"),
+	OPT_STRING('o', "origin", &option_origin, "branch",
+		   "use <branch> instead or 'origin' to track upstream"),
+	OPT_STRING('u', "upload-pack", &option_upload_pack, "path",
+		   "path to git-upload-pack on the remote"),
+	OPT_STRING(0, "depth", &option_depth, "depth",
+		    "create a shallow clone of that depth"),
+
+	OPT_END()
+};
+
+static char *get_repo_path(const char *repo)
+{
+	const char *path;
+	struct stat buf;
+
+	path = mkpath("%s/.git", repo);
+	if (!stat(path, &buf) && S_ISDIR(buf.st_mode))
+		return xstrdup(make_absolute_path(path));
+
+	path = mkpath("%s.git", repo);
+	if (!stat(path, &buf) && S_ISDIR(buf.st_mode))
+		return xstrdup(make_absolute_path(path));
+
+	if (!stat(repo, &buf) && S_ISDIR(buf.st_mode))
+		return xstrdup(make_absolute_path(repo));
+	
+	return NULL;
+}
+
+static char *guess_dir_name(const char *repo)
+{
+	const char *p, *start, *end, *limit;
+	int after_slash_or_colon;
+
+	/* Guess dir name from repository: strip trailing '/',
+	 * strip trailing '[:/]*git', strip leading '.*[/:]'. */
+
+	after_slash_or_colon = 1;
+	limit = repo + strlen(repo);
+	start = repo;
+	end = limit;
+	for (p = repo; p < limit; p++) {
+		if (!prefixcmp(p, ".git")) {
+			if (!after_slash_or_colon)
+				end = p;
+			p += 3;
+		} else if (*p == '/' || *p == ':') {
+			if (end == limit)
+				end = p;
+			after_slash_or_colon = 1;
+		} else if (after_slash_or_colon) {
+			start = p;
+			end = limit;
+			after_slash_or_colon = 0;
+		}
+	}
+
+	return xstrndup(start, end - start);
+}
+
+static void
+write_alternates_file(const char *repo, const char *reference)
+{
+	char *file;
+	char *alternates;
+	int fd;
+
+	file = mkpath("%s/objects/info/alternates", repo);
+	fd = open(file, O_CREAT | O_WRONLY | O_APPEND, 0666);
+	if (fd < 0)
+		die("failed to create %s", file);
+	alternates = mkpath("%s/objects\n", reference);
+	write_or_die(fd, alternates, strlen(alternates));
+	if (close(fd))
+		die("could not close %s", file);
+	fprintf(stderr, "Wrote %s to %s\n", alternates, file);
+}
+
+static int
+setup_tmp_ref(const char *refname,
+	      const unsigned char *sha1, int flags, void *cb_data)
+{
+	const char *ref_temp = cb_data;
+	char *path;
+	struct lock_file lk;
+	struct ref_lock *rl;
+
+	/*
+
+	echo "$ref_git/objects" >"$GIT_DIR/objects/info/alternates"
+	(
+		GIT_DIR="$ref_git" git for-each-ref \
+			--format='%(objectname) %(*objectname)'
+	) |
+	while read a b
+	do
+		test -z "$a" ||
+		git update-ref "refs/reference-tmp/$a" "$a"
+		test -z "$b" ||
+		git update-ref "refs/reference-tmp/$b" "$b"
+	done
+
+	*/
+
+	/* We go a bit out of way to use write_ref_sha1() here.  We
+	 * could just write the ref file directly, since neither
+	 * locking or reflog really matters here.  However, let's use
+	 * the standard interface for writing refs as much as is
+	 * possible given that get_git_dir() != the repo we're writing
+	 * the refs in. */
+
+	printf("%s -> %s/%s\n",
+	       sha1_to_hex(sha1), ref_temp, sha1_to_hex(sha1));
+
+	path = mkpath("%s/%s", ref_temp, sha1_to_hex(sha1));
+	rl = xmalloc(sizeof *rl);
+	rl->force_write = 1;
+	rl->lk = &lk;
+	rl->ref_name = xstrdup(sha1_to_hex(sha1));
+	rl->orig_ref_name = xstrdup(rl->ref_name);
+	rl->lock_fd = hold_lock_file_for_update(rl->lk, path, 1);
+	if (write_ref_sha1(rl, sha1, NULL) < 0)
+		die("failed to write temporary ref %s", lk.filename);
+
+	return 0;
+}
+
+static char *
+setup_reference(const char *repo)
+{
+	struct stat buf;
+	const char *ref_git;
+	char *ref_temp;
+
+	if (!option_reference)
+		return NULL;
+
+	ref_git = make_absolute_path(option_reference);
+
+	if (!stat(mkpath("%s/.git/objects", ref_git), &buf) &&
+	    S_ISDIR(buf.st_mode))
+		ref_git = mkpath("%s/.git", ref_git);
+	else if (stat(mkpath("%s/objects", ref_git), &buf) ||
+		 !S_ISDIR(buf.st_mode))
+		die("reference repository '%s' is not a local directory.",
+		    option_reference);
+
+	set_git_dir(ref_git);
+
+	write_alternates_file(repo, ref_git);
+
+	ref_temp = xstrdup(mkpath("%s/refs/reference-tmp", repo));
+	if (mkdir(ref_temp, 0777))
+		die("could not create directory %s", ref_temp);
+	for_each_ref(setup_tmp_ref, (void *) ref_temp);
+
+	return ref_temp;
+}
+
+static void
+cleanup_reference(char *ref_temp)
+{
+	struct dirent *de;
+	DIR *dir;
+
+	if (!ref_temp)
+		return;
+	dir = opendir(ref_temp);
+	if (!dir) {
+		if (errno == ENOENT)
+			return;
+		die("failed to open directory %s", ref_temp);
+	}
+
+	while ((de = readdir(dir)) != NULL) {
+		if (de->d_name[0] == '.')
+			continue;
+		unlink(mkpath("%s/%s", ref_temp, de->d_name));
+	}
+
+	unlink(ref_temp);
+	free(ref_temp);
+}
+
+static void
+walk_objects(char *src, char *dest)
+{
+	struct dirent *de;
+	struct stat buf;
+	int src_len, dest_len;
+	DIR *dir;
+
+	dir = opendir(src);
+	if (!dir)
+		die("failed to open %s\n", src);
+
+	if (mkdir(dest, 0777)) {
+		if (errno != EEXIST)
+			die("failed to create directory %s\n", dest);
+		else if (stat(dest, &buf))
+			die("failed to stat %s\n", dest);
+		else if (!S_ISDIR(buf.st_mode))
+			die("%s exists and is not a directory\n", dest);
+	}
+
+	src_len = strlen(src);
+	src[src_len] = '/';
+	dest_len = strlen(dest);
+	dest[dest_len] = '/';
+
+	while ((de = readdir(dir)) != NULL) {
+		strcpy(src + src_len + 1, de->d_name);
+		strcpy(dest + dest_len + 1, de->d_name);
+		if (stat(src, &buf)) {
+			fprintf(stderr, "failed to stat %s, ignoring\n", src);
+			continue;
+		}
+		if (S_ISDIR(buf.st_mode)) {
+			if (de->d_name[0] != '.')
+				walk_objects(src, dest);
+			continue;
+		}
+
+		if (unlink(dest) && errno != ENOENT)
+			die("failed to unlink %s\n", dest);
+		if (option_no_hardlinks) {
+			if (copy_file(dest, src, 0666))
+				die("failed to copy file to %s\n", dest);
+		} else {
+			if (link(src, dest))
+				die("failed to create link %s\n", dest);
+		}
+	}
+}
+
+static const struct ref *
+clone_local(const char *src_repo, const char *dest_repo)
+{
+	const struct ref *ret;
+	char src[PATH_MAX];
+	char dest[PATH_MAX];
+	struct remote *remote;
+	struct transport *transport;
+
+	if (option_shared) {
+		write_alternates_file(dest_repo, src_repo);
+	} else {
+		snprintf(src, PATH_MAX, "%s/objects", src_repo);
+		snprintf(dest, PATH_MAX, "%s/objects", dest_repo);
+		walk_objects(src, dest);
+	}
+
+	fprintf(stderr, "Get for %s\n", src_repo);
+	remote = remote_get(src_repo);
+	transport = transport_get(remote, src_repo);
+	ret = transport_get_remote_refs(transport);
+	transport_disconnect(transport);
+	return ret;
+}
+
+static const char *junk_work_tree;
+static const char *junk_git_dir;
+pid_t clone_pid;
+
+static void remove_junk(void)
+{
+	struct strbuf sb;
+	if (getpid() != clone_pid)
+		return;
+	strbuf_init(&sb, 0);
+	if (junk_git_dir) {
+		fprintf(stderr, "Remove junk %s\n", junk_git_dir);
+		strbuf_addstr(&sb, junk_git_dir);
+		remove_dir_recursively(&sb, 0);
+		strbuf_reset(&sb);
+	}
+	if (junk_work_tree) {
+		fprintf(stderr, "Remove junk %s\n", junk_work_tree);
+		strbuf_addstr(&sb, junk_work_tree);
+		remove_dir_recursively(&sb, 0);
+		strbuf_reset(&sb);
+	}
+}
+
+int cmd_clone(int argc, const char **argv, const char *prefix)
+{
+	int use_local_hardlinks = 1;
+	int use_separate_remote = 1;
+	struct stat buf;
+	const char *repo, *work_tree, *git_dir;
+	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];
+
+	clone_pid = getpid();
+
+	argc = parse_options(argc, argv, builtin_clone_options,
+			     builtin_clone_usage, 0);
+
+	if (argc == 0)
+		die("You must specify a repository to clone.");
+
+	if (option_no_hardlinks)
+		use_local_hardlinks = 0;
+
+	if (option_bare) {
+		if (option_origin)
+			die("--bare and --origin %s options are incompatible.",
+			    option_origin);
+		option_no_checkout = 1;
+		use_separate_remote = 0;
+	}
+
+	if (!option_origin)
+		option_origin = "origin";
+
+	repo = argv[0];
+	path = get_repo_path(repo);
+
+	if (argc == 2) {
+		dir = xstrdup(argv[1]);
+	} else {
+		dir = guess_dir_name(repo);
+	}
+
+	if (!stat(dir, &buf))
+		die("destination directory '%s' already exists.", dir);
+
+	if (option_bare)
+		work_tree = NULL;
+	else {
+		work_tree = getenv("GIT_WORK_TREE");
+		if (work_tree && !stat(work_tree, &buf))
+			die("working tree '%s' already exists.", work_tree);
+	}
+
+	atexit(remove_junk);
+
+	if (option_bare || work_tree)
+		git_dir = xstrdup(dir);
+	else {
+		work_tree = dir;
+		git_dir = xstrdup(mkpath("%s/.git", dir));
+	}
+
+	if (!option_bare) {
+		if (mkdir(work_tree, 0755))
+			die("could not create work tree dir '%s'.", work_tree);
+		set_git_work_tree(work_tree);
+		junk_work_tree = work_tree;
+	}
+
+	setenv(CONFIG_ENVIRONMENT, xstrdup(mkpath("%s/config", git_dir)), 1);
+
+	//set_git_dir(make_absolute_path(git_dir));
+
+	fprintf(stderr, "Initialize %s\n", git_dir);
+	init_db(git_dir, option_template, work_tree,
+		option_quiet ? INIT_DB_QUIET : 0);
+	junk_git_dir = git_dir;
+	fprintf(stderr, "Okay\n");
+
+	/* This calls set_git_dir for the reference repo so we can get
+	 * the refs there.  Thus, call this before calling
+	 * set_git_dir() on the repo we're setting up. */
+	ref_temp = setup_reference(git_dir);
+
+	set_git_dir(make_absolute_path(git_dir));
+
+	if (option_bare)
+		git_config_set("core.bare", "true");
+
+	if (path != NULL) {
+		refs = clone_local(path, git_dir);
+		repo = make_absolute_path(path);
+	} else {
+		struct remote *remote = remote_get(argv[0]);
+		struct transport *transport = transport_get(remote, argv[0]);
+		const struct ref *show;
+
+		transport_set_option(transport, TRANS_OPT_KEEP, "yes");
+
+		if (option_depth)
+			transport_set_option(transport, TRANS_OPT_DEPTH,
+					     option_depth);
+
+		if (option_quiet)
+			transport->verbose = -1;
+
+		//args.no_progress = 1;
+
+		fprintf(stderr, "Get refs for %s\n", argv[0]);
+		refs = transport_get_remote_refs(transport);
+
+		transport_fetch_refs(transport, refs);
+	}
+
+	cleanup_reference(ref_temp);
+
+	if (option_bare)
+		strcpy(branch_top, "refs/heads");
+	else
+		snprintf(branch_top, sizeof branch_top,
+			 "refs/remotes/%s", option_origin);
+
+	printf("%p\n", refs);
+	remote_head = NULL;
+	remote_master = NULL;
+	for (r = refs; r; r = r->next) {
+		fprintf(stderr, "%s\n",	r->name);
+		if (strlen(r->name) >= 3 &&
+		    !strcmp(r->name + strlen(r->name) - 3, "^{}"))
+			continue;
+		if (!strcmp(r->name, "HEAD")) {
+			remote_head = r;
+			if (option_bare)
+				continue;
+			snprintf(refname, sizeof refname,
+				 "%s/HEAD", branch_top);
+		} else {
+			if (!strcmp(r->name, "refs/heads/master"))
+				remote_master = r;
+
+			if (!prefixcmp(r->name, "refs/heads/"))
+				snprintf(refname, sizeof refname,
+					 "%s/%s", branch_top, r->name + 11);
+			else if (!prefixcmp(r->name, "refs/tags/"))
+				snprintf(refname, sizeof refname,
+					 "refs/tags/%s", r->name + 10);
+			else
+				continue;
+		}
+
+		update_ref("clone from $repo",
+			   refname, r->old_sha1, NULL, 0, DIE_ON_ERR);
+	}
+
+	head_points_at = NULL;
+	if (!remote_head) {
+		/* If there isn't one, oh well. */
+	} else if (remote_master && !hashcmp(remote_master->old_sha1,
+				      remote_head->old_sha1)) {
+		/* If refs/heads/master could be right, it is. */
+		head_points_at = remote_master;
+	} else
+		for (r = refs; r; r = r->next) {
+			if (r != remote_head &&
+			    !hashcmp(r->old_sha1, remote_head->old_sha1)) {
+				head_points_at = r;
+				printf("head points at %s\n", r->name);
+				break;
+			}
+		}
+
+	/* FIXME: What about the "Uh-oh, the remote told us..." case? */
+	if (!option_bare) {
+		snprintf(key, sizeof key, "remote.%s.url", option_origin);
+		git_config_set(key, repo);
+		snprintf(key, sizeof key, "remote.%s.fetch", option_origin);
+		snprintf(value, sizeof value, "+refs/heads/*:%s/*", branch_top);
+
+		git_config_set_multivar(key, value, "^$", 0);
+	}
+
+	if (option_bare) {
+		if (head_points_at) {
+			/* Local default branch */
+			create_symref("HEAD", head_points_at->name, NULL);
+		}
+		junk_work_tree = NULL;
+		junk_git_dir = NULL;
+		return 0;
+	}
+
+	if (head_points_at) {
+		if (strrchr(head_points_at->name, '/'))
+			head = strrchr(head_points_at->name, '/') + 1;
+		else
+			head = head_points_at->name;
+
+		/* Local default branch */
+		create_symref("HEAD", head_points_at->name, NULL);
+
+		/* Tracking branch for the primary branch at the remote. */
+		update_ref(NULL, "HEAD", head_points_at->old_sha1,
+			   NULL, 0, DIE_ON_ERR);
+	/*
+		rm -f "refs/remotes/$origin/HEAD"
+		git symbolic-ref "refs/remotes/$origin/HEAD" \
+			"refs/remotes/$origin/$head_points_at" &&
+	*/
+
+		snprintf(key, sizeof key, "branch.%s.remote", head);
+		git_config_set(key, option_origin);
+		snprintf(key, sizeof key, "branch.%s.merge", head);
+		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,
+			   NULL, REF_NODEREF, DIE_ON_ERR);
+	} else {
+		/* Nothing to checkout out */
+		if (!option_no_checkout)
+			fprintf(stderr, "Warning: Remote HEAD refers to nonexistent ref, unable to checkout.\n");
+		option_no_checkout = 1;
+	}
+
+	if (!option_no_checkout) {
+		struct lock_file lock_file;
+		struct unpack_trees_options opts;
+		struct tree *tree;
+		struct tree_desc t[2];
+		int fd;
+
+		/* We need to be in the new work tree for the checkout */
+		setup_work_tree();
+
+		fprintf(stderr, "work tree now %s\n", get_git_work_tree());
+
+		fd = hold_locked_index(&lock_file, 1);
+
+		memset(&opts, 0, sizeof opts);
+		opts.update = 1;
+		opts.verbose_update = !option_quiet;
+		opts.merge = 1;
+		opts.fn = twoway_merge;
+
+		tree = parse_tree_indirect(remote_head->old_sha1);
+		parse_tree(tree);
+		init_tree_desc(&t[0], tree->buffer, tree->size);
+		init_tree_desc(&t[1], tree->buffer, tree->size);
+		unpack_trees(2, t, &opts);
+
+		if (write_cache(fd, active_cache, active_nr) ||
+		    commit_locked_index(&lock_file))
+			die("unable to write new index file");
+	}
+	
+	junk_work_tree = NULL;
+	junk_git_dir = NULL;
+	return 0;
+}
diff --git a/builtin-init-db.c b/builtin-init-db.c
index 79eaf8d..bc74188 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -21,6 +21,7 @@ static void safe_create_dir(const char *dir, int share)
 {
 	if (mkdir(dir, 0777) < 0) {
 		if (errno != EEXIST) {
+			fprintf(stderr, "When creating GIT_DIR\n");
 			perror(dir);
 			exit(1);
 		}
@@ -176,6 +177,7 @@ static int create_default_files(const char *git_dir, const char *template_path)
 	if (len > sizeof(path)-50)
 		die("insane git directory %s", git_dir);
 	memcpy(path, git_dir, len);
+	git_dir = make_absolute_path(git_dir);
 
 	if (len && path[len-1] != '/')
 		path[len++] = '/';
@@ -250,8 +252,12 @@ static int create_default_files(const char *git_dir, const char *template_path)
 		/* allow template config file to override the default */
 		if (log_all_ref_updates == -1)
 		    git_config_set("core.logallrefupdates", "true");
-		if (work_tree != git_work_tree_cfg)
+		if (prefixcmp(git_dir, work_tree) ||
+		    strcmp(git_dir + strlen(work_tree), "/.git")) {
+			fprintf(stderr, "Is %s; would be %s\n", work_tree,
+				git_dir);
 			git_config_set("core.worktree", work_tree);
+		}
 	}
 
 	/* Check if symlink is supported in the work tree */
@@ -271,42 +277,93 @@ static int create_default_files(const char *git_dir, const char *template_path)
 	return reinit;
 }
 
-static void guess_repository_type(const char *git_dir)
+static int guess_repository_type(const char *git_dir)
 {
 	char cwd[PATH_MAX];
 	const char *slash;
 
-	if (0 <= is_bare_repository_cfg)
-		return;
-	if (!git_dir)
-		return;
-
 	/*
 	 * "GIT_DIR=. git init" is always bare.
 	 * "GIT_DIR=`pwd` git init" too.
 	 */
 	if (!strcmp(".", git_dir))
-		goto force_bare;
+		return 1;
 	if (!getcwd(cwd, sizeof(cwd)))
 		die("cannot tell cwd");
 	if (!strcmp(git_dir, cwd))
-		goto force_bare;
+		return 1;
 	/*
 	 * "GIT_DIR=.git or GIT_DIR=something/.git is usually not.
 	 */
 	if (!strcmp(git_dir, ".git"))
-		return;
+		return 0;
 	slash = strrchr(git_dir, '/');
 	if (slash && !strcmp(slash, "/.git"))
-		return;
+		return 0;
 
 	/*
 	 * Otherwise it is often bare.  At this point
 	 * we are just guessing.
 	 */
- force_bare:
-	is_bare_repository_cfg = 1;
-	return;
+	return 1;
+}
+
+int init_db(const char *git_dir, const char *template_dir, const char *work_dir,
+	    unsigned int flags)
+{
+	const char *sha1_dir;
+	char *path;
+	int len, reinit;
+
+	safe_create_dir(git_dir, 0);
+
+	set_git_dir(make_absolute_path(git_dir));
+
+	/* Check to see if the repository version is right.
+	 * Note that a newly created repository does not have
+	 * config file, so this will not fail.  What we are catching
+	 * is an attempt to reinitialize new repository with an old tool.
+	 */
+	check_repository_format();
+
+	reinit = create_default_files(git_dir, template_dir);
+
+	/*
+	 * And set up the object store.  Don't use
+	 * get_object_directory() here, since we're initializing
+	 * relative to git_dir, not $GIT_DIR.
+	 */
+	sha1_dir = getenv(DB_ENVIRONMENT);
+	if (!sha1_dir)
+		sha1_dir = mkpath("%s/objects", git_dir);
+	len = strlen(sha1_dir);
+	path = xmalloc(len + 40);
+	memcpy(path, sha1_dir, len);
+
+	safe_create_dir(sha1_dir, 1);
+	strcpy(path+len, "/pack");
+	safe_create_dir(path, 1);
+	strcpy(path+len, "/info");
+	safe_create_dir(path, 1);
+
+	if (shared_repository) {
+		char buf[10];
+		/* We do not spell "group" and such, so that
+		 * the configuration can be read by older version
+		 * of git.
+		 */
+		sprintf(buf, "%d", shared_repository);
+		git_config_set("core.sharedrepository", buf);
+		git_config_set("receive.denyNonFastforwards", "true");
+	}
+
+	if (!(flags & INIT_DB_QUIET))
+		printf("%s%s Git repository in %s/\n",
+		       reinit ? "Reinitialized existing" : "Initialized empty",
+		       shared_repository ? " shared" : "",
+		       git_dir);
+
+	return 0;
 }
 
 static const char init_db_usage[] =
@@ -320,12 +377,10 @@ static const char init_db_usage[] =
  */
 int cmd_init_db(int argc, const char **argv, const char *prefix)
 {
-	const char *git_dir;
-	const char *sha1_dir;
+	const char *git_dir, *work_tree;
 	const char *template_dir = NULL;
-	char *path;
-	int len, i, reinit;
-	int quiet = 0;
+	unsigned int flags = 0;
+	int i;
 
 	for (i = 1; i < argc; i++, argv++) {
 		const char *arg = argv[1];
@@ -336,7 +391,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 		else if (!prefixcmp(arg, "--shared="))
 			shared_repository = git_config_perm("arg", arg+9);
 		else if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet"))
-		        quiet = 1;
+			flags |= INIT_DB_QUIET;
 		else
 			usage(init_db_usage);
 	}
@@ -353,64 +408,30 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 		    GIT_WORK_TREE_ENVIRONMENT,
 		    GIT_DIR_ENVIRONMENT);
 
-	guess_repository_type(git_dir);
-
-	if (is_bare_repository_cfg <= 0) {
-		git_work_tree_cfg = xcalloc(PATH_MAX, 1);
-		if (!getcwd(git_work_tree_cfg, PATH_MAX))
-			die ("Cannot access current working directory.");
-		if (access(get_git_work_tree(), X_OK))
-			die ("Cannot access work tree '%s'",
-			     get_git_work_tree());
-	}
-
 	/*
 	 * Set up the default .git directory contents
 	 */
-	git_dir = getenv(GIT_DIR_ENVIRONMENT);
 	if (!git_dir)
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
-	safe_create_dir(git_dir, 0);
-
-	/* Check to see if the repository version is right.
-	 * Note that a newly created repository does not have
-	 * config file, so this will not fail.  What we are catching
-	 * is an attempt to reinitialize new repository with an old tool.
-	 */
-	check_repository_format();
-
-	reinit = create_default_files(git_dir, template_dir);
-
-	/*
-	 * And set up the object store.
-	 */
-	sha1_dir = get_object_directory();
-	len = strlen(sha1_dir);
-	path = xmalloc(len + 40);
-	memcpy(path, sha1_dir, len);
 
-	safe_create_dir(sha1_dir, 1);
-	strcpy(path+len, "/pack");
-	safe_create_dir(path, 1);
-	strcpy(path+len, "/info");
-	safe_create_dir(path, 1);
+	if (is_bare_repository_cfg < 0)
+		is_bare_repository_cfg = guess_repository_type(git_dir);
 
-	if (shared_repository) {
-		char buf[10];
-		/* We do not spell "group" and such, so that
-		 * the configuration can be read by older version
-		 * of git.
-		 */
-		sprintf(buf, "%d", shared_repository);
-		git_config_set("core.sharedrepository", buf);
-		git_config_set("receive.denyNonFastforwards", "true");
+	if (!is_bare_repository_cfg) {
+		if (git_dir) {
+			const char *git_dir_parent = strrchr(git_dir, '/');
+			if (git_dir_parent)
+				git_work_tree_cfg = strdup(make_absolute_path(xstrndup(git_dir, git_dir_parent - git_dir)));
+		}
+		if (!git_work_tree_cfg) {
+			git_work_tree_cfg = xcalloc(PATH_MAX, 1);
+			if (!getcwd(git_work_tree_cfg, PATH_MAX))
+				die ("Cannot access current working directory.");
+		}
+		if (access(get_git_work_tree(), X_OK))
+			die ("Cannot access work tree '%s'",
+			     get_git_work_tree());
 	}
 
-	if (!quiet)
-		printf("%s%s Git repository in %s/\n",
-		       reinit ? "Reinitialized existing" : "Initialized empty",
-		       shared_repository ? " shared" : "",
-		       git_dir);
-
-	return 0;
+	return init_db(git_dir, template_dir, git_work_tree_cfg, flags);
 }
diff --git a/builtin.h b/builtin.h
index 674c8a1..4a382b5 100644
--- a/builtin.h
+++ b/builtin.h
@@ -24,6 +24,7 @@ extern int cmd_check_attr(int argc, const char **argv, const char *prefix);
 extern int cmd_check_ref_format(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry_pick(int argc, const char **argv, const char *prefix);
+extern int cmd_clone(int argc, const char **argv, const char *prefix);
 extern int cmd_clean(int argc, const char **argv, const char *prefix);
 extern int cmd_commit(int argc, const char **argv, const char *prefix);
 extern int cmd_commit_tree(int argc, const char **argv, const char *prefix);
diff --git a/cache.h b/cache.h
index 660ea04..6d806c4 100644
--- a/cache.h
+++ b/cache.h
@@ -322,6 +322,11 @@ extern const char *prefix_filename(const char *prefix, int len, const char *path
 extern void verify_filename(const char *prefix, const char *name);
 extern void verify_non_filename(const char *prefix, const char *name);
 
+#define INIT_DB_QUIET 0x0001
+
+extern int init_db(const char *git_dir, const char *template_dir,
+		   const char *work_dir, unsigned int flags);
+
 #define alloc_nr(x) (((x)+16)*3/2)
 
 /*
diff --git a/git-clone.sh b/contrib/examples/git-clone.sh
similarity index 100%
rename from git-clone.sh
rename to contrib/examples/git-clone.sh
diff --git a/environment.c b/environment.c
index 6739a3f..d6c6a6b 100644
--- a/environment.c
+++ b/environment.c
@@ -81,6 +81,12 @@ const char *get_git_dir(void)
 	return git_dir;
 }
 
+void set_git_work_tree(const char *new_work_tree)
+{
+	get_git_work_tree(); /* make sure it's initialized */
+	work_tree = xstrdup(make_absolute_path(new_work_tree));
+}
+
 const char *get_git_work_tree(void)
 {
 	static int initialized = 0;
diff --git a/git.c b/git.c
index 9cca81a..f32883a 100644
--- a/git.c
+++ b/git.c
@@ -285,6 +285,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
 		{ "cherry", cmd_cherry, RUN_SETUP },
 		{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
+		{ "clone", cmd_clone },
 		{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
 		{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
 		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
-- 
1.5.4.2.261.g851a5.dirty

Re: [RFC] Build in clone

[Johan Herland]

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

Re: [RFC] Build in clone

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1766 bytes --]

Hi,

On Tue, 26 Feb 2008, Johan Herland wrote:

> 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

Which machine?  What is your base (next or master)?  What does the verbose 
output look like?

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

This is almost certainly wrong.

> @@ -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));
> -	
> +

Trailing whitespace.  Okay.

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

Good catch.

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

I am not so sure about this change.  It is unneeded, and distracts from 
the rest.

Thanks,
Dscho

Re: [RFC] Build in clone

[Johan Herland]

On Tuesday 26 February 2008, Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 26 Feb 2008, Johan Herland wrote:
> 
> > 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
> 
> Which machine?  What is your base (next or master)?  What does the verbose 
> output look like?

I'm at work now (with a different machine), and when I try to reproduce
I get either 4 errors (with "make test") or 7 errors (with
"sh ./t5700-clone-reference.sh"). See [1] for verbose output.
This is with next (v1.5.4.3-342-g99e8cb6) + Daniel's patch on a 64-bit
Gentoo Linux on top of Intel Core 2 Duo with 4GB RAM. My home box is
similar (Quad instead of Duo). Without Daniel's patch, the entire test
suite passes.

> > 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>
> 
> This is almost certainly wrong.

Yes. But the intent is valid: His name should be in UTF-8; not i latin-1.

> > @@ -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));
> > -	
> > +
> 
> Trailing whitespace.  Okay.
> 
> > @@ -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);
> 
> Good catch.

Thanks.

> > @@ -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)) {
> 
> I am not so sure about this change.  It is unneeded, and distracts from 
> the rest.

Yes, if this were meant for inclusion, I'd agree. However, as I originally
stated above the patch, this was pretty much a verbatim dump of my buffers
with the immediate issues I found. It was meant as (hopefully useful)
feedback to Daniel, and not for any kind of consumption by git proper.

Still, I should probably have taken the extra minute to filter out unneeded
whitespace churn.


Have fun! :)

...Johan


[1] Verbose output from t5700-clone-reference.sh:

$ ./t5700-clone-reference.sh --verbose
* expecting success: test_create_repo A && cd A &&
echo first > file1 &&
git add file1 &&
git commit -m initial
Created initial commit 8c40535: initial
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 file1
*   ok 1: preparing first repository

* expecting success: git clone A B && cd B &&
echo second > file2 &&
git add file2 &&
git commit -m addition &&
git repack -a -d &&
git prune
Initialize B/.git
Initialized empty Git repository in B/.git/
Okay
Get for /home/johanh/git-test/git/t/trash/A/.git
0x714f50
HEAD
refs/heads/master
work tree now /home/johanh/git-test/git/t/trash/B
Created commit 4f5d964: addition
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 file2
Counting objects: 6, done.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (6/6), done.
Total 6 (delta 0), reused 0 (delta 0)
*   ok 2: preparing second repository

* expecting success: git clone -l -s --reference B A C
Initialize C/.git
Initialized empty Git repository in C/.git/
Okay
Wrote /home/johanh/git-test/git/t/trash/B/.git/objects
 to C/.git/objects/info/alternates
4f5d96490c0371a2814efb3203ed699ef4814fda -> C/.git/refs/reference-tmp/4f5d96490c0371a2814efb3203ed699ef4814fda
error: refs/remotes/origin/HEAD does not point to a valid object!
error: Z;n�+ does not point to a valid object!
error: Z;n�+ does not point to a valid object!
error: �p does not point to a valid object!
error: Z;n�+ does not point to a valid object!
error: Z;n�+ does not point to a valid object!
error: Y;n�+ does not point to a valid object!
error: q does not point to a valid object!
./test-lib.sh: line 193: 32376 Segmentation fault      git clone -l -s --reference B A C
* FAIL 3: cloning with reference (-l -s)
        git clone -l -s --reference B A C

* expecting success: test `wc -l <C/.git/objects/info/alternates` = 2
* FAIL 4: existence of info/alternates
        test `wc -l <C/.git/objects/info/alternates` = 2

* expecting success: cd C &&
git pull ../B master
*   ok 5: pulling from reference

* expecting success: cd C &&
echo "0 objects, 0 kilobytes" > expected &&
git count-objects > current &&
diff expected current
*   ok 6: that reference gets used

* expecting success: git clone --reference B file://`pwd`/A D
Initialize D/.git
Initialized empty Git repository in D/.git/
Okay
Wrote /home/johanh/git-test/git/t/trash/B/.git/objects
 to D/.git/objects/info/alternates
4f5d96490c0371a2814efb3203ed699ef4814fda -> D/.git/refs/reference-tmp/4f5d96490c0371a2814efb3203ed699ef4814fda
error: refs/remotes/origin/HEAD does not point to a valid object!
�+ does not point to a valid object!
�+ does not point to a valid object!
error: �p does not point to a valid object!
�+ does not point to a valid object!
�+ does not point to a valid object!
error:  does not point to a valid object!
./test-lib.sh: line 193: 32440 Segmentation fault      git clone --reference B file://`pwd`/A D
* FAIL 7: cloning with reference (no -l -s)
        git clone --reference B file://`pwd`/A D

* expecting success: test `wc -l <D/.git/objects/info/alternates` = 1
*   ok 8: existence of info/alternates

* expecting success: cd D && git pull ../B master
*   ok 9: pulling from reference

* expecting success: cd D && echo "0 objects, 0 kilobytes" > expected &&
git count-objects > current &&
diff expected current
*   ok 10: that reference gets used

* expecting success: cd A &&
echo third > file3 &&
git add file3 &&
git commit -m update &&
git repack -a -d &&
git prune
Created commit 20c3827: update
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 file3
Counting objects: 6, done.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (6/6), done.
Total 6 (delta 0), reused 0 (delta 0)
*   ok 11: updating origin

* expecting success: cd C &&
git pull origin
fatal: 'origin': unable to chdir or not a git archive
fatal: The remote end hung up unexpectedly
* FAIL 12: pulling changes from origin
        cd C &&
        git pull origin

* expecting success: cd C &&
echo "2 objects" > expected &&
git count-objects | cut -d, -f1 > current &&
diff expected current
1c1
< 2 objects
---
> 0 objects
* FAIL 13: that alternate to origin gets used
        cd C &&
        echo "2 objects" > expected &&
        git count-objects | cut -d, -f1 > current &&
        diff expected current

* expecting success: cd D &&
git pull origin
fatal: 'origin': unable to chdir or not a git archive
fatal: The remote end hung up unexpectedly
* FAIL 14: pulling changes from origin
        cd D &&
        git pull origin

* expecting success: cd D &&
echo "5 objects" > expected &&
git count-objects | cut -d, -f1 > current &&
diff expected current
1c1
< 5 objects
---
> 0 objects
* FAIL 15: check objects expected to exist locally
        cd D &&
        echo "5 objects" > expected &&
        git count-objects | cut -d, -f1 > current &&
        diff expected current

* failed 7 among 15 test(s)


-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [RFC] Build in clone

[Johan Herland]

On Tuesday 26 February 2008, Johan Herland wrote:
> [1] Verbose output from t5700-clone-reference.sh:
> 
> $ ./t5700-clone-reference.sh --verbose
> * expecting success: test_create_repo A && cd A &&
> echo first > file1 &&
> git add file1 &&
> git commit -m initial
> Created initial commit 8c40535: initial
>  1 files changed, 1 insertions(+), 0 deletions(-)
>  create mode 100644 file1
> *   ok 1: preparing first repository
> 
> * expecting success: git clone A B && cd B &&
> echo second > file2 &&
> git add file2 &&
> git commit -m addition &&
> git repack -a -d &&
> git prune
> Initialize B/.git
> Initialized empty Git repository in B/.git/
> Okay
> Get for /home/johanh/git-test/git/t/trash/A/.git
> 0x714f50
> HEAD
> refs/heads/master
> work tree now /home/johanh/git-test/git/t/trash/B
> Created commit 4f5d964: addition
>  1 files changed, 1 insertions(+), 0 deletions(-)
>  create mode 100644 file2
> Counting objects: 6, done.
> Compressing objects: 100% (3/3), done.
> Writing objects: 100% (6/6), done.
> Total 6 (delta 0), reused 0 (delta 0)
> *   ok 2: preparing second repository
> 
> * expecting success: git clone -l -s --reference B A C
> Initialize C/.git
> Initialized empty Git repository in C/.git/
> Okay
> Wrote /home/johanh/git-test/git/t/trash/B/.git/objects
>  to C/.git/objects/info/alternates
> 4f5d96490c0371a2814efb3203ed699ef4814fda -> C/.git/refs/reference-tmp/4f5d96490c0371a2814efb3203ed699ef4814fda
> error: refs/remotes/origin/HEAD does not point to a valid object!
> error: Z;n�+ does not point to a valid object!
> error: Z;n�+ does not point to a valid object!
> error: �p does not point to a valid object!
> error: Z;n�+ does not point to a valid object!
> error: Z;n�+ does not point to a valid object!
> error: Y;n�+ does not point to a valid object!
> error: q does not point to a valid object!
> ./test-lib.sh: line 193: 32376 Segmentation fault      git clone -l -s --reference B A C
> * FAIL 3: cloning with reference (-l -s)
>         git clone -l -s --reference B A C

Running this test with GDB, I get the following backtrace:

#0  0x0000000000474b87 in is_null_sha1 (sha1=0x100000008 <Address 0x100000008 out of bounds>) at cache.h:464
#1  0x0000000000474ad3 in do_one_ref (base=0x4dc8ff "refs/", fn=0x419471 <setup_tmp_ref>, trim=0, cb_data=0x7498d0, entry=0xffffffff) at refs.c:474
#2  0x0000000000474e28 in do_for_each_ref (base=0x4dc8ff "refs/", fn=0x419471 <setup_tmp_ref>, trim=0, cb_data=0x7498d0) at refs.c:558
#3  0x0000000000474ecd in for_each_ref (fn=0x419471 <setup_tmp_ref>, cb_data=0x7498d0) at refs.c:580
#4  0x0000000000419706 in setup_reference (repo=0x745070 "C/.git") at builtin-clone.c:211
#5  0x0000000000419fce in cmd_clone (argc=2, argv=0x7fff7a282fa0, prefix=0x0) at builtin-clone.c:422
#6  0x0000000000404ba3 in run_command (p=0x6ff710, argc=7, argv=0x7fff7a282fa0) at git.c:248
#7  0x0000000000404d55 in handle_internal_command (argc=7, argv=0x7fff7a282fa0) at git.c:378
#8  0x0000000000404ebe in main (argc=7, argv=0x7fff7a282fa0) at git.c:442

Seems the "loose" ref_list in do_for_each_ref() becomes corrupted.

Will investigate more later.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [RFC] Build in clone

[Johan Herland]

On Tuesday 26 February 2008, Johan Herland wrote:
> Running this test with GDB, I get the following backtrace:
> 
> #0  0x0000000000474b87 in is_null_sha1 (sha1=0x100000008 <Address 0x100000008 out of bounds>) at cache.h:464
> #1  0x0000000000474ad3 in do_one_ref (base=0x4dc8ff "refs/", fn=0x419471 <setup_tmp_ref>, trim=0, cb_data=0x7498d0, entry=0xffffffff) at refs.c:474
> #2  0x0000000000474e28 in do_for_each_ref (base=0x4dc8ff "refs/", fn=0x419471 <setup_tmp_ref>, trim=0, cb_data=0x7498d0) at refs.c:558
> #3  0x0000000000474ecd in for_each_ref (fn=0x419471 <setup_tmp_ref>, cb_data=0x7498d0) at refs.c:580
> #4  0x0000000000419706 in setup_reference (repo=0x745070 "C/.git") at builtin-clone.c:211
> #5  0x0000000000419fce in cmd_clone (argc=2, argv=0x7fff7a282fa0, prefix=0x0) at builtin-clone.c:422
> #6  0x0000000000404ba3 in run_command (p=0x6ff710, argc=7, argv=0x7fff7a282fa0) at git.c:248
> #7  0x0000000000404d55 in handle_internal_command (argc=7, argv=0x7fff7a282fa0) at git.c:378
> #8  0x0000000000404ebe in main (argc=7, argv=0x7fff7a282fa0) at git.c:442
> 
> Seems the "loose" ref_list in do_for_each_ref() becomes corrupted.

...and the corruption is done when setup_tmp_ref() calls write_ref_sha1()
which calls invalidate_cached_refs() (which frees the ref_list that
do_for_each_ref() is iterating over).

Not sure how to best solve this. Maybe setup_tmp_ref() shouldn't use
write_ref_sha1(), but write the ref file directly instead, as hinted
at in a comment in setup_tmp_ref()?


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCH] Fix premature free of ref_lists while writing temporary refs to file

[Johan Herland]

We cannot call write_ref_sha1() from within a for_each_ref() callback, since
it will free() the ref_list that the for_each_ref() is currently traversing.

Therefore rewrite setup_tmp_ref() to not call write_ref_sha1(), as already
hinted at in a comment.

This causes the t5700-clone-reference testcases to pass for me.

Signed-off-by: Johan Herland <johan@herland.net>
---

On Tuesday 26 February 2008, Johan Herland wrote:
> On Tuesday 26 February 2008, Johan Herland wrote:
> > Running this test with GDB, I get the following backtrace:
> > 
> > #0  0x0000000000474b87 in is_null_sha1 (sha1=0x100000008 <Address 0x100000008 out of bounds>) at cache.h:464
> > #1  0x0000000000474ad3 in do_one_ref (base=0x4dc8ff "refs/", fn=0x419471 <setup_tmp_ref>, trim=0, cb_data=0x7498d0, entry=0xffffffff) at refs.c:474
> > #2  0x0000000000474e28 in do_for_each_ref (base=0x4dc8ff "refs/", fn=0x419471 <setup_tmp_ref>, trim=0, cb_data=0x7498d0) at refs.c:558
> > #3  0x0000000000474ecd in for_each_ref (fn=0x419471 <setup_tmp_ref>, cb_data=0x7498d0) at refs.c:580
> > #4  0x0000000000419706 in setup_reference (repo=0x745070 "C/.git") at builtin-clone.c:211
> > #5  0x0000000000419fce in cmd_clone (argc=2, argv=0x7fff7a282fa0, prefix=0x0) at builtin-clone.c:422
> > #6  0x0000000000404ba3 in run_command (p=0x6ff710, argc=7, argv=0x7fff7a282fa0) at git.c:248
> > #7  0x0000000000404d55 in handle_internal_command (argc=7, argv=0x7fff7a282fa0) at git.c:378
> > #8  0x0000000000404ebe in main (argc=7, argv=0x7fff7a282fa0) at git.c:442
> > 
> > Seems the "loose" ref_list in do_for_each_ref() becomes corrupted.
> 
> ...and the corruption is done when setup_tmp_ref() calls write_ref_sha1()
> which calls invalidate_cached_refs() (which frees the ref_list that
> do_for_each_ref() is iterating over).
> 
> Not sure how to best solve this. Maybe setup_tmp_ref() shouldn't use
> write_ref_sha1(), but write the ref file directly instead, as hinted
> at in a comment in setup_tmp_ref()?

Here is a shot at fixing this, although I'm not sure it's the best way
of doing so.


Have fun! :)

...Johan

 builtin-clone.c |   43 ++++++++++++++++++++-----------------------
 1 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 6e34e52..d5baffc 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -136,14 +136,12 @@ static int
 setup_tmp_ref(const char *refname,
 	      const unsigned char *sha1, int flags, void *cb_data)
 {
-	const char *ref_temp = cb_data;
+	const char *ref_temp = cb_data, *sha1_hex = sha1_to_hex(sha1);
 	char *path;
-	struct lock_file lk;
-	struct ref_lock *rl;
+	int fd;
 
 	/*
 
-	echo "$ref_git/objects" >"$GIT_DIR/objects/info/alternates"
 	(
 		GIT_DIR="$ref_git" git for-each-ref \
 			--format='%(objectname) %(*objectname)'
@@ -158,25 +156,24 @@ setup_tmp_ref(const char *refname,
 
 	*/
 
-	/* We go a bit out of way to use write_ref_sha1() here.  We
-	 * could just write the ref file directly, since neither
-	 * locking or reflog really matters here.  However, let's use
-	 * the standard interface for writing refs as much as is
-	 * possible given that get_git_dir() != the repo we're writing
-	 * the refs in. */
-
-	printf("%s -> %s/%s\n",
-	       sha1_to_hex(sha1), ref_temp, sha1_to_hex(sha1));
-
-	path = mkpath("%s/%s", ref_temp, sha1_to_hex(sha1));
-	rl = xmalloc(sizeof *rl);
-	rl->force_write = 1;
-	rl->lk = &lk;
-	rl->ref_name = xstrdup(sha1_to_hex(sha1));
-	rl->orig_ref_name = xstrdup(rl->ref_name);
-	rl->lock_fd = hold_lock_file_for_update(rl->lk, path, 1);
-	if (write_ref_sha1(rl, sha1, NULL) < 0)
-		die("failed to write temporary ref %s", lk.filename);
+	/* Write the ref file directly, since neither locking or reflog really
+	 * matters here. We should probably use some standard interface for
+	 * writing refs here, although write_ref_sha1() does not work.
+	 * (It frees the ref_list that is currently being iterated by
+	 * for_each_ref().) Keep in mind that get_git_dir() != the repo we're
+	 * writing the refs in. */
+
+	path = mkpath("%s/%s", ref_temp, sha1_hex);
+
+	printf("%s -> %s\n", sha1_hex, path);
+
+	fd = open(path, O_CREAT | O_WRONLY, 0666);
+	if (fd < 0)
+		die("failed to create %s", path);
+	write_or_die(fd, sha1_hex, strlen(sha1_hex));
+	if (close(fd))
+		die("could not close %s", path);
+	fprintf(stderr, "Wrote %s to %s\n", sha1_hex, path);
 
 	return 0;
 }
-- 
1.5.4.3.342.g99e8

[PATCH] Fix premature call to git_config() causing t1020-subdirectory to fail

[Johan Herland]

We need to call git_config(git_default_config) in order to get user.name and
user.email (so that reflogs will be correct), but if we do it too early, it
interferes with the setup of reference repos. Therefore, move git_config()
call to _after_ the reference has been setup (but before we start writing
reflogs). However, in order for git_config() to read in the global
configuration at that point, we must unset CONFIG_ENVIRONMENT.

There are probably better ways of resolving this issue.

Signed-off-by: Johan Herland <johan@herland.net>
---

On Tuesday 26 February 2008, Johan Herland wrote:
> - 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)

Here is a fix for this breakage, although I think it's ugly as hell.

But with this fix, and the other one I just sent out for the
for_each_ref() corruption, the whole test suite finally passes on my
box.

Feel free to incorporate this into the further builtin-clone work,
or ignore it, and find better ways of solving these issues.


Have fun! :)

...Johan


 builtin-clone.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index d5baffc..c02a04c 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -346,8 +346,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	char branch_top[256], key[256], refname[256], value[256];
 	struct strbuf reflog_msg;
 
-	git_config(git_default_config);
-
 	clone_pid = getpid();
 
 	argc = parse_options(argc, argv, builtin_clone_options,
@@ -423,6 +421,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	set_git_dir(make_absolute_path(git_dir));
 
+	/* This must happen _after_ git_dir has been set up */
+	unsetenv(CONFIG_ENVIRONMENT); /* need user/email from global config */
+	git_config(git_default_config);
+
 	if (option_bare)
 		git_config_set("core.bare", "true");
 
-- 
1.5.4.3.342.g99e8

Re: [PATCH] Fix premature free of ref_lists while writing temporary refs to file

[Johannes Schindelin]

Hi,

On Tue, 26 Feb 2008, Johan Herland wrote:

> We cannot call write_ref_sha1() from within a for_each_ref() callback, 
> since it will free() the ref_list that the for_each_ref() is currently 
> traversing.
> 
> Therefore rewrite setup_tmp_ref() to not call write_ref_sha1(), as 
> already hinted at in a comment.

I guess the reason was to use a much of an API as possible.

If you already avoid that, why not write into .git/packed-refs directly?

Ciao,
Dscho

Re: [PATCH] Fix premature call to git_config() causing t1020-subdirectory to fail

[Johannes Schindelin]

Hi,

On Tue, 26 Feb 2008, Johan Herland wrote:

> On Tuesday 26 February 2008, Johan Herland wrote:
> > - 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)
> 
> Here is a fix for this breakage, although I think it's ugly as hell.

I think it is not ugly, but correct.

Ciao,
Dscho

Re: [PATCH] Fix premature free of ref_lists while writing temporary refs to file

[Johan Herland]

On Tuesday 26 February 2008, Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 26 Feb 2008, Johan Herland wrote:
> 
> > We cannot call write_ref_sha1() from within a for_each_ref() callback, 
> > since it will free() the ref_list that the for_each_ref() is currently 
> > traversing.
> > 
> > Therefore rewrite setup_tmp_ref() to not call write_ref_sha1(), as 
> > already hinted at in a comment.
> 
> I guess the reason was to use a much of an API as possible.
> 
> If you already avoid that, why not write into .git/packed-refs directly?

That's exactly what I'm planning. ;-)

...just wanted to get the existing code working first.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [RFC] Build in clone

[Daniel Barkalow]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2808 bytes --]

On Tue, 26 Feb 2008, Johan Herland wrote:

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

Good to hear. A certain amount of the point is performance, and I've only 
got relatively simple repositories on Linux to test with, where everything 
is too fast to tell anyway.

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

I think that's just a call to the wrong function (and a lack of very very 
explicit documentation).

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

I should have read email last night; I could have identified a bunch of 
the odd errors for you, but you've figured most of them out by now.

I need to look into the config system further; things should be configured 
such that the local config is in the new directory and the global config 
is unchanged. If no environment variable is set and pwd is a certain way, 
git_config_set() will write to the wrong file.

> - Fix "clone from $repo" reflog messages (using strbufs; something tells
>   me more of this code would benefit from using strbufs)

Most likely. I think Kristian wrote most of this before strbuf existed or 
something of the sort.

> - Høgsberg's name should be in UTF-8 (not sure if this will survive this
>   mail)

It is for me; I bet it didn't survive the mail from me to you. (That is, 
your mailer got my UTF-8 message and converted it to Latin-1 on export, 
and so applying it gave a Latin-1 ø in your tree, which you changed to 
UTF-8, and then your mailer again thought your patch was changing 
one Latin-1 character to two, and sent the patch like that.)

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

I haven't gotten to the level of style in this code myself, so no worries 
there. :) And whitespace noise is welcome at this point, since I'm still 
incorporating changes and evaluating the resulting state, instead of 
evaluating the changes themselves.

	-Daniel
*This .sig left intentionally blank*

Re: [RFC] Build in clone

[Kristian Høgsberg]

On Tue, 2008-02-26 at 12:36 -0500, Daniel Barkalow wrote:
> On Tue, 26 Feb 2008, Johan Herland wrote:
> 
> > 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) :)
> 
> Good to hear. A certain amount of the point is performance, and I've only 
> got relatively simple repositories on Linux to test with, where everything 
> is too fast to tell anyway.

Yeah, that's pretty cool.

> > - Fix "clone from $repo" reflog messages (using strbufs; something tells
> >   me more of this code would benefit from using strbufs)
> 
> Most likely. I think Kristian wrote most of this before strbuf existed or 
> something of the sort.

No this was after the strbuf API went in.  The "clone from $repo"
messages I just left at the time because I was lazy, but yeah, they'll
need some strbuf love, or maybe just snprintf.  I don't agree that
strbuf applies very well for the rest of the code.  I'm a big fan of the
strbuf functionality, but I've been very deliberate about using and not
using it.

cheers,
Kristian

Re: [PATCH] Fix premature call to git_config() causing t1020-subdirectory to fail

[Daniel Barkalow]

On Tue, 26 Feb 2008, Johan Herland wrote:

> We need to call git_config(git_default_config) in order to get user.name and
> user.email (so that reflogs will be correct), but if we do it too early, it
> interferes with the setup of reference repos. Therefore, move git_config()
> call to _after_ the reference has been setup (but before we start writing
> reflogs). However, in order for git_config() to read in the global
> configuration at that point, we must unset CONFIG_ENVIRONMENT.
> 
> There are probably better ways of resolving this issue.
> 
> Signed-off-by: Johan Herland <johan@herland.net>
> ---
> 
> On Tuesday 26 February 2008, Johan Herland wrote:
> > - 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)
> 
> Here is a fix for this breakage, although I think it's ugly as hell.
> 
> But with this fix, and the other one I just sent out for the
> for_each_ref() corruption, the whole test suite finally passes on my
> box.
> 
> Feel free to incorporate this into the further builtin-clone work,
> or ignore it, and find better ways of solving these issues.

Actually, I think I'll be leaving CONFIG_ENVIRONMENT alone entirely; I was 
only using it to override the setting that t5505 uses, but t5505 is just 
wrong to set it. So this is the right placement of git_config(), and the 
setenv and unsetenv aren't needed.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Fix premature call to git_config() causing t1020-subdirectory to fail

[Johannes Schindelin]

Hi,

On Tue, 26 Feb 2008, Daniel Barkalow wrote:

> Actually, I think I'll be leaving CONFIG_ENVIRONMENT alone entirely; I 
> was only using it to override the setting that t5505 uses, but t5505 is 
> just wrong to set it. So this is the right placement of git_config(), 
> and the setenv and unsetenv aren't needed.

Well, existing git-clone.sh sets GIT_CONFIG.  So we have to unset any 
existing GIT_CONFIG at least.

Ciao,
Dscho

Re: [PATCH] Fix premature call to git_config() causing t1020-subdirectory to fail

[Daniel Barkalow]

On Tue, 26 Feb 2008, Johannes Schindelin wrote:

> Hi,
> 
> On Tue, 26 Feb 2008, Daniel Barkalow wrote:
> 
> > Actually, I think I'll be leaving CONFIG_ENVIRONMENT alone entirely; I 
> > was only using it to override the setting that t5505 uses, but t5505 is 
> > just wrong to set it. So this is the right placement of git_config(), 
> > and the setenv and unsetenv aren't needed.
> 
> Well, existing git-clone.sh sets GIT_CONFIG.  So we have to unset any 
> existing GIT_CONFIG at least.

As far as I can tell, that's a flaw in git-clone.sh; if the user has set 
GIT_CONFIG, it shouldn't be the case that every program other than 
git-clone obeys it while git-clone ignores it. (On the other hand, 
possibly every program other than git-config should ignore it, since it's 
only documented as affecting git-config.) git-clone.sh only sets it, I 
think, because it runs programs from the wrong context for them to do the 
right thing by default, not because it's specifically trying to override a 
user-provided setting.

	-Daniel
*This .sig left intentionally blank*.

Re: [PATCH] Fix premature free of ref_lists while writing temporary refs to file

[Daniel Barkalow]

On Tue, 26 Feb 2008, Johannes Schindelin wrote:

> Hi,
> 
> On Tue, 26 Feb 2008, Johan Herland wrote:
> 
> > We cannot call write_ref_sha1() from within a for_each_ref() callback, 
> > since it will free() the ref_list that the for_each_ref() is currently 
> > traversing.
> > 
> > Therefore rewrite setup_tmp_ref() to not call write_ref_sha1(), as 
> > already hinted at in a comment.
> 
> I guess the reason was to use a much of an API as possible.
> 
> If you already avoid that, why not write into .git/packed-refs directly?

Actually, it looks to me like the really right thing to do is tell 
for_each_ref() to also include these refs temporarily, and not actually 
write them to disk, read them back, and then delete them.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Fix premature free of ref_lists while writing temporary refs to file

[Johan Herland]

On Wednesday 27 February 2008, Daniel Barkalow wrote:
> On Tue, 26 Feb 2008, Johannes Schindelin wrote:
> 
> > Hi,
> > 
> > On Tue, 26 Feb 2008, Johan Herland wrote:
> > 
> > > We cannot call write_ref_sha1() from within a for_each_ref() callback, 
> > > since it will free() the ref_list that the for_each_ref() is currently 
> > > traversing.
> > > 
> > > Therefore rewrite setup_tmp_ref() to not call write_ref_sha1(), as 
> > > already hinted at in a comment.
> > 
> > I guess the reason was to use a much of an API as possible.
> > 
> > If you already avoid that, why not write into .git/packed-refs directly?
> 
> Actually, it looks to me like the really right thing to do is tell 
> for_each_ref() to also include these refs temporarily, and not actually 
> write them to disk, read them back, and then delete them.

I completely agree.


...Johan


-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH] Fix premature call to git_config() causing t1020-subdirectory to fail

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Tue, 26 Feb 2008, Johannes Schindelin wrote:
>
>> Hi,
>> 
>> On Tue, 26 Feb 2008, Daniel Barkalow wrote:
>> 
>> > Actually, I think I'll be leaving CONFIG_ENVIRONMENT alone entirely; I 
>> > was only using it to override the setting that t5505 uses, but t5505 is 
>> > just wrong to set it. So this is the right placement of git_config(), 
>> > and the setenv and unsetenv aren't needed.
>> 
>> Well, existing git-clone.sh sets GIT_CONFIG.  So we have to unset any 
>> existing GIT_CONFIG at least.
>
> As far as I can tell, that's a flaw in git-clone.sh; if the user has set 
> GIT_CONFIG, it shouldn't be the case that every program other than 
> git-clone obeys it while git-clone ignores it. (On the other hand, 
> possibly every program other than git-config should ignore it, since it's 
> only documented as affecting git-config.) git-clone.sh only sets it, I 
> think, because it runs programs from the wrong context for them to do the 
> right thing by default, not because it's specifically trying to override a 
> user-provided setting.

When cloning locally, there are two repositories involved, and
GIT_CONFIG if exists in the environment talks about the original
one that gets cloned.  Without setting GIT_CONFIG explicitly how
would you set the configuration that is about the new repository
clone creates?

And to be consistent, if cloning remotely, we should do the
same, which means GIT_CONFIG should be reset to point at the
configuration file inside the new repository, be it .git/config
or $repo/config (if bare).

I think that is the reason behind it.

Re: [PATCH] Fix premature call to git_config() causing t1020-subdirectory to fail

[Daniel Barkalow]

On Tue, 26 Feb 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > On Tue, 26 Feb 2008, Johannes Schindelin wrote:
> >
> >> Hi,
> >> 
> >> On Tue, 26 Feb 2008, Daniel Barkalow wrote:
> >> 
> >> > Actually, I think I'll be leaving CONFIG_ENVIRONMENT alone entirely; I 
> >> > was only using it to override the setting that t5505 uses, but t5505 is 
> >> > just wrong to set it. So this is the right placement of git_config(), 
> >> > and the setenv and unsetenv aren't needed.
> >> 
> >> Well, existing git-clone.sh sets GIT_CONFIG.  So we have to unset any 
> >> existing GIT_CONFIG at least.
> >
> > As far as I can tell, that's a flaw in git-clone.sh; if the user has set 
> > GIT_CONFIG, it shouldn't be the case that every program other than 
> > git-clone obeys it while git-clone ignores it. (On the other hand, 
> > possibly every program other than git-config should ignore it, since it's 
> > only documented as affecting git-config.) git-clone.sh only sets it, I 
> > think, because it runs programs from the wrong context for them to do the 
> > right thing by default, not because it's specifically trying to override a 
> > user-provided setting.
> 
> When cloning locally, there are two repositories involved, and
> GIT_CONFIG if exists in the environment talks about the original
> one that gets cloned.

There's nothing in the documentation to suggest that you can use 
GIT_CONFIG to affect how the old repository is read, or that GIT_CONFIG 
doesn't affect the new repository. Actually, as far as I can tell, the 
configuration of a repository you're cloning (local or remote) doesn't 
matter at all. Note that GIT_DIR and GIT_WORK_TREE refer to the new repo, 
so it would be surprising for GIT_CONFIG to refer to the old one.

> Without setting GIT_CONFIG explicitly how would you set the 
> configuration that is about the new repository clone creates?

By setting GIT_DIR, which also makes everything else work right. (We may 
want to probe the old repository some by setting GIT_DIR to it 
temporarily, but we can just collect information that way, and then set it 
to the new one to configure it and set it up.)

My current design is to collect some initial information, create 
directories, and then set the work tree and git dir. Then we run fetch, 
configure things, etc., in the new context.

I'm not sure why git-clone.sh doesn't just set GIT_DIR to whatever it 
decides the git dir with be, and leave GIT_CONFIG alone (or unset it, if 
it's expected to mean something different).

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Fix premature call to git_config() causing t1020-subdirectory to fail

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> There's nothing in the documentation to suggest that you can use 
> GIT_CONFIG to affect how the old repository is read, or that GIT_CONFIG 
> doesn't affect the new repository. Actually, as far as I can tell, the 
> configuration of a repository you're cloning (local or remote) doesn't 
> matter at all. Note that GIT_DIR and GIT_WORK_TREE refer to the new repo, 
> so it would be surprising for GIT_CONFIG to refer to the old one.

There was a bit of confusion in this discussion.

GIT_DIR the user may have in the environment may refer to the
old reopsitory before "git clone" is invoked, but it should not
matter at all, as the origin of the cloning comes from the
command line and that is where we will read from.  The scripted
version sets GIT_DIR for our own use to point at the new
repository upfront and exports it, so we are safe from bogus
GIT_DIR value the user may have in the environment.

GIT_WORK_TREE naming the new repository feels Ok, as you do not
care about the work tree of the original tree when cloning, and
you may want to have a say in where the work tree associated
with the new repository should go.

GIT_CONFIG the user may have will refer to the old repository
before "git clone" is invoked, as there is no new repository
built yet.  But clone does not read from the old config, so "you
can use GIT_CONFIG to read from old repository" may be true, but
it does not matter.  We won't use it (we do _not_ want to use
it) to read from the old configuration file.

We would however want to make sure that we write to the correct
configuration file of the new repository and not some random
other place, and that's where the environment variable in the
scripted version comes into the picture.

In the scripted version, the only way to make sure which exact
configuration file is updated is to set and export GIT_CONFIG
when running "git config", so there are a few places that does
exactly that (e.g. call to git-init and setting of core.bare).
Unfortunately many codepaths in the scripted version are utterly
careless (e.g. setting of remote."$origin".fetch); they should
make sure that they protect themselves against GIT_CONFIG the
user may have in the environment that point at random places.

> My current design is to collect some initial information, create 
> directories, and then set the work tree and git dir. Then we run fetch, 
> configure things, etc., in the new context.

That sounds like a clean design to me.

Re: [PATCH] Fix premature call to git_config() causing t1020-subdirectory to fail

[Daniel Barkalow]

On Tue, 26 Feb 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > There's nothing in the documentation to suggest that you can use 
> > GIT_CONFIG to affect how the old repository is read, or that GIT_CONFIG 
> > doesn't affect the new repository. Actually, as far as I can tell, the 
> > configuration of a repository you're cloning (local or remote) doesn't 
> > matter at all. Note that GIT_DIR and GIT_WORK_TREE refer to the new repo, 
> > so it would be surprising for GIT_CONFIG to refer to the old one.
> 
> There was a bit of confusion in this discussion.
> 
> GIT_DIR the user may have in the environment may refer to the
> old reopsitory before "git clone" is invoked, but it should not
> matter at all, as the origin of the cloning comes from the
> command line and that is where we will read from.  The scripted
> version sets GIT_DIR for our own use to point at the new
> repository upfront and exports it, so we are safe from bogus
> GIT_DIR value the user may have in the environment.

Huh. I think there's a comment in some test or somewhere that made me 
think that "GIT_DIR=dest.git git clone foo" would write to dest.git 
instead of ./foo/.git, but your description here is accurate.

> GIT_WORK_TREE naming the new repository feels Ok, as you do not
> care about the work tree of the original tree when cloning, and
> you may want to have a say in where the work tree associated
> with the new repository should go.

We currently definitely support "GIT_WORK_TREE=work git clone something", 
pretty much explicitly on line 235 of git-clone.sh.

> GIT_CONFIG the user may have will refer to the old repository
> before "git clone" is invoked, as there is no new repository
> built yet.  But clone does not read from the old config, so "you
> can use GIT_CONFIG to read from old repository" may be true, but
> it does not matter.  We won't use it (we do _not_ want to use
> it) to read from the old configuration file.
> 
> We would however want to make sure that we write to the correct
> configuration file of the new repository and not some random
> other place, and that's where the environment variable in the
> scripted version comes into the picture.
> 
> In the scripted version, the only way to make sure which exact
> configuration file is updated is to set and export GIT_CONFIG
> when running "git config", so there are a few places that does
> exactly that (e.g. call to git-init and setting of core.bare).
> Unfortunately many codepaths in the scripted version are utterly
> careless (e.g. setting of remote."$origin".fetch); they should
> make sure that they protect themselves against GIT_CONFIG the
> user may have in the environment that point at random places.

Since it sets GIT_DIR, it also could simply unset GIT_CONFIG, and then 
everything would just write to the config file for the new GIT_DIR. On the 
other hand, if you have GIT_CONFIG exported in your environment, and you 
set up a repository with "git clone", and clone unsets or overrides 
GIT_CONFIG, then your new repository will immediately be unusable, because 
clone will set up the config file inside the new repository, but nothing 
you run after that will look in the new repository, since everything else 
obeys the GIT_CONFIG you still have set.

On the other hand, I don't see why any git command other than "git config" 
(run my the user directly) has any business looking at GIT_CONFIG, since 
it's only mentioned in the man page for git-config, and not in general for 
configuration, the wrapper, or other programs.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Fix premature call to git_config() causing t1020-subdirectory to fail

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> Since it sets GIT_DIR, it also could simply unset GIT_CONFIG, and then 
> everything would just write to the config file for the new GIT_DIR. On the 
> other hand, if you have GIT_CONFIG exported in your environment, and you 
> set up a repository with "git clone", and clone unsets or overrides 
> GIT_CONFIG, then your new repository will immediately be unusable, because 
> clone will set up the config file inside the new repository, but nothing 
> you run after that will look in the new repository, since everything else 
> obeys the GIT_CONFIG you still have set.

Yes, I think an interactive environment that has GIT_CONFIG is
simply misconfigured.

But on the other hand, I could well imagine a script that does
this:

	#!/bin/sh
	GIT_CONFIG=$elsewhere; export GIT_CONFIG
        do things to the $elsewhere file via git-config
        git clone $something $new
        talk about the $new in the $elsewhere file via git-config
	(
        	unset GIT_CONFIG ;# I am writing the script carefully!
		cd $new
                do something inside the clone
	)
        talk more about the $new in the $elsewhere file via git-config
	exit
        
> On the other hand, I don't see why any git command other than "git config" 
> (run my the user directly) has any business looking at GIT_CONFIG, since 
> it's only mentioned in the man page for git-config, and not in general for 
> configuration, the wrapper, or other programs.

I think reading from the configuration file is done by
everybody, and GIT_CONFIG affects where the information is read
from.  Maybe it was a misfeature.  I dunno.

Re: [PATCH] Fix premature call to git_config() causing t1020-subdirectory to fail

[Daniel Barkalow]

On Wed, 27 Feb 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > Since it sets GIT_DIR, it also could simply unset GIT_CONFIG, and then 
> > everything would just write to the config file for the new GIT_DIR. On the 
> > other hand, if you have GIT_CONFIG exported in your environment, and you 
> > set up a repository with "git clone", and clone unsets or overrides 
> > GIT_CONFIG, then your new repository will immediately be unusable, because 
> > clone will set up the config file inside the new repository, but nothing 
> > you run after that will look in the new repository, since everything else 
> > obeys the GIT_CONFIG you still have set.
> 
> Yes, I think an interactive environment that has GIT_CONFIG is
> simply misconfigured.
> 
> But on the other hand, I could well imagine a script that does
> this:
> 
> 	#!/bin/sh
> 	GIT_CONFIG=$elsewhere; export GIT_CONFIG
>         do things to the $elsewhere file via git-config
>         git clone $something $new
>         talk about the $new in the $elsewhere file via git-config
> 	(
>         	unset GIT_CONFIG ;# I am writing the script carefully!
> 		cd $new
>                 do something inside the clone
> 	)
>         talk more about the $new in the $elsewhere file via git-config
> 	exit

That seems counterintuitive to me. If you created $new with init instead 
of clone, entirely different things would happen. And any other git 
commands outside the subshell would behave oddly. (Actually, the first of 
these reminds me why I thought git-clone used the caller's $GIT_DIR: 
git-init does.)

> > On the other hand, I don't see why any git command other than "git config" 
> > (run my the user directly) has any business looking at GIT_CONFIG, since 
> > it's only mentioned in the man page for git-config, and not in general for 
> > configuration, the wrapper, or other programs.
> 
> I think reading from the configuration file is done by
> everybody, and GIT_CONFIG affects where the information is read
> from.  Maybe it was a misfeature.  I dunno.

It makes sense that it would work that way, but the documentation should 
probably reflect that in config.txt.

	-Daniel
*This .sig left intentionally blank*

[PATCH] builtin-clone: create remotes/origin/HEAD symref, if guessed

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Tue, 26 Feb 2008, Daniel Barkalow wrote:

	> On Tue, 26 Feb 2008, Johan Herland wrote:
	> 
	> > 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).
	> 
	> I think that's just a call to the wrong function (and a lack of 
	> very very explicit documentation).

	This fixes it.

 builtin-clone.c |   26 ++++++++++++++------------
 1 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 89ef665..bd09b0f 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -518,33 +518,35 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		git_config_set_multivar(key, value, "^$", 0);
 	}
 
+	if (head_points_at)
+		/* Local default branch */
+		create_symref("HEAD", head_points_at->name, NULL);
+
 	if (option_bare) {
-		if (head_points_at) {
-			/* Local default branch */
-			create_symref("HEAD", head_points_at->name, NULL);
-		}
 		junk_work_tree = NULL;
 		junk_git_dir = NULL;
 		return 0;
 	}
 
 	if (head_points_at) {
+		struct strbuf buf;
+
 		if (strrchr(head_points_at->name, '/'))
 			head = strrchr(head_points_at->name, '/') + 1;
 		else
 			head = head_points_at->name;
 
-		/* Local default branch */
-		create_symref("HEAD", head_points_at->name, NULL);

[PATCH, fixed] builtin-clone: create remotes/origin/HEAD symref, if guessed

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Sorry, my previous patch was broken in so many ways.  This one
	is better, promise.

	BTW this incidentally fixes the branch.<branch>.{remote,merge} 
	setup: it used to strip all up-to and including a slash from the 
	ref name.  This just _happened_ to work, because commonly HEAD is 
	at "refs/heads/master".  However, if it is at "refs/heads/a/b", it 
	would fail.

 builtin-clone.c |   35 ++++++++++++++++++++---------------
 1 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 056e8a3..f27d205 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -523,33 +523,38 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		git_config_set_multivar(key, value, "^$", 0);
 	}
 
+	if (head_points_at)
+		/* Local default branch */
+		create_symref("HEAD", head_points_at->name, NULL);
+
 	if (option_bare) {
-		if (head_points_at) {
-			/* Local default branch */
-			create_symref("HEAD", head_points_at->name, NULL);
-		}
 		junk_work_tree = NULL;
 		junk_git_dir = NULL;
 		return 0;
 	}
 
 	if (head_points_at) {
-		if (strrchr(head_points_at->name, '/'))
-			head = strrchr(head_points_at->name, '/') + 1;
-		else
-			head = head_points_at->name;
+		struct strbuf head_ref, real_ref;
 
-		/* Local default branch */
-		create_symref("HEAD", head_points_at->name, NULL);
+		head = head_points_at->name;
+		if (!prefixcmp(head, "refs/heads/"))
+			head += 11;
 
 		/* Tracking branch for the primary branch at the remote. */
 		update_ref(NULL, "HEAD", head_points_at->old_sha1,
 			   NULL, 0, DIE_ON_ERR);
-	/*
-		rm -f "refs/remotes/$origin/HEAD"
-		git symbolic-ref "refs/remotes/$origin/HEAD" \
-			"refs/remotes/$origin/$head_points_at" &&
-	*/
+
+		strbuf_init(&head_ref, 0);
+		strbuf_addstr(&head_ref, branch_top);
+		strbuf_addstr(&head_ref, "/HEAD");
+		delete_ref(head_ref.buf, head_points_at->old_sha1);
+		strbuf_init(&real_ref, 0);
+		strbuf_addstr(&real_ref, branch_top);
+		strbuf_addch(&real_ref, '/');
+		strbuf_addstr(&real_ref, head);
+		create_symref(head_ref.buf, real_ref.buf, reflog_msg.buf);
+		strbuf_release(&head_ref);
+		strbuf_release(&real_ref);
 
 		snprintf(key, sizeof key, "branch.%s.remote", head);
 		git_config_set(key, option_origin);
-- 
1.5.4.3.446.gbe8932

[PATCH] builtin clone: support bundles

[Johannes Schindelin]

This forward-ports c6fef0bb(clone: support cloning full bundles) to the
builtin clone.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Now my private tree passes all tests again.

	Daniel, do you have a branch where you have your current version 
	of builting clone?  Mine is in "my-next" of 
	http://repo.or.cz/w/git/dscho.git.

 builtin-clone.c |   59 ++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index f27d205..29cd09d 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -64,42 +64,52 @@ static struct option builtin_clone_options[] = {
 	OPT_END()
 };
 
-static char *get_repo_path(const char *repo)
+static char *get_repo_path(const char *repo, int *is_bundle)
 {
-	const char *path;
-	struct stat buf;
-
-	path = mkpath("%s/.git", repo);
-	if (!stat(path, &buf) && S_ISDIR(buf.st_mode))
-		return xstrdup(make_absolute_path(path));
-
-	path = mkpath("%s.git", repo);
-	if (!stat(path, &buf) && S_ISDIR(buf.st_mode))
-		return xstrdup(make_absolute_path(path));
+	static char *suffix[] = { "/.git", ".git", "" };
+	static char *bundle_suffix[] = { ".bundle", "" };
+	struct stat st;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(suffix); i++) {
+		const char *path;
+		path = mkpath("%s%s", repo, suffix[i]);
+		if (!stat(path, &st) && S_ISDIR(st.st_mode)) {
+			*is_bundle = 0;
+			return xstrdup(make_absolute_path(path));
+		}
+	}
 
-	if (!stat(repo, &buf) && S_ISDIR(buf.st_mode))
-		return xstrdup(make_absolute_path(repo));
+	for (i = 0; i < ARRAY_SIZE(bundle_suffix); i++) {
+		const char *path;
+		path = mkpath("%s%s", repo, bundle_suffix[i]);
+		if (!stat(path, &st) && S_ISREG(st.st_mode)) {
+			*is_bundle = 1;
+			return xstrdup(make_absolute_path(path));
+		}
+	}
 
 	return NULL;
 }
 
-static char *guess_dir_name(const char *repo)
+static char *guess_dir_name(const char *repo, int is_bundle)
 {
 	const char *p, *start, *end, *limit;
 	int after_slash_or_colon;
 
 	/* Guess dir name from repository: strip trailing '/',
-	 * strip trailing '[:/]*git', strip leading '.*[/:]'. */
+	 * strip trailing '[:/]*.{git,bundle}', strip leading '.*[/:]'. */
 
 	after_slash_or_colon = 1;
 	limit = repo + strlen(repo);
 	start = repo;
 	end = limit;
 	for (p = repo; p < limit; p++) {
-		if (!prefixcmp(p, ".git")) {
+		const char *prefix = is_bundle ? ".bundle" : ".git";
+		if (!prefixcmp(p, prefix)) {
 			if (!after_slash_or_colon)
 				end = p;
-			p += 3;
+			p += strlen(prefix) - 1;
 		} else if (*p == '/' || *p == ':') {
 			if (end == limit)
 				end = p;
@@ -287,7 +297,7 @@ walk_objects(char *src, char *dest)
 }
 
 static const struct ref *
-clone_local(const char *src_repo, const char *dest_repo)
+clone_local(const char *src_repo, const char *dest_repo, int is_bundle)
 {
 	const struct ref *ret;
 	char src[PATH_MAX];
@@ -295,7 +305,9 @@ clone_local(const char *src_repo, const char *dest_repo)
 	struct remote *remote;
 	struct transport *transport;
 
-	if (option_shared) {
+	if (is_bundle)
+		; /* do nothing */
+	else if (option_shared) {
 		write_alternates_file(dest_repo, src_repo);
 	} else {
 		snprintf(src, PATH_MAX, "%s/objects", src_repo);
@@ -307,6 +319,8 @@ clone_local(const char *src_repo, const char *dest_repo)
 	remote = remote_get(src_repo);
 	transport = transport_get(remote, src_repo);
 	ret = transport_get_remote_refs(transport);
+	if (is_bundle && transport_fetch_refs(transport, (struct ref *)ret))
+		die ("Could not read bundle");
 	transport_disconnect(transport);
 	return ret;
 }
@@ -339,6 +353,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int use_local_hardlinks = 1;
 	int use_separate_remote = 1;
+	int is_bundle;
 	struct stat buf;
 	const char *repo, *work_tree, *git_dir;
 	char *path, *dir, *head, *ref_temp;
@@ -369,12 +384,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		option_origin = "origin";
 
 	repo = argv[0];
-	path = get_repo_path(repo);
+	path = get_repo_path(repo, &is_bundle);
 
 	if (argc == 2) {
 		dir = xstrdup(argv[1]);
 	} else {
-		dir = guess_dir_name(repo);
+		dir = guess_dir_name(repo, is_bundle);
 	}
 
 	if (!stat(dir, &buf))
@@ -429,7 +444,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		git_config_set("core.bare", "true");
 
 	if (path != NULL) {
-		refs = clone_local(path, git_dir);
+		refs = clone_local(path, git_dir, is_bundle);
 		repo = make_absolute_path(path);
 	} else {
 		struct remote *remote = remote_get(argv[0]);
-- 
1.5.4.3.446.gbe8932

Re: [PATCH] builtin clone: support bundles

[Daniel Barkalow]

On Sun, 2 Mar 2008, Johannes Schindelin wrote:

> 	Daniel, do you have a branch where you have your current version 
> 	of builting clone?  Mine is in "my-next" of 
> 	http://repo.or.cz/w/git/dscho.git.

I don't yet, and I've been meaning to post my latest, but haven't gotten 
to it. Once I get these two patches integrated, I'll put something up at:

git://iabervon.org/~barkalow/git.git builtin-clone

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] builtin clone: support bundles

[Daniel Barkalow]

On Sun, 2 Mar 2008, Johannes Schindelin wrote:

> +	for (i = 0; i < ARRAY_SIZE(bundle_suffix); i++) {
> +		const char *path;
> +		path = mkpath("%s%s", repo, bundle_suffix[i]);
> +		if (!stat(path, &st) && S_ISREG(st.st_mode)) {
> +			*is_bundle = 1;
> +			return xstrdup(make_absolute_path(path));

The problem I'm seeing in general is that origin/next's make_absolute_path 
doesn't work on a regular file in the current directory. How are you 
getting those tests to pass?

In any case, I've got my current version at

git://iabervon.org/~barkalow/git.git builtin-clone

The top patch is possibly the correct change for make_absolute_path(), and 
I've incorporated your changes (except that I'd reorganized a bunch of 
stuff already, making some of them unnecessary and doing some of them 
slightly differently). For example, I just make bundles not count as 
local, and let transport.c deal with them in the normal path.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] builtin clone: support bundles

[Johannes Schindelin]

Hi,

On Sun, 2 Mar 2008, Daniel Barkalow wrote:

> On Sun, 2 Mar 2008, Johannes Schindelin wrote:
> 
> > +	for (i = 0; i < ARRAY_SIZE(bundle_suffix); i++) {
> > +		const char *path;
> > +		path = mkpath("%s%s", repo, bundle_suffix[i]);
> > +		if (!stat(path, &st) && S_ISREG(st.st_mode)) {
> > +			*is_bundle = 1;
> > +			return xstrdup(make_absolute_path(path));
> 
> The problem I'm seeing in general is that origin/next's make_absolute_path 
> doesn't work on a regular file in the current directory. How are you 
> getting those tests to pass?

http://repo.or.cz/w/git/dscho.git?a=commitdiff;h=d066bd60e5a93d69c47318cf6e71f77c84b737e6

> In any case, I've got my current version at
> 
> git://iabervon.org/~barkalow/git.git builtin-clone

Thanks.

> The top patch is possibly the correct change for make_absolute_path(), 

Mine has a test now, too...

> and I've incorporated your changes (except that I'd reorganized a bunch 
> of stuff already, making some of them unnecessary and doing some of them 
> slightly differently). For example, I just make bundles not count as 
> local, and let transport.c deal with them in the normal path.

Makes sense.

Ciao,
Dscho

Re: [PATCH] builtin clone: support bundles

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Sun, 2 Mar 2008, Daniel Barkalow wrote:
>
>> On Sun, 2 Mar 2008, Johannes Schindelin wrote:
>> 
>> > +	for (i = 0; i < ARRAY_SIZE(bundle_suffix); i++) {
>> > +		const char *path;
>> > +		path = mkpath("%s%s", repo, bundle_suffix[i]);
>> > +		if (!stat(path, &st) && S_ISREG(st.st_mode)) {
>> > +			*is_bundle = 1;
>> > +			return xstrdup(make_absolute_path(path));
>> 
>> The problem I'm seeing in general is that origin/next's make_absolute_path 
>> doesn't work on a regular file in the current directory. How are you 
>> getting those tests to pass?
>
> http://repo.or.cz/w/git/dscho.git?a=commitdiff;h=d066bd60e5a93d69c47318cf6e71f77c84b737e6

FYI, I already queued this for 'master'.

Re: [PATCH] builtin clone: support bundles

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> http://repo.or.cz/w/git/dscho.git?a=commitdiff;h=d066bd60e5a93d69c47318cf6e71f77c84b737e6
>
> FYI, I already queued this for 'master'.

Clarification.

What I queued is _not_ the builtin-clone change, but the patch that your
gitweb URL shows, which is a fix for e5392c5 (Add is_absolute_path() and
make_absolute_path()).

Re: [PATCH] builtin clone: support bundles

[Santi Béjar]

On Sun, Mar 2, 2008 at 5:19 PM, Daniel Barkalow <barkalow@iabervon.org> wrote:
> On Sun, 2 Mar 2008, Johannes Schindelin wrote:
>
>  >       Daniel, do you have a branch where you have your current version
>  >       of builting clone?  Mine is in "my-next" of
>  >       http://repo.or.cz/w/git/dscho.git.
>
>  I don't yet, and I've been meaning to post my latest, but haven't gotten
>  to it. Once I get these two patches integrated, I'll put something up at:
>
>  git://iabervon.org/~barkalow/git.git builtin-clone

I've tested the bundle modifications in it and they work for me, so:

Tested-by: Santi Béjar <sbejar@gmail.com>

at least for the bundles part (022fc130). Thanks Johannes for the forward-port.

Santi

[PATCH] Add test for cloning with "--reference" repo being a subset of source repo

[Johan Herland]

The first test in this series tests "git clone -l -s --reference B A C",
where repo B is a superset of repo A (A has one commit, B has the same
commit plus another). In this case, all objects to be cloned are already
present in B.

However, we should also test the case where the "--reference" repo is a
_subset_ of the source repo (e.g. "git clone -l -s --reference A B C"),
i.e. some objects are not available in the "--reference" repo, and will
have to be found in the source repo.

Signed-off-by: Johan Herland <johan@herland.net>
---

On Sunday 02 March 2008, Daniel Barkalow wrote:
> In any case, I've got my current version at
> 
> git://iabervon.org/~barkalow/git.git builtin-clone

Thanks, it already looks much better than the initial version. :)

However, this added test currently fails for me with the following output:

repo is /home/johan/git/git/t/trash/B/.git
dir is E
Initialize E/.git
Initialized empty Git repository in E/.git/
Okay
Wrote /home/johan/git/git/t/trash/A/.git/objects
 to E/.git/objects/info/alternates
Wrote /home/johan/git/git/t/trash/B/.git/objects
 to E/.git/objects/info/alternates
Get for /home/johan/git/git/t/trash/B/.git
error: Trying to write ref refs/remotes/origin/master with nonexistant object 276cf9e94287a7c4e6f79b2724460e9650fa4871
fatal: Cannot update the ref 'refs/remotes/origin/master'.
Remove junk E/.git
Remove junk E

The same test work well with git-clone.sh.
Not sure what's going on here, yet, but I thought I'd give you a heads up.

...Johan


 t/t5700-clone-reference.sh |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index b6a5486..d318780 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -113,4 +113,9 @@ diff expected current'
 
 cd "$base_dir"
 
+test_expect_success 'cloning with reference being subset of source (-l -s)' \
+'git clone -l -s --reference A B E'
+
+cd "$base_dir"
+
 test_done
-- 
1.5.4.3.328.gcaed

Re: [PATCH] Add test for cloning with "--reference" repo being a subset of source repo

[Daniel Barkalow]

On Mon, 3 Mar 2008, Johan Herland wrote:

> However, we should also test the case where the "--reference" repo is a
> _subset_ of the source repo (e.g. "git clone -l -s --reference A B C"),
> i.e. some objects are not available in the "--reference" repo, and will
> have to be found in the source repo.
> 
> However, this added test currently fails for me with the following output:
> 
> repo is /home/johan/git/git/t/trash/B/.git
> dir is E
> Initialize E/.git
> Initialized empty Git repository in E/.git/
> Okay
> Wrote /home/johan/git/git/t/trash/A/.git/objects
>  to E/.git/objects/info/alternates
> Wrote /home/johan/git/git/t/trash/B/.git/objects
>  to E/.git/objects/info/alternates
> Get for /home/johan/git/git/t/trash/B/.git
> error: Trying to write ref refs/remotes/origin/master with nonexistant object 276cf9e94287a7c4e6f79b2724460e9650fa4871
> fatal: Cannot update the ref 'refs/remotes/origin/master'.
> Remove junk E/.git
> Remove junk E
> 
> The same test work well with git-clone.sh.
> Not sure what's going on here, yet, but I thought I'd give you a heads up.

Thanks for the report; I haven't really gone through the local clone 
stuff, and I've altered the use of chdir at various points, so it's quite 
possible that it's not right at all for some cases.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH, fixed] builtin-clone: create remotes/origin/HEAD symref, if guessed

[Kristian Høgsberg]

On Sun, 2008-03-02 at 06:25 +0000, Johannes Schindelin wrote:
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 
> 	Sorry, my previous patch was broken in so many ways.  This one
> 	is better, promise.
> 
> 	BTW this incidentally fixes the branch.<branch>.{remote,merge} 
> 	setup: it used to strip all up-to and including a slash from the 
> 	ref name.  This just _happened_ to work, because commonly HEAD is 
> 	at "refs/heads/master".  However, if it is at "refs/heads/a/b", it 
> 	would fail.
> 
>  builtin-clone.c |   35 ++++++++++++++++++++---------------
>  1 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/builtin-clone.c b/builtin-clone.c
> index 056e8a3..f27d205 100644
> --- a/builtin-clone.c
> +++ b/builtin-clone.c
> @@ -523,33 +523,38 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		git_config_set_multivar(key, value, "^$", 0);
>  	}
>  
> +	if (head_points_at)
> +		/* Local default branch */
> +		create_symref("HEAD", head_points_at->name, NULL);
> +
>  	if (option_bare) {
> -		if (head_points_at) {
> -			/* Local default branch */
> -			create_symref("HEAD", head_points_at->name, NULL);
> -		}
>  		junk_work_tree = NULL;
>  		junk_git_dir = NULL;
>  		return 0;
>  	}
>  
>  	if (head_points_at) {
> -		if (strrchr(head_points_at->name, '/'))
> -			head = strrchr(head_points_at->name, '/') + 1;
> -		else
> -			head = head_points_at->name;
> +		struct strbuf head_ref, real_ref;
>  
> -		/* Local default branch */
> -		create_symref("HEAD", head_points_at->name, NULL);
> +		head = head_points_at->name;
> +		if (!prefixcmp(head, "refs/heads/"))
> +			head += 11;
>  
>  		/* Tracking branch for the primary branch at the remote. */
>  		update_ref(NULL, "HEAD", head_points_at->old_sha1,
>  			   NULL, 0, DIE_ON_ERR);
> -	/*
> -		rm -f "refs/remotes/$origin/HEAD"
> -		git symbolic-ref "refs/remotes/$origin/HEAD" \
> -			"refs/remotes/$origin/$head_points_at" &&
> -	*/
> +
> +		strbuf_init(&head_ref, 0);
> +		strbuf_addstr(&head_ref, branch_top);
> +		strbuf_addstr(&head_ref, "/HEAD");
> +		delete_ref(head_ref.buf, head_points_at->old_sha1);
> +		strbuf_init(&real_ref, 0);
> +		strbuf_addstr(&real_ref, branch_top);
> +		strbuf_addch(&real_ref, '/');
> +		strbuf_addstr(&real_ref, head);

What about just using

  strbuf_addf(&real_ref, "%s/%s", branch_top, head);

Are you worried about performance? :-p

Oh and I'm wondering if

  strbuf_initf(&real_ref, "%s/%s", branch_top, head);

would be a worthwhile addition to the strbuf API...

Kristian

Re: [PATCH, fixed] builtin-clone: create remotes/origin/HEAD symref, if guessed

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1163 bytes --]

On Mon, Mar 03, 2008 at 05:05:13PM +0000, Kristian Høgsberg wrote:
> On Sun, 2008-03-02 at 06:25 +0000, Johannes Schindelin wrote:
> > +		strbuf_init(&head_ref, 0);
> > +		strbuf_addstr(&head_ref, branch_top);
> > +		strbuf_addstr(&head_ref, "/HEAD");
> > +		delete_ref(head_ref.buf, head_points_at->old_sha1);
> > +		strbuf_init(&real_ref, 0);
> > +		strbuf_addstr(&real_ref, branch_top);
> > +		strbuf_addch(&real_ref, '/');
> > +		strbuf_addstr(&real_ref, head);
> 
> What about just using
> 
>   strbuf_addf(&real_ref, "%s/%s", branch_top, head);
> 
> Are you worried about performance? :-p

  If he was he would have used strbuf_init(&real_ref, 1024) or sth like
that I assume :)

> Oh and I'm wondering if
> 
>   strbuf_initf(&real_ref, "%s/%s", branch_top, head);
> 
> would be a worthwhile addition to the strbuf API...

  I don't think so, unless there are 1289 places in git that would
benefit from the shortcut it gives, but I really doubt it.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH, fixed] builtin-clone: create remotes/origin/HEAD symref, if guessed

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3055 bytes --]

Hi,

On Mon, 3 Mar 2008, Kristian Høgsberg wrote:

> On Sun, 2008-03-02 at 06:25 +0000, Johannes Schindelin wrote:
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > 
> > 	Sorry, my previous patch was broken in so many ways.  This one
> > 	is better, promise.
> > 
> > 	BTW this incidentally fixes the branch.<branch>.{remote,merge} 
> > 	setup: it used to strip all up-to and including a slash from the 
> > 	ref name.  This just _happened_ to work, because commonly HEAD is 
> > 	at "refs/heads/master".  However, if it is at "refs/heads/a/b", it 
> > 	would fail.
> > 
> >  builtin-clone.c |   35 ++++++++++++++++++++---------------
> >  1 files changed, 20 insertions(+), 15 deletions(-)
> > 
> > diff --git a/builtin-clone.c b/builtin-clone.c
> > index 056e8a3..f27d205 100644
> > --- a/builtin-clone.c
> > +++ b/builtin-clone.c
> > @@ -523,33 +523,38 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> >  		git_config_set_multivar(key, value, "^$", 0);
> >  	}
> >  
> > +	if (head_points_at)
> > +		/* Local default branch */
> > +		create_symref("HEAD", head_points_at->name, NULL);
> > +
> >  	if (option_bare) {
> > -		if (head_points_at) {
> > -			/* Local default branch */
> > -			create_symref("HEAD", head_points_at->name, NULL);
> > -		}
> >  		junk_work_tree = NULL;
> >  		junk_git_dir = NULL;
> >  		return 0;
> >  	}
> >  
> >  	if (head_points_at) {
> > -		if (strrchr(head_points_at->name, '/'))
> > -			head = strrchr(head_points_at->name, '/') + 1;
> > -		else
> > -			head = head_points_at->name;
> > +		struct strbuf head_ref, real_ref;
> >  
> > -		/* Local default branch */
> > -		create_symref("HEAD", head_points_at->name, NULL);
> > +		head = head_points_at->name;
> > +		if (!prefixcmp(head, "refs/heads/"))
> > +			head += 11;
> >  
> >  		/* Tracking branch for the primary branch at the remote. */
> >  		update_ref(NULL, "HEAD", head_points_at->old_sha1,
> >  			   NULL, 0, DIE_ON_ERR);
> > -	/*
> > -		rm -f "refs/remotes/$origin/HEAD"
> > -		git symbolic-ref "refs/remotes/$origin/HEAD" \
> > -			"refs/remotes/$origin/$head_points_at" &&
> > -	*/
> > +
> > +		strbuf_init(&head_ref, 0);
> > +		strbuf_addstr(&head_ref, branch_top);
> > +		strbuf_addstr(&head_ref, "/HEAD");
> > +		delete_ref(head_ref.buf, head_points_at->old_sha1);
> > +		strbuf_init(&real_ref, 0);
> > +		strbuf_addstr(&real_ref, branch_top);
> > +		strbuf_addch(&real_ref, '/');
> > +		strbuf_addstr(&real_ref, head);
> 
> What about just using
> 
>   strbuf_addf(&real_ref, "%s/%s", branch_top, head);
> 
> Are you worried about performance? :-p

You know, just after sending, I thought the same.

> Oh and I'm wondering if
> 
>   strbuf_initf(&real_ref, "%s/%s", branch_top, head);
> 
> would be a worthwhile addition to the strbuf API...

And exactly this was crossing my mind, too, as

static inline int strbuf_initf(struct strbuf *buf, const char *format, ...)
{
	strbuf_init(buf, strlen(format));
	return strbuf_addf(format, ...);
}

(just a sketch, but you get the idea...)

Ciao,
Dscho

Re: [PATCH, fixed] builtin-clone: create remotes/origin/HEAD symref, if guessed

[Johan Herland]

On Monday 03 March 2008, Kristian Høgsberg wrote:
> Oh and I'm wondering if
> 
>   strbuf_initf(&real_ref, "%s/%s", branch_top, head);
> 
> would be a worthwhile addition to the strbuf API...

+1. This is about the first thing I started looking for in strbuf.h when I first looked at strbufs...


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH] Add test for cloning with "--reference" repo being a subset of source repo

[Daniel Barkalow]

On Mon, 3 Mar 2008, Johan Herland wrote:

> Not sure what's going on here, yet, but I thought I'd give you a heads up.

I figured it out, and pushed out a fix; it was doing everything correctly, 
but it wrote to the alternates files after the library had read that file, 
so it then didn't notice that it actually had the objects that are in the 
second alternate repository.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH, fixed] builtin-clone: create remotes/origin/HEAD symref, if guessed

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 549 bytes --]

Hi,

On Mon, 3 Mar 2008, Pierre Habouzit wrote:

> On Mon, Mar 03, 2008 at 05:05:13PM +0000, Kristian Høgsberg wrote:
> > Oh and I'm wondering if
> > 
> >   strbuf_initf(&real_ref, "%s/%s", branch_top, head);
> > 
> > would be a worthwhile addition to the strbuf API...
> 
>   I don't think so, unless there are 1289 places in git that would 
> benefit from the shortcut it gives, but I really doubt it.

I think the proper question is: how many places in Git would benefit from 
strbuf_initf()?

Well, I stated already that I like it.

Ciao,
Dscho

Re: [PATCH] Add test for cloning with "--reference" repo being a subset of source repo

[Johan Herland]

On Monday 03 March 2008, Daniel Barkalow wrote:
> On Mon, 3 Mar 2008, Johan Herland wrote:
> 
> > Not sure what's going on here, yet, but I thought I'd give you a heads up.
> 
> I figured it out, and pushed out a fix; it was doing everything correctly, 
> but it wrote to the alternates files after the library had read that file, 
> so it then didn't notice that it actually had the objects that are in the 
> second alternate repository.

Thanks. After looking a bit more at the original test repo where I found
this issue, I discovered another, similar bug. This one seems ugly; brace
yourself:

In some cases (I'm not exactly sure of all the preconditions) when
cloning with "--reference", it seems git tries to access a loose object
in the "--reference" repo instead of in the cloned repo, even if that
object is already present in the cloned repo and _missing_ in the
"--reference" repo. The symptom is this error message:
    error: Trying to write ref $ref with nonexistant object $sha1

After playing around with this in gdb, it seems the problem is all the
way down in sha1_file_name() (sha1_file.c). This function is responsible
for generating the loose object filename for a given $sha1. It keeps a
static char *base which is initially set to the object directory name,
and then calls fill_sha1_path() to copy the rest of the object filename
into the following bytes. On subsequent calls, only the fill_sha1_path()
part is done, thereby reusing the base from the previous invocation.

What I observe is that this base is not reset after accessing loose
objects in the "--reference" repo. Thus, later when accessing objects in
the cloned repo, sha1_file_name() generates incorrect filenames (pointing
to the "--reference" repo instead of the cloned repo).

Of course, this often goes undetected since the "--reference" repo often
have the same loose objects as the clone.

Unfortunately (from a builtin git-clone's POV) this seems to be
symptomatic of a deeper problem in this part of the code: Using
function-static variables as caches only works as far as the cache
is in sync with reality. Especially when switching between multiple
repositories within the same process, it seems that several of these
variables are left with invalid data in them. This needs to be fixed,
if not only for now, then at least as part of the libification effort.

I'm not sure what is the best way of fixing this issue; my initial guess
is to move these function-static variables out to file-level, and make
sure they're properly reset whenever the appropriate context is changed
(typically when set_git_dir() is called, I guess).

Here are the function-static variables I immediately found in sha1_file.c
(there may be more, both in sha1_file.c and in other files):
- sha1_file_name(): static char *base
- sha1_pack_name(): static char *base
- sha1_pack_index_name(): static char *base
- find_pack_entry(): static struct packed_git *last_found
  (not sure about this one)

I will follow up this email with two patches, one adding the failing test,
and one providing a simple fix for that specific test (although very much
insufficient as a fix for the actual issue described above).


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCH 1/2] Add test illustrating issues with sha1_file_name() and switching repos

[Johan Herland]

This test fails with the current iteration of builtin-clone.

After builtin-clone.c have finished processing the "--reference" option,
it switches (i.e. calls set_git_dir()) to the cloned repo. However, when
updating refs in the cloned repo, git is unable to find the (loose) objects
they point at due to the underlying plumbing generating incorrect filenames
for these loose objects (referring to non-existing files in the
"--reference" repo instead of existing files in the cloned repo).

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t5700-clone-reference.sh |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index d318780..40826ac 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -118,4 +118,25 @@ test_expect_success 'cloning with reference being subset of source (-l -s)' \
 
 cd "$base_dir"
 
+test_expect_success 'preparing alternate repository #1' \
+'test_create_repo F && cd F &&
+echo first > file1 &&
+git add file1 &&
+git commit -m initial'
+
+cd "$base_dir"
+
+test_expect_success 'cloning alternate repo #2 and adding changes to repo #1' \
+'git clone F G && cd F &&
+echo second > file2 &&
+git add file2 &&
+git commit -m addition'
+
+cd "$base_dir"
+
+test_expect_failure 'cloning alternate repo #1, using #2 as reference' \
+'git clone --reference G F H'
+
+cd "$base_dir"
+
 test_done
-- 
1.5.4.3.328.gcaed

[PATCH 2/2] Overly simplistic fix for issue with sha1_file_name() and switching repos

[Johan Herland]

This is not a final fix, just an illustration of how synchronizing the
"char *base" holding the object directory does in fact fix the test
added by the previous patch. A real fix will explicitly reset "base"
when needed, without comparing with get_object_directory() on every
invocation. A real fix will also have to deal with the other similar
issue in this (and possibly other) file(s).

Signed-off-by: Johan Herland <johan@herland.net>
---
 sha1_file.c                |    5 +++--
 t/t5700-clone-reference.sh |    2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 4ce4d9d..909226e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -161,9 +161,10 @@ char *sha1_file_name(const unsigned char *sha1)
 {
 	static char *name, *base;
 
-	if (!base) {
-		const char *sha1_file_directory = get_object_directory();
+	const char *sha1_file_directory = get_object_directory();
+	if (!base || prefixcmp(base, sha1_file_directory) != 0) {
 		int len = strlen(sha1_file_directory);
+		free(base);
 		base = xmalloc(len + 60);
 		memcpy(base, sha1_file_directory, len);
 		memset(base+len, 0, 60);
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index 40826ac..0c42d9f 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -134,7 +134,7 @@ git commit -m addition'
 
 cd "$base_dir"
 
-test_expect_failure 'cloning alternate repo #1, using #2 as reference' \
+test_expect_success 'cloning alternate repo #1, using #2 as reference' \
 'git clone --reference G F H'
 
 cd "$base_dir"
-- 
1.5.4.3.328.gcaed

Re: [PATCH] Add test for cloning with "--reference" repo being a subset of source repo

[Daniel Barkalow]

On Tue, 4 Mar 2008, Johan Herland wrote:

> On Monday 03 March 2008, Daniel Barkalow wrote:
> > On Mon, 3 Mar 2008, Johan Herland wrote:
> > 
> > > Not sure what's going on here, yet, but I thought I'd give you a heads up.
> > 
> > I figured it out, and pushed out a fix; it was doing everything correctly, 
> > but it wrote to the alternates files after the library had read that file, 
> > so it then didn't notice that it actually had the objects that are in the 
> > second alternate repository.
> 
> Thanks. After looking a bit more at the original test repo where I found
> this issue, I discovered another, similar bug. This one seems ugly; brace
> yourself:
> 
> In some cases (I'm not exactly sure of all the preconditions) when
> cloning with "--reference", it seems git tries to access a loose object
> in the "--reference" repo instead of in the cloned repo, even if that
> object is already present in the cloned repo and _missing_ in the
> "--reference" repo. The symptom is this error message:
>     error: Trying to write ref $ref with nonexistant object $sha1
> 
> After playing around with this in gdb, it seems the problem is all the
> way down in sha1_file_name() (sha1_file.c). This function is responsible
> for generating the loose object filename for a given $sha1. It keeps a
> static char *base which is initially set to the object directory name,
> and then calls fill_sha1_path() to copy the rest of the object filename
> into the following bytes. On subsequent calls, only the fill_sha1_path()
> part is done, thereby reusing the base from the previous invocation.
>
> What I observe is that this base is not reset after accessing loose
> objects in the "--reference" repo. Thus, later when accessing objects in
> the cloned repo, sha1_file_name() generates incorrect filenames (pointing
> to the "--reference" repo instead of the cloned repo).
> 
> Of course, this often goes undetected since the "--reference" repo often
> have the same loose objects as the clone.
> 
> Unfortunately (from a builtin git-clone's POV) this seems to be
> symptomatic of a deeper problem in this part of the code: Using
> function-static variables as caches only works as far as the cache
> is in sync with reality. Especially when switching between multiple
> repositories within the same process, it seems that several of these
> variables are left with invalid data in them. This needs to be fixed,
> if not only for now, then at least as part of the libification effort.
> 
> I'm not sure what is the best way of fixing this issue; my initial guess
> is to move these function-static variables out to file-level, and make
> sure they're properly reset whenever the appropriate context is changed
> (typically when set_git_dir() is called, I guess).

I think we should be able to avoid setting git_dir to anything other than 
the repo we're creating, which would avoid this problem for the present, 
although, as you say, it would be good to be able to switch around as 
instructed for libification purposes eventually.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Add test for cloning with "--reference" repo being a subset of source repo

[Daniel Barkalow]

On Tue, 4 Mar 2008, Daniel Barkalow wrote:

> I think we should be able to avoid setting git_dir to anything other than 
> the repo we're creating, which would avoid this problem for the present, 
> although, as you say, it would be good to be able to switch around as 
> instructed for libification purposes eventually.

Okay, stuff pushed out to not use git_dir to access the reference repo, 
and an additional test that requires that we actually note that we have 
the refs in the reference repository (because otherwise we could pass all 
the tests by just making --reference useless, but that's obviously no 
good).

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Add test for cloning with "--reference" repo being a subset of source repo

[Johan Herland]

On Wednesday 05 March 2008, Daniel Barkalow wrote:
> On Tue, 4 Mar 2008, Daniel Barkalow wrote:
> > I think we should be able to avoid setting git_dir to anything other than 
> > the repo we're creating, which would avoid this problem for the present, 
> > although, as you say, it would be good to be able to switch around as 
> > instructed for libification purposes eventually.
> 
> Okay, stuff pushed out to not use git_dir to access the reference repo, 
> and an additional test that requires that we actually note that we have 
> the refs in the reference repository (because otherwise we could pass all 
> the tests by just making --reference useless, but that's obviously no 
> good).

Thanks. Nice work. The testsuite passes, and it all looks good from here. :)


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net