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

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

[Daniel Barkalow]

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

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

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

[Johannes Schindelin]

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

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

[Daniel Barkalow]

On Fri, 6 Jun 2008, Johannes Schindelin wrote:

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

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

[Junio C Hamano]

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

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

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

[Johannes Schindelin]

Hi,

On Fri, 6 Jun 2008, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

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

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