{"thread":{"id":"22329","subject":"[PATCH] Handle double slashes in make_relative_path()","startedAt":"2010-01-22T00:07:31Z","lastAt":"2010-01-25T01:06:48Z","messageCount":20,"participants":["Thomas Rast","Junio C Hamano","Johannes Sixt","Robin Rosenberg","Sverre Rabbelier","Bernhard R. Link"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"132341","messageId":"379d55c6a4110736aadb8ace3b050de879a9deab.1264118830.git.trast@student.ethz.ch","threadId":"22329","inReplyTo":null,"subject":"[PATCH] Handle double slashes in make_relative_path()","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-01-22T00:07:31Z","receivedAt":"2010-01-22T00:07:31Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"If you say\n\n  git --git-dir=/some//path --work-tree=/some/ add <somefile>\n\nthen setup_work_tree() will call into make_relative_path() with\nabs=\"/some//path\" and base=\"/some\".  (Note how the latter has already\nlost its trailing slash.  One unfortunate user managed to trigger this\nbecause his $HOME ended in a slash.)\n\nThis means that when checking whether 'abs' is a path under 'base', we\nneed to skip *two* slashes where the previous code only accounted for\none.  Fix it to handle an arbitrary number of slashes at that\nposition.\n\nNoticed-by: eldenz on freenode\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n path.c              |    6 +++---\n t/t1501-worktree.sh |    6 ++++++\n 2 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 2ec950b..a195bab 100644\n--- a/path.c\n+++ b/path.c\n@@ -400,10 +400,10 @@ int set_shared_perm(const char *path, int mode)\n \tbaselen = strlen(base);\n \tif (prefixcmp(abs, base))\n \t\treturn abs;\n-\tif (abs[baselen] == '/')\n-\t\tbaselen++;\n-\telse if (base[baselen - 1] != '/')\n+\tif (abs[baselen] != '/' && base[baselen - 1] != '/')\n \t\treturn abs;\n+\twhile (abs[baselen] == '/')\n+\t\tbaselen++;\n \tstrcpy(buf, abs + baselen);\n \treturn buf;\n }\ndiff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh\nindex 74e6443..9df3012 100755\n--- a/t/t1501-worktree.sh\n+++ b/t/t1501-worktree.sh\n@@ -189,4 +189,10 @@ test_expect_success 'absolute pathspec should fail gracefully' '\n \t)\n '\n \n+test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '\n+\t: > dummy_file\n+\techo git --git-dir=\"$(pwd)//repo.git\" --work-tree=\"$(pwd)\" add dummy_file &&\n+\tgit --git-dir=\"$(pwd)//repo.git\" --work-tree=\"$(pwd)\" add dummy_file\n+'\n+\n test_done\n-- \n1.6.6.1.532.g594fe\n"},{"id":"132355","messageId":"7vpr52gbmu.fsf@alter.siamese.dyndns.org","threadId":"22329","inReplyTo":"379d55c6a4110736aadb8ace3b050de879a9deab.1264118830.git.trast@student.ethz.ch","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-22T01:40:41Z","receivedAt":"2010-01-22T01:40:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> diff --git a/path.c b/path.c\n> index 2ec950b..a195bab 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -400,10 +400,10 @@ int set_shared_perm(const char *path, int mode)\n>  \tbaselen = strlen(base);\n>  \tif (prefixcmp(abs, base))\n>  \t\treturn abs;\n> -\tif (abs[baselen] == '/')\n> -\t\tbaselen++;\n> -\telse if (base[baselen - 1] != '/')\n> +\tif (abs[baselen] != '/' && base[baselen - 1] != '/')\n>  \t\treturn abs;\n> +\twhile (abs[baselen] == '/')\n> +\t\tbaselen++;\n>  \tstrcpy(buf, abs + baselen);\n>  \treturn buf;\n>  }\n\nCurious; why does your hunk header says set_shared_perm() while this is a\npatch to make_relative_path()?  Do you run a broken git with funny\nfuncname regexp pattern?\n\nThe function takes two paths, an early part of abs is supposed to\nmatch base; otherwise abs is not a path under base and the function\nreturns the full path of abs.\n\nNow what is the goal of this patch?  To allow people to have duplicated\nslashes at random places in either abs or base, or is it only interested\nin a particular input that is malformed?  If the latter, what is the\npermitted non-canonical input?\n\nIf abs were \"/a//b/c\" and base were \"/a/b\", then the combination is\nrejected by prefixcmp() and full \"/a//b/c\" is returned.  Is it the\nintended behaviour of the patch?\n\nI would actually have expected to see something like this, but I haven't\neven compile tested it, so... \n\n path.c |   29 ++++++++++++++++++++---------\n 1 files changed, 20 insertions(+), 9 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 2ec950b..1c3570c 100644\n--- a/path.c\n+++ b/path.c\n@@ -394,17 +394,28 @@ int set_shared_perm(const char *path, int mode)\n const char *make_relative_path(const char *abs, const char *base)\n {\n \tstatic char buf[PATH_MAX + 1];\n-\tint baselen;\n+\tint i = 0, j = 0;\n+\n \tif (!base)\n \t\treturn abs;\n-\tbaselen = strlen(base);\n-\tif (prefixcmp(abs, base))\n-\t\treturn abs;\n-\tif (abs[baselen] == '/')\n-\t\tbaselen++;\n-\telse if (base[baselen - 1] != '/')\n-\t\treturn abs;\n-\tstrcpy(buf, abs + baselen);\n+\twhile (base[i]) {\n+\t\tif (base[i] == '/') {\n+\t\t\tif (abs[j] != '/')\n+\t\t\t\treturn abs;\n+\t\t\twhile (base[i] == '/')\n+\t\t\t\ti++;\n+\t\t\twhile (abs[j] == '/')\n+\t\t\t\tj++;\n+\t\t\tcontinue;\n+\t\t} else if (abs[j] != base[i]) {\n+\t\t\treturn abs;\n+\t\t}\n+\t\ti++;\n+\t\tj++;\n+\t}\n+\twhile (abs[j] == '/')\n+\t\tj++;\n+\tstrcpy(buf, abs + j);\n \treturn buf;\n }\n \n"},{"id":"132371","messageId":"7vmy06et5c.fsf@alter.siamese.dyndns.org","threadId":"22329","inReplyTo":"7vpr52gbmu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-22T03:05:19Z","receivedAt":"2010-01-22T03:05:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I would actually have expected to see something like this, but I haven't\n> even compile tested it, so... \n\nOk, here is a compile and \"make test\" tested one, together with your\naddition to the test script.\n\nI am still curious how you managed to end up with a wrong function name in\nthe context header, though.  The patch below has \"set_shared_perm\" because\nthat is the header we find before the context of the hunk, so it is sort\nof understandable; we might want to squelch the hunk header string when\nthe first context line of the hunk already matches the funcname pattern,\nthough.\n\n-- >8 --\nSubject: ignore duplicated slashes in make_relative_path()\n\nThe function takes two paths, an early part of abs is supposed to match\nbase; otherwise abs is not a path under base and the function returns the\nfull path of abs.  The caller can easily confuse the implementation by\ngiving duplicated and needless slashes in these path arguments.\n\nCredit for test script, motivation and initial patch goes to Thomas Rast,\nbut the bugs in the implementation of this patch are mine..\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n path.c              |   32 +++++++++++++++++++++++---------\n t/t1501-worktree.sh |    6 ++++++\n 2 files changed, 29 insertions(+), 9 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 2ec950b..5906fa3 100644\n--- a/path.c\n+++ b/path.c\n@@ -394,17 +394,31 @@ int set_shared_perm(const char *path, int mode)\n const char *make_relative_path(const char *abs, const char *base)\n {\n \tstatic char buf[PATH_MAX + 1];\n-\tint baselen;\n+\tint i = 0, j = 0;\n+\n \tif (!base)\n \t\treturn abs;\n-\tbaselen = strlen(base);\n-\tif (prefixcmp(abs, base))\n-\t\treturn abs;\n-\tif (abs[baselen] == '/')\n-\t\tbaselen++;\n-\telse if (base[baselen - 1] != '/')\n-\t\treturn abs;\n-\tstrcpy(buf, abs + baselen);\n+\twhile (base[i]) {\n+\t\tif (base[i] == '/') {\n+\t\t\tif (abs[j] != '/')\n+\t\t\t\treturn abs;\n+\t\t\twhile (base[i] == '/')\n+\t\t\t\ti++;\n+\t\t\twhile (abs[j] == '/')\n+\t\t\t\tj++;\n+\t\t\tcontinue;\n+\t\t} else if (abs[j] != base[i]) {\n+\t\t\treturn abs;\n+\t\t}\n+\t\ti++;\n+\t\tj++;\n+\t}\n+\twhile (abs[j] == '/')\n+\t\tj++;\n+\tif (!abs[j])\n+\t\tstrcpy(buf, \".\");\n+\telse\n+\t\tstrcpy(buf, abs + j);\n \treturn buf;\n }\n \ndiff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh\nindex 74e6443..9df3012 100755\n--- a/t/t1501-worktree.sh\n+++ b/t/t1501-worktree.sh\n@@ -189,4 +189,10 @@ test_expect_success 'absolute pathspec should fail gracefully' '\n \t)\n '\n \n+test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '\n+\t: > dummy_file\n+\techo git --git-dir=\"$(pwd)//repo.git\" --work-tree=\"$(pwd)\" add dummy_file &&\n+\tgit --git-dir=\"$(pwd)//repo.git\" --work-tree=\"$(pwd)\" add dummy_file\n+'\n+\n test_done\n"},{"id":"132387","messageId":"4B59637D.4090503@viscovery.net","threadId":"22329","inReplyTo":"7vmy06et5c.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-01-22T08:36:13Z","receivedAt":"2010-01-22T08:36:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Credit for test script, motivation and initial patch goes to Thomas Rast,\n> but the bugs in the implementation of this patch are mine..\n\nAnd with this squashed in it has fewer of them ;-) and is more portable.\nThe bug was that /foo was incorrectly stripped from /foobar.\n\n-- Hannes\n\ndiff --git a/path.c b/path.c\nindex 78ab54a..3cb19c7 100644\n--- a/path.c\n+++ b/path.c\n@@ -396,15 +396,15 @@ const char *make_relative_path(const char *abs, const char *base)\n \tstatic char buf[PATH_MAX + 1];\n \tint i = 0, j = 0;\n\n-\tif (!base)\n+\tif (!base || !base[0])\n \t\treturn abs;\n \twhile (base[i]) {\n-\t\tif (base[i] == '/') {\n-\t\t\tif (abs[j] != '/')\n+\t\tif (is_dir_sep(base[i])) {\n+\t\t\tif (!is_dir_sep(abs[j]))\n \t\t\t\treturn abs;\n-\t\t\twhile (base[i] == '/')\n+\t\t\twhile (is_dir_sep(base[i]))\n \t\t\t\ti++;\n-\t\t\twhile (abs[j] == '/')\n+\t\t\twhile (is_dir_sep(abs[j]))\n \t\t\t\tj++;\n \t\t\tcontinue;\n \t\t} else if (abs[j] != base[i]) {\n@@ -413,7 +413,14 @@ const char *make_relative_path(const char *abs, const char *base)\n \t\ti++;\n \t\tj++;\n \t}\n-\twhile (abs[j] == '/')\n+\tif (\n+\t    /* \"/foo\" is a prefix of \"/foo\" */\n+\t    abs[j] &&\n+\t    /* \"/foo\" is not a prefix of \"/foobar\" */\n+\t    !is_dir_sep(base[i-1]) && !is_dir_sep(abs[j])\n+\t   )\n+\t\treturn abs;\n+\twhile (is_dir_sep(abs[j]))\n \t\tj++;\n \tif (!abs[j])\n \t\tstrcpy(buf, \".\");\n"},{"id":"132424","messageId":"201001222211.14743.trast@student.ethz.ch","threadId":"22329","inReplyTo":"7vpr52gbmu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-01-22T21:11:14Z","receivedAt":"2010-01-22T21:11:14Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"On Friday 22 January 2010 02:40:41 Junio C Hamano wrote:\n> \n> Now what is the goal of this patch?  To allow people to have duplicated\n> slashes at random places in either abs or base, or is it only interested\n> in a particular input that is malformed?  If the latter, what is the\n> permitted non-canonical input?\n> \n> If abs were \"/a//b/c\" and base were \"/a/b\", then the combination is\n> rejected by prefixcmp() and full \"/a//b/c\" is returned.  Is it the\n> intended behaviour of the patch?\n> \n> I would actually have expected to see [a real fix that handles\n> duplicate slashes in all instances.]\n\nIt's not about *permitted* input; the problem is simply that the\ncurrent function gives back *bogus* paths, which causes git to fail.\nSo I only went for the minimal patch to fix this.\n\nNot handling the abs=\"/a//b/c\" base=\"/a/b\" case seemed ok to me since\nthat was never turned as a relative \"c\", hence there would not be any\nspeed loss (nor gain) from my patch.\n\nDoes that answer the question?\n\nAs for your patch, thanks for coming up with a real fix.  I read the\namended version, and it seems correct to me.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"132428","messageId":"7vpr51k91c.fsf@alter.siamese.dyndns.org","threadId":"22329","inReplyTo":"201001222211.14743.trast@student.ethz.ch","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-22T23:35:27Z","receivedAt":"2010-01-22T23:35:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> It's not about *permitted* input; the problem is simply that the\n> current function gives back *bogus* paths, which causes git to fail.\n> So I only went for the minimal patch to fix this.\n\nWith that logic a minimal patch would have been not to call the function\nat all, as apparently the caller seem to be able to cope with absolute\npaths returned when they could be made relative, no?\n\nIn other words, it wasn't obvious to me if the minimal patch avoided\nreturning a bogus result claiming that is a path relative to the base\ndirectory and instead returned an absolute path (which might be suboptimal\nbut way better than giving a wrong thing back) in _all_ cases, or only\njust on _some_ cases but not others, and if it was the latter, what are\nthe cases that it did better than the original.\n\n> As for your patch, thanks for coming up with a real fix.  I read the\n> amended version, and it seems correct to me.\n\nBy \"amended\", I take it to mean the fix-up by Hannes.  I'll queue one\nfor 'maint'.\n\nThanks.\n"},{"id":"132480","messageId":"201001231240.28138.robin.rosenberg@dewire.com","threadId":"22329","inReplyTo":"4B59637D.4090503@viscovery.net","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2010-01-23T11:40:27Z","receivedAt":"2010-01-23T11:40:27Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"fredagen den 22 januari 2010 09.36.13 skrev  Johannes Sixt:\n> Junio C Hamano schrieb:\n> > Credit for test script, motivation and initial patch goes to Thomas Rast,\n> > but the bugs in the implementation of this patch are mine..\n> \n> And with this squashed in it has fewer of them ;-) and is more portable.\n> The bug was that /foo was incorrectly stripped from /foobar.\n\nIt seems this function does something unhealthy when you pass a path of the \nform //server/share. On windows dropping the double // at the beginning makes\nit a different path since // is the UNC prefix.\n\nI'm not sure git on windows actually works with UNC-prefix anyway, so my point \nmay be moot, but having even more places to fix to make it work doesn't help.\n\n-- robin\n"},{"id":"132481","messageId":"201001231409.30706.j6t@kdbg.org","threadId":"22329","inReplyTo":"201001231240.28138.robin.rosenberg@dewire.com","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-23T13:09:29Z","receivedAt":"2010-01-23T13:09:29Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Samstag, 23. Januar 2010, Robin Rosenberg wrote:\n> It seems this function does something unhealthy when you pass a path of the\n> form //server/share. On windows dropping the double // at the beginning\n> makes it a different path since // is the UNC prefix.\n\nThere is no problem in practice.\n\nThe function returns either the input unmodified, or it strips also at least \none directory component, except when base is only \"/\" (or \"//\" or \"///\"...). \nI said in practice, because on Windows it does not make sense to invoke git \nwith (literally)\n\n   git --git-dir=//server/share/repo.git --work-tree=/ ...\n\ni.e., without a drive prefix before the slash of --work-tree.\n\n-- Hannes\n"},{"id":"132489","messageId":"201001231448.42721.robin.rosenberg@dewire.com","threadId":"22329","inReplyTo":"201001231409.30706.j6t@kdbg.org","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2010-01-23T13:48:42Z","receivedAt":"2010-01-23T13:48:42Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"lördagen den 23 januari 2010 14.09.29 skrev  Johannes Sixt:\n> On Samstag, 23. Januar 2010, Robin Rosenberg wrote:\n> > It seems this function does something unhealthy when you pass a path of\n> > the form //server/share. On windows dropping the double // at the\n> > beginning makes it a different path since // is the UNC prefix.\n> \n> There is no problem in practice.\n> \n> The function returns either the input unmodified, or it strips also at\n>  least one directory component, except when base is only \"/\" (or \"//\" or\n>  \"///\"...). I said in practice, because on Windows it does not make sense\n>  to invoke git with (literally)\n> \n>    git --git-dir=//server/share/repo.git --work-tree=/ ...\n> \n> i.e., without a drive prefix before the slash of --work-tree.\n\nWhy not? //foo/bar/z is just as valid and useful a path as x:/z. \n\nDefining a drive-letter with msysgit is tricky because I have to find one that \nis available and then also restart every msys bash instance to make msys\nsee it.\n\n-- robin\n"},{"id":"132492","messageId":"201001232000.28896.j6t@kdbg.org","threadId":"22329","inReplyTo":"201001231448.42721.robin.rosenberg@dewire.com","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-23T19:00:28Z","receivedAt":"2010-01-23T19:00:28Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Samstag, 23. Januar 2010, Robin Rosenberg wrote:\n> lördagen den 23 januari 2010 14.09.29 skrev  Johannes Sixt:\n> > The function returns either the input unmodified, or it strips also at\n> >  least one directory component, except when base is only \"/\" (or \"//\" or\n> >  \"///\"...). I said in practice, because on Windows it does not make sense\n> >  to invoke git with (literally)\n> >\n> >    git --git-dir=//server/share/repo.git --work-tree=/ ...\n> >\n> > i.e., without a drive prefix before the slash of --work-tree.\n>\n> Why not? //foo/bar/z is just as valid and useful a path as x:/z.\n\nFortunately, make_relative_path() does not have the slightest problem \nwith //foo/bar/z, either as value of abs (the path to make relative) or as \nbase (the path to strip from abs).\n\n-- Hannes\n"},{"id":"132498","messageId":"7vwrz8bnbj.fsf@alter.siamese.dyndns.org","threadId":"22329","inReplyTo":"201001231409.30706.j6t@kdbg.org","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-23T20:04:00Z","receivedAt":"2010-01-23T20:04:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> On Samstag, 23. Januar 2010, Robin Rosenberg wrote:\n>> It seems this function does something unhealthy when you pass a path of the\n>> form //server/share. On windows dropping the double // at the beginning\n>> makes it a different path since // is the UNC prefix.\n>\n> There is no problem in practice.\n>\n> The function returns either the input unmodified, or it strips also at least \n> one directory component, except when base is only \"/\" (or \"//\" or \"///\"...). \n> I said in practice, because on Windows it does not make sense to invoke git \n> with (literally)\n>\n>    git --git-dir=//server/share/repo.git --work-tree=/ ...\n>\n> i.e., without a drive prefix before the slash of --work-tree.\n\nIf you did this:\n\n    git --git-dir=//reposerver/repo.git --work-tree=//buildserver/workarea\n\nthen we would say \"one is not a prefix of the other\", so it would be\nfine.  At least I don't think the \"recover from unintentionally doubled\nslashes in user supplied path\" fix is introducing any new problem in that\ncase.\n\nIf on the other hand, if you did this:\n\n    git --git-dir=//server/repo.git --work-tree=//server/workarea\n\nthat also would be Ok.\n\nI think one issue is what happens when you did this:\n\n    cd //server\n    git --git-dir=//server/repo/repo.git --work-tree=repo\n\nDoes msysgit implementation figures out that the work tree is located at\n\"//server/repo\" when get_git_work_tree() is asked to produce an absolute\npath so that it can be compared with //server/repo/repo.git with the code?\nIf it does (with the leading double slash), then \"doubled slahses fix\" is\na regression we should do something about it.  If it doesn't, then it\nprobably doesn't matter.\n"},{"id":"132502","messageId":"7viqasbmtc.fsf@alter.siamese.dyndns.org","threadId":"22329","inReplyTo":"7vwrz8bnbj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-23T20:14:55Z","receivedAt":"2010-01-23T20:14:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I think one issue is what happens when you did this:\n>\n>     cd //server\n>     git --git-dir=//server/repo/repo.git --work-tree=repo\n>\n> Does msysgit implementation figures out that the work tree is located at\n> \"//server/repo\" when get_git_work_tree() is asked to produce an absolute\n> path so that it can be compared with //server/repo/repo.git with the code?\n> If it does (with the leading double slash), then \"doubled slahses fix\" is\n> a regression we should do something about it.  If it doesn't, then it\n> probably doesn't matter.\n\nNah, I wasn't thinking straight.  What happens if you did this?\n\n\tgit --git-dir=//git/repo/repo.git --work-tree=/git/repo\n\nwhere \"//git/repo\" is on the \"git server\" and you are working in local\nhierarchy \"/git/repo\"?\n"},{"id":"132503","messageId":"201001232141.49556.j6t@kdbg.org","threadId":"22329","inReplyTo":"7viqasbmtc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-23T20:41:49Z","receivedAt":"2010-01-23T20:41:49Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Samstag, 23. Januar 2010, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> > I think one issue is what happens when you did this:\n> >\n> >     cd //server\n> >     git --git-dir=//server/repo/repo.git --work-tree=repo\n> >\n> > Does msysgit implementation figures out that the work tree is located at\n> > \"//server/repo\" when get_git_work_tree() is asked to produce an absolute\n> > path so that it can be compared with //server/repo/repo.git with the\n> > code? If it does (with the leading double slash), then \"doubled slahses\n> > fix\" is a regression we should do something about it.  If it doesn't,\n> > then it probably doesn't matter.\n>\n> Nah, I wasn't thinking straight.  What happens if you did this?\n>\n> \tgit --git-dir=//git/repo/repo.git --work-tree=/git/repo\n>\n> where \"//git/repo\" is on the \"git server\" and you are working in local\n> hierarchy \"/git/repo\"?\n\nAh, right, this would not do the right thing. (But I can't verify this claim \nright now.)\n\nThe problem is that /git/repo without a drive prefix is a valid path and it \nmeans the path that begins at the same drive that CWD currently is. I would \nnot dismiss this form of paths as too exotic, so we should care about them. \nOTOH, it can be worked around easily by the user (just insert the drive \nprefix). Dunno...\n\n-- Hannes\n"},{"id":"132504","messageId":"fabb9a1e1001231301o149bb13es236a7150f57ce161@mail.gmail.com","threadId":"22329","inReplyTo":"201001232141.49556.j6t@kdbg.org","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-01-23T21:01:38Z","receivedAt":"2010-01-23T21:01:38Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sat, Jan 23, 2010 at 21:41, Johannes Sixt <j6t@kdbg.org> wrote:\n\n> OTOH, it can be worked around easily by the user (just insert the drive\n> prefix). Dunno...\n\nI think it's preferable to keep the old behavior where we fail if the\nuser gives us an invalid argument, rather than fix a user error and\nbreak on a a valid argument instead. I think we should be correct\nfirst, and try and fix incorrect user behavior after.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"132538","messageId":"201001241457.43297.trast@student.ethz.ch","threadId":"22329","inReplyTo":"fabb9a1e1001231301o149bb13es236a7150f57ce161@mail.gmail.com","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-01-24T13:57:42Z","receivedAt":"2010-01-24T13:57:42Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"On Saturday 23 January 2010 22:01:38 Sverre Rabbelier wrote:\n> Heya,\n> \n> On Sat, Jan 23, 2010 at 21:41, Johannes Sixt <j6t@kdbg.org> wrote:\n> \n> > OTOH, it can be worked around easily by the user (just insert the drive\n> > prefix). Dunno...\n> \n> I think it's preferable to keep the old behavior where we fail if the\n> user gives us an invalid argument, rather than fix a user error and\n> break on a a valid argument instead. I think we should be correct\n> first, and try and fix incorrect user behavior after.\n\nI can't really comment on the Windows side of things, but I tried to\ncome up with some more data points.\n\nPOSIX specifies that multiple slashes must be treated as if they were\na single slash (except as in the next bullet point).  Leading _double_\nslashes may be treated implementation-dependently. [1]  Non-leading\ndouble slashes do not seem to be specified.\n\nThere's a manpage path_resolution(7) on my system, which can also be\nfound on the web quite easily, e.g. [2].  It doesn't say anything\nabout multiple slashes, but experimentally my Linux resolves them as\nif they were single slashes (even a leading double slash).\n\nJunio's patch is already in maint, so I suppose we're in the somewhat\nunfortunate situation where the old version didn't work in all cases\non Linux, but the current one breaks on Windows in some cases.  Then\nagain, shouldn't windows get special support to figure out that /c/foo\n[3] is a prefix of /foo and vice versa, assuming you're currently in\nC:?\n\n\n[1] http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html#tag_04_11\nActually the server doesn't work for me, but google has a cached copy:\nhttp://209.85.129.132/search?q=cache:QUuajH7Dp5gJ:www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html+http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html#tag_04_11&cd=1&hl=en&ct=clnk&gl=uk\n\n[2] http://www.kernel.org/doc/man-pages/online/pages/man7/path_resolution.7.html\n\n[3] Don't blame me if I didn't get that syntax right, I'm actively\ntrying to forget I ever used Windows and it shows.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"132544","messageId":"201001241744.57139.j6t@kdbg.org","threadId":"22329","inReplyTo":"201001232141.49556.j6t@kdbg.org","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-24T16:44:56Z","receivedAt":"2010-01-24T16:44:56Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Samstag, 23. Januar 2010, Johannes Sixt wrote:\n> On Samstag, 23. Januar 2010, Junio C Hamano wrote:\n> > What happens if you did this?\n> >\n> > \tgit --git-dir=//git/repo/repo.git --work-tree=/git/repo\n> >\n> > where \"//git/repo\" is on the \"git server\" and you are working in local\n> > hierarchy \"/git/repo\"?\n>\n> Ah, right, this would not do the right thing. (But I can't verify this\n> claim right now.)\n\nI tested it, and it does the right thing. The reason is that before \nsetup_work_tree() calls make_relative_path(), the --work-tree argument has \nbeen processed by make_absolute_path(), which adds the drive prefix.\n\nAs long as setup_work_tree() remains the only caller of make_relative_path(), \nwe are safe.\n\n-- Hannes\n"},{"id":"132548","messageId":"7v8wbn8ie2.fsf@alter.siamese.dyndns.org","threadId":"22329","inReplyTo":"201001241744.57139.j6t@kdbg.org","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-24T18:31:01Z","receivedAt":"2010-01-24T18:31:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> On Samstag, 23. Januar 2010, Johannes Sixt wrote:\n>> On Samstag, 23. Januar 2010, Junio C Hamano wrote:\n>> > What happens if you did this?\n>> >\n>> > \tgit --git-dir=//git/repo/repo.git --work-tree=/git/repo\n>> >\n>> > where \"//git/repo\" is on the \"git server\" and you are working in local\n>> > hierarchy \"/git/repo\"?\n>>\n>> Ah, right, this would not do the right thing. (But I can't verify this\n>> claim right now.)\n>\n> I tested it, and it does the right thing. The reason is that before \n> setup_work_tree() calls make_relative_path(), the --work-tree argument has \n> been processed by make_absolute_path(), which adds the drive prefix.\n>\n> As long as setup_work_tree() remains the only caller of make_relative_path(), \n> we are safe.\n\nThanks; I think a more correct description of your findings is:\n\n - msysgit's make_absolute_path() does the right thing (i.e. adds \"drive\n   prefix\" to \"git/repo\" given to --work-tree); and\n\n - as long as the callers feed what the platform considers absolute paths\n   in abs and base, make_relative_path() does the right thing.\n\nSo I think we are Ok.  We _might_ want to add Windows-only test at the\nbeginning of make_relative_path() to make sure that the both inputs have\ndouble-slashes at the beginning to catch future broken callers, but I\nthink that is a separate topic, and we don't have to do that as long as\nsetup_work_tree(0 remains the only caller, as you said.\n"},{"id":"132557","messageId":"20100124190424.GA30585@pcpool00.mathematik.uni-freiburg.de","threadId":"22329","inReplyTo":"201001241457.43297.trast@student.ethz.ch","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Bernhard R. Link","fromEmail":"brlink@debian.org","sentAt":"2010-01-24T19:04:24Z","receivedAt":"2010-01-24T19:04:24Z","isPatch":true,"sender":{"key":"brlink@debian.org","avatar":null},"body":"* Thomas Rast <trast@student.ethz.ch> [100124 14:58]:\n> POSIX specifies that multiple slashes must be treated as if they were\n> a single slash (except as in the next bullet point).  Leading _double_\n> slashes may be treated implementation-dependently. [1]  Non-leading\n> double slashes do not seem to be specified.\n\nhttp://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266\n\n| 3.266 Pathname\n|\n| A character string that is used to identify a file. In the context of\n| IEEE Std 1003.1-2001, a pathname consists of, at most, {PATH_MAX} bytes,\n| including the terminating null byte. It has an optional beginning\n| slash, followed by zero or more filenames separated by slashes. A\n| pathname may optionally contain one or more trailing slashes.\n| Multiple successive slashes are considered to be the same as one\n| slash.\n"},{"id":"132558","messageId":"7vr5pf5kw4.fsf@alter.siamese.dyndns.org","threadId":"22329","inReplyTo":"20100124190424.GA30585@pcpool00.mathematik.uni-freiburg.de","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-24T20:05:15Z","receivedAt":"2010-01-24T20:05:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Bernhard R. Link\" <brlink@debian.org> writes:\n\n> http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266\n>\n> | 3.266 Pathname\n> |\n> | A character string that is used to identify a file. In the context of\n> | IEEE Std 1003.1-2001, a pathname consists of, at most, {PATH_MAX} bytes,\n> | including the terminating null byte. It has an optional beginning\n> | slash, followed by zero or more filenames separated by slashes. A\n> | pathname may optionally contain one or more trailing slashes.\n> | Multiple successive slashes are considered to be the same as one\n> | slash.\n\nWhich is a bit stale, and there is an update that is crucial when\ndiscussing the issue that is the topic of this thread.\n\nTry this one instead, especially the last \", except for\" which is an\nimportant clarification.\n\nhttp://www.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html\n\n| 3.266 Pathname\n| \n| A character string that is used to identify a file. In the context of\n| POSIX.1-2008, a pathname may be limited to {PATH_MAX} bytes, including the\n| terminating null byte. It has an optional beginning <slash>, followed by\n| zero or more filenames separated by <slash> characters. A pathname may\n| optionally contain one or more trailing <slash> characters. Multiple\n| successive <slash> characters are considered to be the same as one\n| <slash>, except for the case of exactly two leading <slash> characters.\n\nAnd 4.12 in the same issue of POSIX says pathnames with exactly two\nleading slashes may be handled in an implemenation defined manner\n(i.e. three or more means the same thing as a single slash).  Confusing\n;-)\n"},{"id":"132572","messageId":"201001250206.48138.robin.rosenberg@dewire.com","threadId":"22329","inReplyTo":"7v8wbn8ie2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Handle double slashes in make_relative_path()","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2010-01-25T01:06:48Z","receivedAt":"2010-01-25T01:06:48Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"söndagen den 24 januari 2010 19.31.01 skrev  Junio C Hamano:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> > On Samstag, 23. Januar 2010, Johannes Sixt wrote:\n> >> On Samstag, 23. Januar 2010, Junio C Hamano wrote:\n> >> > What happens if you did this?\n> >> >\n> >> > \tgit --git-dir=//git/repo/repo.git --work-tree=/git/repo\n> >> >\n> >> > where \"//git/repo\" is on the \"git server\" and you are working in local\n> >> > hierarchy \"/git/repo\"?\n> >>\n> >> Ah, right, this would not do the right thing. (But I can't verify this\n> >> claim right now.)\n> >\n> > I tested it, and it does the right thing. The reason is that before\n> > setup_work_tree() calls make_relative_path(), the --work-tree argument\n> > has been processed by make_absolute_path(), which adds the drive prefix.\n> >\n> > As long as setup_work_tree() remains the only caller of\n> > make_relative_path(), we are safe.\n> \n> Thanks; I think a more correct description of your findings is:\n> \n>  - msysgit's make_absolute_path() does the right thing (i.e. adds \"drive\n>    prefix\" to \"git/repo\" given to --work-tree); and\n> \n>  - as long as the callers feed what the platform considers absolute paths\n>    in abs and base, make_relative_path() does the right thing.\n> \n> So I think we are Ok.  We _might_ want to add Windows-only test at the\n> beginning of make_relative_path() to make sure that the both inputs have\n> double-slashes at the beginning to catch future broken callers, but I\n> think that is a separate topic, and we don't have to do that as long as\n> setup_work_tree(0 remains the only caller, as you said.\n\nSeparate patch posted since this problem occurs in more than one place. \n\n-- robin\n"}]}