{"thread":{"id":"13823","subject":"[PATCH] Use nonrelative paths instead of absolute paths for cloned repositories","startedAt":"2008-06-06T03:15:19Z","lastAt":"2008-06-06T18:07:19Z","messageCount":6,"participants":["Daniel Barkalow","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"78855","messageId":"alpine.LNX.1.00.0806052244300.19665@iabervon.org","threadId":"13823","inReplyTo":null,"subject":"[PATCH] Use nonrelative paths instead of absolute paths for cloned repositories","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-06-06T03:15:19Z","receivedAt":"2008-06-06T03:15:19Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"Particularly for the \"alternates\" file, if one will be created, we\nwant a path that doesn't depend on the current directory, but we want\nto retain any symlinks in the path as given and any in the user's view\nof the current directory when the path was given.\n\nSigned-off-by: Daniel Barkalow <barkalow@iabervon.org>\n---\nLightly tested; I haven't put together a test cases for the problem that \nstarted this, though.\n\n builtin-clone.c |    4 ++--\n cache.h         |    1 +\n path.c          |   34 ++++++++++++++++++++++++++++++++++\n 3 files changed, 37 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex f4accbe..7190952 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -76,7 +76,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n \t\tpath = mkpath(\"%s%s\", repo, suffix[i]);\n \t\tif (!stat(path, &st) && S_ISDIR(st.st_mode)) {\n \t\t\t*is_bundle = 0;\n-\t\t\treturn xstrdup(make_absolute_path(path));\n+\t\t\treturn xstrdup(make_nonrelative_path(path));\n \t\t}\n \t}\n \n@@ -85,7 +85,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)\n \t\tpath = mkpath(\"%s%s\", repo, bundle_suffix[i]);\n \t\tif (!stat(path, &st) && S_ISREG(st.st_mode)) {\n \t\t\t*is_bundle = 1;\n-\t\t\treturn xstrdup(make_absolute_path(path));\n+\t\t\treturn xstrdup(make_nonrelative_path(path));\n \t\t}\n \t}\n \ndiff --git a/cache.h b/cache.h\nindex 092a997..0a63c0e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -524,6 +524,7 @@ static inline int is_absolute_path(const char *path)\n \treturn path[0] == '/';\n }\n const char *make_absolute_path(const char *path);\n+const char *make_nonrelative_path(const char *path);\n \n /* Read and unpack a sha1 file into memory, write memory to a sha1 file */\n extern int sha1_object_info(const unsigned char *, unsigned long *);\ndiff --git a/path.c b/path.c\nindex b7c24a2..60b55ae 100644\n--- a/path.c\n+++ b/path.c\n@@ -291,6 +291,40 @@ int adjust_shared_perm(const char *path)\n \treturn 0;\n }\n \n+static const char *get_pwd_cwd(void)\n+{\n+\tstatic char cwd[PATH_MAX + 1];\n+\tchar *pwd;\n+\tstruct stat cwd_stat, pwd_stat;\n+\tif (getcwd(cwd, PATH_MAX) == NULL)\n+\t\treturn NULL;\n+\tpwd = getenv(\"PWD\");\n+\tif (pwd && strcmp(pwd, cwd)) {\n+\t\tstat(cwd, &cwd_stat);\n+\t\tif (!stat(pwd, &pwd_stat) &&\n+\t\t    pwd_stat.st_dev == cwd_stat.st_dev &&\n+\t\t    pwd_stat.st_ino == cwd_stat.st_ino) {\n+\t\t\tstrlcpy(cwd, pwd, PATH_MAX);\n+\t\t}\n+\t}\n+\treturn cwd;\n+}\n+\n+const char *make_nonrelative_path(const char *path)\n+{\n+\tstatic char buf[PATH_MAX + 1];\n+\n+\tif (path[0] == '/') {\n+\t\tif (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)\n+\t\t\tdie (\"Too long path: %.*s\", 60, path);\n+\t} else {\n+\t\tconst char *cwd = get_pwd_cwd();\n+\t\tif (!cwd || snprintf(buf, PATH_MAX, \"%s/%s\", cwd, path) > PATH_MAX)\n+\t\t\tdie (\"Too long path: %.*s\", 60, path);\n+\t}\n+\treturn buf;\n+}\n+\n /* We allow \"recursive\" symbolic links. Only within reason, though. */\n #define MAXDEPTH 5\n \n-- \n1.5.4.3.610.gea6cd\n"},{"id":"78856","messageId":"alpine.DEB.1.00.0806060422310.21190@racer","threadId":"13823","inReplyTo":"alpine.LNX.1.00.0806052244300.19665@iabervon.org","subject":"Re: [PATCH] Use nonrelative paths instead of absolute paths for cloned repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-06T03:23:01Z","receivedAt":"2008-06-06T03:23:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 5 Jun 2008, Daniel Barkalow wrote:\n\n> Particularly for the \"alternates\" file, if one will be created, we want \n> a path that doesn't depend on the current directory, but we want to \n> retain any symlinks in the path as given and any in the user's view of \n> the current directory when the path was given.\n\nI have to say that I do not follow why the symlinks should be trusted any \nmore than the absolute paths.\n\nCiao,\nDscho\n"},{"id":"78857","messageId":"alpine.LNX.1.00.0806052327160.19665@iabervon.org","threadId":"13823","inReplyTo":"alpine.DEB.1.00.0806060422310.21190@racer","subject":"Re: [PATCH] Use nonrelative paths instead of absolute paths for cloned repositories","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-06-06T03:47:13Z","receivedAt":"2008-06-06T03:47:13Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 6 Jun 2008, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Thu, 5 Jun 2008, Daniel Barkalow wrote:\n> \n> > Particularly for the \"alternates\" file, if one will be created, we want \n> > a path that doesn't depend on the current directory, but we want to \n> > retain any symlinks in the path as given and any in the user's view of \n> > the current directory when the path was given.\n> \n> I have to say that I do not follow why the symlinks should be trusted any \n> more than the absolute paths.\n\nIt's not so much a case of trusting symlinks more, but trusting the \nversion of the path that the user used more.\n\nThis came up because kernel.org has HTTP service arranged such that \n/pub/... is served at http://kernel.org/pub/..., but /pub is a symlink to \n/home/ftp/pub/, and http://kernel.org/home/ftp/pub/... doesn't work. \nClever people like Greg use \"git clone -s /pub/.../linux-2.6.git\" in order \nto share Linus's object storage, and this generated an HTTP-clonable repo \nwith git-clone.sh, because it didn't expand symlinks, but the builtin \nversion, without this patch, would expand them into the thing that doesn't \nwork.\n\nIf there's a difference between using the symlink and using the thing it \nlinks to, probably the user picked the right thing, or wouldn't be too \nsurprised that git doesn't come up with something better.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"78929","messageId":"7viqwmv7ff.fsf@gitster.siamese.dyndns.org","threadId":"13823","inReplyTo":"alpine.DEB.1.00.0806060422310.21190@racer","subject":"Re: [PATCH] Use nonrelative paths instead of absolute paths for cloned repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-06T16:45:24Z","receivedAt":"2008-06-06T16:45:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Thu, 5 Jun 2008, Daniel Barkalow wrote:\n>\n>> Particularly for the \"alternates\" file, if one will be created, we want \n>> a path that doesn't depend on the current directory, but we want to \n>> retain any symlinks in the path as given and any in the user's view of \n>> the current directory when the path was given.\n>\n> I have to say that I do not follow why the symlinks should be trusted any \n> more than the absolute paths.\n\nThanks, Daniel.\n\nI am obviously in favor of fixing this regression before 1.5.6, and the\nintroduction of make_nonrelative_path() for the use of clone alone is\nprobably the least impact and safe solution in the short term after -rc1.\n\nIn the longer term, we would inevitably face \"when should one use\nnonrelative and when should one use absolute?\" and we would eventually\nhave to answer it.  It may turn out that many current users of \"absolute\"\nare better off using \"nonrelative\", but I suspect we won't get rid of\n\"absolute\" completely, because one of the reasons it avoids symlinks at\ngreat lengths is so that it can check the containment relationships\nbetween paths reliably (e.g. \"is this path outside the repository, in\nwhich case we should refuse to add it to the index, and we use --no-index\nwithout being asked when running \"diff\"\").\n\nBut using \"absolute\" for containment comparison is one thing.  Storing the\nresult of \"absolute\" is quite another.\n\nWe've already learned to love a similar crazyness somebody's system has on\nthis list.  If you want to check if the user string is equivalent to a\npath you stored in the filesystem, you compare them after normalizing and\nthat is a sensible approach.  Storing path after normalizing, which would\nbe different form than what the user originally used to create, is not so\nsane and leads to all sorts of pain.  Ending up storing the output from\n\"absolute\" in info/alternates is just like giving normalized sequence back\nfrom readdir(3) on such a system.\n"},{"id":"78934","messageId":"alpine.DEB.1.00.0806061832330.1783@racer","threadId":"13823","inReplyTo":"7viqwmv7ff.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Use nonrelative paths instead of absolute paths for cloned repositories","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-06T17:34:13Z","receivedAt":"2008-06-06T17:34:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 6 Jun 2008, Junio C Hamano wrote:\n\n> In the longer term, we would inevitably face \"when should one use \n> nonrelative and when should one use absolute?\" and we would eventually \n> have to answer it.  It may turn out that many current users of \n> \"absolute\" are better off using \"nonrelative\", but I suspect we won't \n> get rid of \"absolute\" completely, because one of the reasons it avoids \n> symlinks at great lengths is so that it can check the containment \n> relationships between paths reliably (e.g. \"is this path outside the \n> repository, in which case we should refuse to add it to the index, and \n> we use --no-index without being asked when running \"diff\"\").\n> \n> But using \"absolute\" for containment comparison is one thing.  Storing \n> the result of \"absolute\" is quite another.\n\nThe easy way would be to add an option to make_absolute_path(), say \n\"resolve_symlinks\".\n\nCiao,\nDscho\n"},{"id":"78936","messageId":"7v7id2v3mw.fsf@gitster.siamese.dyndns.org","threadId":"13823","inReplyTo":"alpine.DEB.1.00.0806061832330.1783@racer","subject":"Re: [PATCH] Use nonrelative paths instead of absolute paths for cloned repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-06T18:07:19Z","receivedAt":"2008-06-06T18:07:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> In the longer term, we would inevitably face \"when should one use \n>> nonrelative and when should one use absolute?\" and we would eventually \n>> have to answer it.  It may turn out that many current users of \n>> \"absolute\" are better off using \"nonrelative\", but I suspect we won't \n>> get rid of \"absolute\" completely, because one of the reasons it avoids \n>> symlinks at great lengths is so that it can check the containment \n>> relationships between paths reliably (e.g. \"is this path outside the \n>> repository, in which case we should refuse to add it to the index, and \n>> we use --no-index without being asked when running \"diff\"\").\n>> \n>> But using \"absolute\" for containment comparison is one thing.  Storing \n>> the result of \"absolute\" is quite another.\n>\n> The easy way would be to add an option to make_absolute_path(), say \n> \"resolve_symlinks\".\n\nI am afraid that it does not solve anything.\n\nBe they two separate functions, or a one function that has two different\nsemantics depending on an option, the API documentation needs to answer\nthe \"when should I use one and when should I use the other\" question.\n\nAnd the hard part is figuring out which of the current \"absolute\" callers\nneed to be fixed in a way similar to how Daniel fixed git-clone, and which\nof them stay the same.  Perhaps all of the \"chdir then getpwd\" patterns\nneed to be looked at and some of them need to be restructured to honor $PWD\nbetter.  I dunno.\n"}]}