[PATCH 0/1] git-clone: fix relative path problem in the alternates

[PATCH 0/1] git-clone: fix relative path problem in the alternates

[Hui Wang]

Hi Junio & everyone,

Several days ago, i reported a "git clone" issue, the url is at:
http://marc.info/?l=git&m=131219022632724&w=2

Here i describe the problem again and try to provide a fix for this
problem, please help to review it. Thanks.

The problem:
Suppose we have two git repositories on the local machine, one is
called base repository, another is called source repository, source
repository need refer to some objects of the base repository via
.git/objects/info/alternates. If we set a relative path (source repository
to base repository) in the alternates, we can execute any git commands (branch,
tag, pull, checkout...) successfully in the source repository. But if we
want to clone a destiantion repository from source repository without
"--shared" on the same machine, just like this:
%>git clone $local_path/src_repos $local_path/des_repos
This clone command will fail because git can't get objects of the base
repository.

The reason:
When we execute git clone $local_path/src_repos $local_path/des_repos,
git will copy alternates of the src_repos to the des_repos first, then
des_repos will use this alternates to refer to base repository, this is
the problem, the relative in the alternates is src_repos to base_repos
rather than des_repos to base_repos.

The fix:
After the alternates is copied from src_repos to des_repos and before this
file is used by the des_repos, we can parse it and replace all relative
paths to a corrected path (absolute path of base_repos or new relative path
of des_repos to base_repos). In my patch, i choose to replace relative paths
to absoulte paths of base_repos.

If we add "--shared", this problem doesn't exist becase git doesn't need to
copy alternates, if we add prefix before src_repos like "file://", "git://"...,
this problem also doesn't exist. So i add a function to replace relative paths
in the clone_local(), this only affect git clone $local_path/src_repos
$local_path/des_repos without "--shared" and it has replative paths in the
alternates of src_repos, for other situation, this function will not introduce
any changes. Below is a execution result example of my fix:

the content of the alternates in the source repos (4 lines):
$<relative_path1_source_repos_to_base1>
$<relative_path2_source_repos_to_base2>
# comment line
$<absolute_path_of_base3>

After clone from source repos, the content of the alternates in the des_repos (4 lines):
$<absolute_path_of_base1>
$<absolute_path_of_base2>
# comment line
$<absolute_path_of_base3> 

[PATCH 1/1] clone: replace relative paths in the alternates

[Hui Wang]

When we clone a local source repository to a new local destination
repository without "--shared" option, if source repository has
relative path in the alternates, the clone will fail.

The root cause is when cloning local source repository to local
destination repository, the alternates of the source repository
will be copied to the destination repository, the relative path in
the alternates is source repository to base repository rather than
destination repository to base repository, so the destination
repository get a wrong relative path in the alternates, this will
cause errors when refer to base repository in the destination
repository.

To fix it, adding a function update_alt_rel_path() to replace all
relative paths to absolute paths in the alternates of the destination
repository before destination repository use those paths to look for
the base repository.

update_alt_rel_path() is referred to link_alt_odb_entries() in the
sha1_file.c, update_alternates_file() is referred to
add_to_alternates_file() in the sha1_file.c.

Signed-off-by: Hui Wang <jason77.wang@gmail.com>
---
 builtin/clone.c |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 cache.h         |    1 +
 sha1_file.c     |   11 +++++++
 3 files changed, 95 insertions(+), 1 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 7663bc2..65c1eaf 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -286,6 +286,87 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest)
 	closedir(dir);
 }
 
+#define linesize(end, start) (end - start + 1)
+/*
+ * This function is called by clone_local() of clone.c
+ * If alternates in the src_repo (source repository) has relative paths,
+ * call this function will replace the relative paths to absolute paths
+ * in the des_repo (destination repository) alternates.
+ */
+static void update_alt_rel_path(const char *src_repo)
+{
+	void *map;
+	size_t mapsz;
+	char *cp, *last, *ep, sep = '\n';
+	int fd, buf_start = 0, needwrite = 0;
+	char *tmpbuf;
+	struct stat st;
+
+	fd = open(git_path("objects/info/alternates"), O_RDONLY);
+	if (fd < 0)
+		return;
+
+	if (fstat(fd, &st) || (st.st_size == 0)) {
+		close(fd);
+		return;
+	}
+
+	mapsz = xsize_t(st.st_size);
+	map = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, fd, 0);
+	close(fd);
+
+	tmpbuf = mapsz > 4096 ? xmalloc(mapsz * 2) : xmalloc(mapsz + 4096);
+
+	ep = map + mapsz;
+	last = map;
+	buf_start = 0;
+	while (last < ep) {
+		cp = last;
+		if (cp < ep && *cp == '#') {
+			while (cp < ep && *cp != sep)
+				cp++;
+			memcpy(tmpbuf + buf_start, last, linesize(cp, last));
+			buf_start += linesize(cp, last);
+			last = cp + 1;
+			continue;
+		}
+		while (cp < ep && *cp != sep)
+			cp++;
+		if (last != cp) {
+			if (!is_absolute_path(last)) {
+				char *abs_path, *abs_buf;
+				int len;
+
+				needwrite = 1;
+				memcpy(tmpbuf + buf_start, last, linesize(cp, last));
+				tmpbuf[buf_start + linesize(cp, last)] = '\0';
+				abs_path = mkpath("%s/objects/%s", src_repo, tmpbuf + buf_start);
+				abs_buf = xmalloc(PATH_MAX);
+				normalize_path_copy(abs_buf, abs_path);
+				len = strlen(abs_buf);
+				memcpy(tmpbuf + buf_start, abs_buf, len);
+				free(abs_buf);
+				buf_start += len;
+			} else {
+				memcpy(tmpbuf + buf_start, last, linesize(cp, last));
+				buf_start += linesize(cp, last);
+			}
+		}
+		while (cp < ep && *cp == sep)
+			cp++;
+		last = cp;
+	}
+	tmpbuf[buf_start] = '\0';
+
+	if (needwrite)
+		update_alternates_file_content(tmpbuf);
+
+	munmap(map, mapsz);
+	free(tmpbuf);
+
+	return;
+}
+
 static const struct ref *clone_local(const char *src_repo,
 				     const char *dest_repo)
 {
@@ -303,8 +384,9 @@ static const struct ref *clone_local(const char *src_repo,
 		copy_or_link_directory(&src, &dest);
 		strbuf_release(&src);
 		strbuf_release(&dest);
-	}
 
+		update_alt_rel_path(src_repo);
+	}
 	remote = remote_get(src_repo);
 	transport = transport_get(remote, src_repo);
 	ret = transport_get_remote_refs(transport);
diff --git a/cache.h b/cache.h
index fcf4501..d81c0f0 100644
--- a/cache.h
+++ b/cache.h
@@ -913,6 +913,7 @@ extern struct alternate_object_database {
 } *alt_odb_list;
 extern void prepare_alt_odb(void);
 extern void add_to_alternates_file(const char *reference);
+extern void update_alternates_file_content(const char *content);
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
 extern void foreach_alt_odb(alt_odb_fn, void*);
 
diff --git a/sha1_file.c b/sha1_file.c
index d5616dc..dae7bf6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -388,6 +388,17 @@ void add_to_alternates_file(const char *reference)
 		link_alt_odb_entries(alt, alt + strlen(alt), '\n', NULL, 0);
 }
 
+void update_alternates_file_content(const char *content)
+{
+	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+	int fd = hold_lock_file_for_update(lock, git_path("objects/info/alternates"), LOCK_DIE_ON_ERROR);
+	write_or_die(fd, content, strlen(content));
+	if (commit_lock_file(lock))
+		die("could not close alternates file");
+	if (alt_odb_tail)
+		link_alt_odb_entries(content, content + strlen(content), '\n', NULL, 0);
+}
+
 void foreach_alt_odb(alt_odb_fn fn, void *cb)
 {
 	struct alternate_object_database *ent;
-- 
1.7.6

Re: [PATCH 1/1] clone: replace relative paths in the alternates

[Junio C Hamano]

Hui Wang <jason77.wang@gmail.com> writes:

> +	mapsz = xsize_t(st.st_size);
> +	map = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, fd, 0);
> +	close(fd);
> +
> +	tmpbuf = mapsz > 4096 ? xmalloc(mapsz * 2) : xmalloc(mapsz + 4096);

Where do these magic numbers come from, and what guarantee do we have that
these magic numbers give safe upper bound for pathnames that are expanded
to be absolute?

Wouldn't it be a lot cleaner and much less error prone if you open the
original, read from it with strbuf_readline(), convert the path using
strbuf manipulation functions and write the resulting line out to the
lockfile you obtained to update it, line by line?

Re: [PATCH 1/1] clone: replace relative paths in the alternates

[Hui Wang]

Junio C Hamano wrote:
> Hui Wang <jason77.wang@gmail.com> writes:
>
>   
>> +	mapsz = xsize_t(st.st_size);
>> +	map = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, fd, 0);
>> +	close(fd);
>> +
>> +	tmpbuf = mapsz > 4096 ? xmalloc(mapsz * 2) : xmalloc(mapsz + 4096);
>>     
>
> Where do these magic numbers come from, and what guarantee do we have that
> these magic numbers give safe upper bound for pathnames that are expanded
> to be absolute?
>
>   
If an alternates file has dozens of relative path lines, this code has 
great risk.
> Wouldn't it be a lot cleaner and much less error prone if you open the
> original, read from it with strbuf_readline(), convert the path using
> strbuf manipulation functions and write the resulting line out to the
> lockfile you obtained to update it, line by line?
>   
This is a good suggestion, i will try to use the above way to prepare a 
V2 patch.

Thanks,
Jason.