threads / patch / 13823

patchUse nonrelative paths instead of absolute paths for cloned repositories

Subject: [PATCH] Use nonrelative paths instead of absolute paths for cloned repositories

## tl;dr

6 messages between Jun 6, 2008 and Jun 6, 2008. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Daniel Barkalow· Jun 6, 2008, 03:15 UTC · lore

Particularly for the "alternates" file, if one will be created, we want a path that doesn't depend on the current directory, but we want to retain any symlinks in the path as given and any in the user's view of the current directory when the path was given.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
Lightly tested; I haven't put together a test cases for the problem that 
started this, though.
 builtin-clone.c |    4 ++--
 cache.h         |    1 +
 path.c          |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)
Show changes to 3 files +37 −2

builtin-clone.c, cache.h, path.c

diff --git a/builtin-clone.c b/builtin-clone.c
index f4accbe..7190952 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -76,7 +76,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 		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));
+			return xstrdup(make_nonrelative_path(path));
 		}
 	}
 
@@ -85,7 +85,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
 		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 xstrdup(make_nonrelative_path(path));
 		}
 	}
 
diff --git a/cache.h b/cache.h
index 092a997..0a63c0e 100644
--- a/cache.h
+++ b/cache.h
@@ -524,6 +524,7 @@ static inline int is_absolute_path(const char *path)
 	return path[0] == '/';
 }
 const char *make_absolute_path(const char *path);
+const char *make_nonrelative_path(const char *path);
 
 /* Read and unpack a sha1 file into memory, write memory to a sha1 file */
 extern int sha1_object_info(const unsigned char *, unsigned long *);
diff --git a/path.c b/path.c
index b7c24a2..60b55ae 100644
--- a/path.c
+++ b/path.c
@@ -291,6 +291,40 @@ int adjust_shared_perm(const char *path)
 	return 0;
 }
 
+static const char *get_pwd_cwd(void)
+{
+	static char cwd[PATH_MAX + 1];
+	char *pwd;
+	struct stat cwd_stat, pwd_stat;
+	if (getcwd(cwd, PATH_MAX) == NULL)
+		return NULL;
+	pwd = getenv("PWD");
+	if (pwd && strcmp(pwd, cwd)) {
+		stat(cwd, &cwd_stat);
+		if (!stat(pwd, &pwd_stat) &&
+		    pwd_stat.st_dev == cwd_stat.st_dev &&
+		    pwd_stat.st_ino == cwd_stat.st_ino) {
+			strlcpy(cwd, pwd, PATH_MAX);
+		}
+	}
+	return cwd;
+}
+
+const char *make_nonrelative_path(const char *path)
+{
+	static char buf[PATH_MAX + 1];
+
+	if (path[0] == '/') {
+		if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
+			die ("Too long path: %.*s", 60, path);
+	} else {
+		const char *cwd = get_pwd_cwd();
+		if (!cwd || snprintf(buf, PATH_MAX, "%s/%s", cwd, path) > PATH_MAX)
+			die ("Too long path: %.*s", 60, path);
+	}
+	return buf;
+}
+
 /* We allow "recursive" symbolic links. Only within reason, though. */
 #define MAXDEPTH 5
 

-- 
1.5.4.3.610.gea6cd
Johannes Schindelin· Jun 6, 2008, 03:23 UTC · re: Daniel Barkalow · lore

Re: [PATCH] Use nonrelative paths instead of absolute paths for cloned repositories

Hi,
On Thu, 5 Jun 2008, Daniel Barkalow wrote:
> Particularly for the "alternates" file, if one will be created, we want 
> a path that doesn't depend on the current directory, but we want to 
> retain any symlinks in the path as given and any in the user's view of 
> the current directory when the path was given.

I have to say that I do not follow why the symlinks should be trusted any more than the absolute paths.

Ciao, Dscho

Daniel Barkalow· Jun 6, 2008, 03:47 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Use nonrelative paths instead of absolute paths for cloned repositories

On Fri, 6 Jun 2008, Johannes Schindelin wrote:
Show 11 quoted lines
> Hi,
> 
> On Thu, 5 Jun 2008, Daniel Barkalow wrote:
> 
> > Particularly for the "alternates" file, if one will be created, we want 
> > a path that doesn't depend on the current directory, but we want to 
> > retain any symlinks in the path as given and any in the user's view of 
> > the current directory when the path was given.
> 
> I have to say that I do not follow why the symlinks should be trusted any 
> more than the absolute paths.

It's not so much a case of trusting symlinks more, but trusting the version of the path that the user used more.

This came up because kernel.org has HTTP service arranged such that /pub/... is served at http://kernel.org/pub/..., but /pub is a symlink to /home/ftp/pub/, and http://kernel.org/home/ftp/pub/... doesn't work. Clever people like Greg use "git clone -s /pub/.../linux-2.6.git" in order to share Linus's object storage, and this generated an HTTP-clonable repo with git-clone.sh, because it didn't expand symlinks, but the builtin version, without this patch, would expand them into the thing that doesn't work.

If there's a difference between using the symlink and using the thing it links to, probably the user picked the right thing, or wouldn't be too surprised that git doesn't come up with something better.

	-Daniel
*This .sig left intentionally blank*
Junio C Hamano· Jun 6, 2008, 16:45 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Use nonrelative paths instead of absolute paths for cloned repositories

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 9 quoted lines
> On Thu, 5 Jun 2008, Daniel Barkalow wrote:
>
>> Particularly for the "alternates" file, if one will be created, we want 
>> a path that doesn't depend on the current directory, but we want to 
>> retain any symlinks in the path as given and any in the user's view of 
>> the current directory when the path was given.
>
> I have to say that I do not follow why the symlinks should be trusted any 
> more than the absolute paths.
Thanks, Daniel.

I am obviously in favor of fixing this regression before 1.5.6, and the introduction of make_nonrelative_path() for the use of clone alone is probably the least impact and safe solution in the short term after -rc1.

In the longer term, we would inevitably face "when should one use nonrelative and when should one use absolute?" and we would eventually have to answer it. It may turn out that many current users of "absolute" are better off using "nonrelative", but I suspect we won't get rid of "absolute" completely, because one of the reasons it avoids symlinks at great lengths is so that it can check the containment relationships between paths reliably (e.g. "is this path outside the repository, in which case we should refuse to add it to the index, and we use --no-index without being asked when running "diff"").

But using "absolute" for containment comparison is one thing. Storing the result of "absolute" is quite another.

We've already learned to love a similar crazyness somebody's system has on this list. If you want to check if the user string is equivalent to a path you stored in the filesystem, you compare them after normalizing and that is a sensible approach. Storing path after normalizing, which would be different form than what the user originally used to create, is not so sane and leads to all sorts of pain. Ending up storing the output from "absolute" in info/alternates is just like giving normalized sequence back from readdir(3) on such a system.

Johannes Schindelin· Jun 6, 2008, 17:34 UTC · re: Junio C Hamano · lore

Re: [PATCH] Use nonrelative paths instead of absolute paths for cloned repositories

Hi,
On Fri, 6 Jun 2008, Junio C Hamano wrote:
Show 12 quoted lines
> In the longer term, we would inevitably face "when should one use 
> nonrelative and when should one use absolute?" and we would eventually 
> have to answer it.  It may turn out that many current users of 
> "absolute" are better off using "nonrelative", but I suspect we won't 
> get rid of "absolute" completely, because one of the reasons it avoids 
> symlinks at great lengths is so that it can check the containment 
> relationships between paths reliably (e.g. "is this path outside the 
> repository, in which case we should refuse to add it to the index, and 
> we use --no-index without being asked when running "diff"").
> 
> But using "absolute" for containment comparison is one thing.  Storing 
> the result of "absolute" is quite another.

The easy way would be to add an option to make_absolute_path(), say "resolve_symlinks".

Ciao, Dscho

Junio C Hamano· Jun 6, 2008, 18:07 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Use nonrelative paths instead of absolute paths for cloned repositories

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 15 quoted lines
>> In the longer term, we would inevitably face "when should one use 
>> nonrelative and when should one use absolute?" and we would eventually 
>> have to answer it.  It may turn out that many current users of 
>> "absolute" are better off using "nonrelative", but I suspect we won't 
>> get rid of "absolute" completely, because one of the reasons it avoids 
>> symlinks at great lengths is so that it can check the containment 
>> relationships between paths reliably (e.g. "is this path outside the 
>> repository, in which case we should refuse to add it to the index, and 
>> we use --no-index without being asked when running "diff"").
>> 
>> But using "absolute" for containment comparison is one thing.  Storing 
>> the result of "absolute" is quite another.
>
> The easy way would be to add an option to make_absolute_path(), say 
> "resolve_symlinks".
I am afraid that it does not solve anything.

Be they two separate functions, or a one function that has two different semantics depending on an option, the API documentation needs to answer the "when should I use one and when should I use the other" question.

And the hard part is figuring out which of the current "absolute" callers need to be fixed in a way similar to how Daniel fixed git-clone, and which of them stay the same. Perhaps all of the "chdir then getpwd" patterns need to be looked at and some of them need to be restructured to honor $PWD better. I dunno.

← back to recent threads