# [PATCH] Handle double slashes in make_relative_path() 20 messages from 2010-01-22 to 2010-01-25. Participants: Thomas Rast, Junio C Hamano, Johannes Sixt, Robin Rosenberg, Sverre Rabbelier, Bernhard R. Link. Thread: https://gitlist.dev/t/22329 ## Thomas Rast, 2010-01-22 00:07 Subject: [PATCH] Handle double slashes in make_relative_path() Message-ID: <379d55c6a4110736aadb8ace3b050de879a9deab.1264118830.git.trast@student.ethz.ch> URL: https://gitlist.dev/e/379d55c6a4110736aadb8ace3b050de879a9deab.1264118830.git.trast%40student.ethz.ch ``` If you say git --git-dir=/some//path --work-tree=/some/ add then setup_work_tree() will call into make_relative_path() with abs="/some//path" and base="/some". (Note how the latter has already lost its trailing slash. One unfortunate user managed to trigger this because his $HOME ended in a slash.) This means that when checking whether 'abs' is a path under 'base', we need to skip *two* slashes where the previous code only accounted for one. Fix it to handle an arbitrary number of slashes at that position. Noticed-by: eldenz on freenode Signed-off-by: Thomas Rast --- path.c | 6 +++--- t/t1501-worktree.sh | 6 ++++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/path.c b/path.c index 2ec950b..a195bab 100644 --- a/path.c +++ b/path.c @@ -400,10 +400,10 @@ int set_shared_perm(const char *path, int mode) baselen = strlen(base); if (prefixcmp(abs, base)) return abs; - if (abs[baselen] == '/') - baselen++; - else if (base[baselen - 1] != '/') + if (abs[baselen] != '/' && base[baselen - 1] != '/') return abs; + while (abs[baselen] == '/') + baselen++; strcpy(buf, abs + baselen); return buf; } diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh index 74e6443..9df3012 100755 --- a/t/t1501-worktree.sh +++ b/t/t1501-worktree.sh @@ -189,4 +189,10 @@ test_expect_success 'absolute pathspec should fail gracefully' ' ) ' +test_expect_success 'make_relative_path handles double slashes in GIT_DIR' ' + : > dummy_file + echo git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file && + git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file +' + test_done -- 1.6.6.1.532.g594fe ``` ## Junio C Hamano, 2010-01-22 01:40 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <7vpr52gbmu.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vpr52gbmu.fsf%40alter.siamese.dyndns.org In-Reply-To: <379d55c6a4110736aadb8ace3b050de879a9deab.1264118830.git.trast@student.ethz.ch> ``` Thomas Rast writes: > diff --git a/path.c b/path.c > index 2ec950b..a195bab 100644 > --- a/path.c > +++ b/path.c > @@ -400,10 +400,10 @@ int set_shared_perm(const char *path, int mode) > baselen = strlen(base); > if (prefixcmp(abs, base)) > return abs; > - if (abs[baselen] == '/') > - baselen++; > - else if (base[baselen - 1] != '/') > + if (abs[baselen] != '/' && base[baselen - 1] != '/') > return abs; > + while (abs[baselen] == '/') > + baselen++; > strcpy(buf, abs + baselen); > return buf; > } Curious; why does your hunk header says set_shared_perm() while this is a patch to make_relative_path()? Do you run a broken git with funny funcname regexp pattern? The function takes two paths, an early part of abs is supposed to match base; otherwise abs is not a path under base and the function returns the full path of abs. Now what is the goal of this patch? To allow people to have duplicated slashes at random places in either abs or base, or is it only interested in a particular input that is malformed? If the latter, what is the permitted non-canonical input? If abs were "/a//b/c" and base were "/a/b", then the combination is rejected by prefixcmp() and full "/a//b/c" is returned. Is it the intended behaviour of the patch? I would actually have expected to see something like this, but I haven't even compile tested it, so... path.c | 29 ++++++++++++++++++++--------- 1 files changed, 20 insertions(+), 9 deletions(-) diff --git a/path.c b/path.c index 2ec950b..1c3570c 100644 --- a/path.c +++ b/path.c @@ -394,17 +394,28 @@ int set_shared_perm(const char *path, int mode) const char *make_relative_path(const char *abs, const char *base) { static char buf[PATH_MAX + 1]; - int baselen; + int i = 0, j = 0; + if (!base) return abs; - baselen = strlen(base); - if (prefixcmp(abs, base)) - return abs; - if (abs[baselen] == '/') - baselen++; - else if (base[baselen - 1] != '/') - return abs; - strcpy(buf, abs + baselen); + while (base[i]) { + if (base[i] == '/') { + if (abs[j] != '/') + return abs; + while (base[i] == '/') + i++; + while (abs[j] == '/') + j++; + continue; + } else if (abs[j] != base[i]) { + return abs; + } + i++; + j++; + } + while (abs[j] == '/') + j++; + strcpy(buf, abs + j); return buf; } ``` ## Junio C Hamano, 2010-01-22 03:05 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <7vmy06et5c.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmy06et5c.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vpr52gbmu.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > I would actually have expected to see something like this, but I haven't > even compile tested it, so... Ok, here is a compile and "make test" tested one, together with your addition to the test script. I am still curious how you managed to end up with a wrong function name in the context header, though. The patch below has "set_shared_perm" because that is the header we find before the context of the hunk, so it is sort of understandable; we might want to squelch the hunk header string when the first context line of the hunk already matches the funcname pattern, though. -- >8 -- Subject: ignore duplicated slashes in make_relative_path() The function takes two paths, an early part of abs is supposed to match base; otherwise abs is not a path under base and the function returns the full path of abs. The caller can easily confuse the implementation by giving duplicated and needless slashes in these path arguments. Credit for test script, motivation and initial patch goes to Thomas Rast, but the bugs in the implementation of this patch are mine.. Signed-off-by: Junio C Hamano --- path.c | 32 +++++++++++++++++++++++--------- t/t1501-worktree.sh | 6 ++++++ 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/path.c b/path.c index 2ec950b..5906fa3 100644 --- a/path.c +++ b/path.c @@ -394,17 +394,31 @@ int set_shared_perm(const char *path, int mode) const char *make_relative_path(const char *abs, const char *base) { static char buf[PATH_MAX + 1]; - int baselen; + int i = 0, j = 0; + if (!base) return abs; - baselen = strlen(base); - if (prefixcmp(abs, base)) - return abs; - if (abs[baselen] == '/') - baselen++; - else if (base[baselen - 1] != '/') - return abs; - strcpy(buf, abs + baselen); + while (base[i]) { + if (base[i] == '/') { + if (abs[j] != '/') + return abs; + while (base[i] == '/') + i++; + while (abs[j] == '/') + j++; + continue; + } else if (abs[j] != base[i]) { + return abs; + } + i++; + j++; + } + while (abs[j] == '/') + j++; + if (!abs[j]) + strcpy(buf, "."); + else + strcpy(buf, abs + j); return buf; } diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh index 74e6443..9df3012 100755 --- a/t/t1501-worktree.sh +++ b/t/t1501-worktree.sh @@ -189,4 +189,10 @@ test_expect_success 'absolute pathspec should fail gracefully' ' ) ' +test_expect_success 'make_relative_path handles double slashes in GIT_DIR' ' + : > dummy_file + echo git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file && + git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file +' + test_done ``` ## Johannes Sixt, 2010-01-22 08:36 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <4B59637D.4090503@viscovery.net> URL: https://gitlist.dev/e/4B59637D.4090503%40viscovery.net In-Reply-To: <7vmy06et5c.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano schrieb: > Credit for test script, motivation and initial patch goes to Thomas Rast, > but the bugs in the implementation of this patch are mine.. And with this squashed in it has fewer of them ;-) and is more portable. The bug was that /foo was incorrectly stripped from /foobar. -- Hannes diff --git a/path.c b/path.c index 78ab54a..3cb19c7 100644 --- a/path.c +++ b/path.c @@ -396,15 +396,15 @@ const char *make_relative_path(const char *abs, const char *base) static char buf[PATH_MAX + 1]; int i = 0, j = 0; - if (!base) + if (!base || !base[0]) return abs; while (base[i]) { - if (base[i] == '/') { - if (abs[j] != '/') + if (is_dir_sep(base[i])) { + if (!is_dir_sep(abs[j])) return abs; - while (base[i] == '/') + while (is_dir_sep(base[i])) i++; - while (abs[j] == '/') + while (is_dir_sep(abs[j])) j++; continue; } else if (abs[j] != base[i]) { @@ -413,7 +413,14 @@ const char *make_relative_path(const char *abs, const char *base) i++; j++; } - while (abs[j] == '/') + if ( + /* "/foo" is a prefix of "/foo" */ + abs[j] && + /* "/foo" is not a prefix of "/foobar" */ + !is_dir_sep(base[i-1]) && !is_dir_sep(abs[j]) + ) + return abs; + while (is_dir_sep(abs[j])) j++; if (!abs[j]) strcpy(buf, "."); ``` ## Thomas Rast, 2010-01-22 21:11 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <201001222211.14743.trast@student.ethz.ch> URL: https://gitlist.dev/e/201001222211.14743.trast%40student.ethz.ch In-Reply-To: <7vpr52gbmu.fsf@alter.siamese.dyndns.org> ``` On Friday 22 January 2010 02:40:41 Junio C Hamano wrote: > > Now what is the goal of this patch? To allow people to have duplicated > slashes at random places in either abs or base, or is it only interested > in a particular input that is malformed? If the latter, what is the > permitted non-canonical input? > > If abs were "/a//b/c" and base were "/a/b", then the combination is > rejected by prefixcmp() and full "/a//b/c" is returned. Is it the > intended behaviour of the patch? > > I would actually have expected to see [a real fix that handles > duplicate slashes in all instances.] It's not about *permitted* input; the problem is simply that the current function gives back *bogus* paths, which causes git to fail. So I only went for the minimal patch to fix this. Not handling the abs="/a//b/c" base="/a/b" case seemed ok to me since that was never turned as a relative "c", hence there would not be any speed loss (nor gain) from my patch. Does that answer the question? As for your patch, thanks for coming up with a real fix. I read the amended version, and it seems correct to me. -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Junio C Hamano, 2010-01-22 23:35 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <7vpr51k91c.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vpr51k91c.fsf%40alter.siamese.dyndns.org In-Reply-To: <201001222211.14743.trast@student.ethz.ch> ``` Thomas Rast writes: > It's not about *permitted* input; the problem is simply that the > current function gives back *bogus* paths, which causes git to fail. > So I only went for the minimal patch to fix this. With that logic a minimal patch would have been not to call the function at all, as apparently the caller seem to be able to cope with absolute paths returned when they could be made relative, no? In other words, it wasn't obvious to me if the minimal patch avoided returning a bogus result claiming that is a path relative to the base directory and instead returned an absolute path (which might be suboptimal but way better than giving a wrong thing back) in _all_ cases, or only just on _some_ cases but not others, and if it was the latter, what are the cases that it did better than the original. > As for your patch, thanks for coming up with a real fix. I read the > amended version, and it seems correct to me. By "amended", I take it to mean the fix-up by Hannes. I'll queue one for 'maint'. Thanks. ``` ## Robin Rosenberg, 2010-01-23 11:40 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <201001231240.28138.robin.rosenberg@dewire.com> URL: https://gitlist.dev/e/201001231240.28138.robin.rosenberg%40dewire.com In-Reply-To: <4B59637D.4090503@viscovery.net> ``` fredagen den 22 januari 2010 09.36.13 skrev Johannes Sixt: > Junio C Hamano schrieb: > > Credit for test script, motivation and initial patch goes to Thomas Rast, > > but the bugs in the implementation of this patch are mine.. > > And with this squashed in it has fewer of them ;-) and is more portable. > The bug was that /foo was incorrectly stripped from /foobar. It seems this function does something unhealthy when you pass a path of the form //server/share. On windows dropping the double // at the beginning makes it a different path since // is the UNC prefix. I'm not sure git on windows actually works with UNC-prefix anyway, so my point may be moot, but having even more places to fix to make it work doesn't help. -- robin ``` ## Johannes Sixt, 2010-01-23 13:09 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <201001231409.30706.j6t@kdbg.org> URL: https://gitlist.dev/e/201001231409.30706.j6t%40kdbg.org In-Reply-To: <201001231240.28138.robin.rosenberg@dewire.com> ``` On Samstag, 23. Januar 2010, Robin Rosenberg wrote: > It seems this function does something unhealthy when you pass a path of the > form //server/share. On windows dropping the double // at the beginning > makes it a different path since // is the UNC prefix. There is no problem in practice. The function returns either the input unmodified, or it strips also at least one directory component, except when base is only "/" (or "//" or "///"...). I said in practice, because on Windows it does not make sense to invoke git with (literally) git --git-dir=//server/share/repo.git --work-tree=/ ... i.e., without a drive prefix before the slash of --work-tree. -- Hannes ``` ## Robin Rosenberg, 2010-01-23 13:48 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <201001231448.42721.robin.rosenberg@dewire.com> URL: https://gitlist.dev/e/201001231448.42721.robin.rosenberg%40dewire.com In-Reply-To: <201001231409.30706.j6t@kdbg.org> ``` lördagen den 23 januari 2010 14.09.29 skrev Johannes Sixt: > On Samstag, 23. Januar 2010, Robin Rosenberg wrote: > > It seems this function does something unhealthy when you pass a path of > > the form //server/share. On windows dropping the double // at the > > beginning makes it a different path since // is the UNC prefix. > > There is no problem in practice. > > The function returns either the input unmodified, or it strips also at > least one directory component, except when base is only "/" (or "//" or > "///"...). I said in practice, because on Windows it does not make sense > to invoke git with (literally) > > git --git-dir=//server/share/repo.git --work-tree=/ ... > > i.e., without a drive prefix before the slash of --work-tree. Why not? //foo/bar/z is just as valid and useful a path as x:/z. Defining a drive-letter with msysgit is tricky because I have to find one that is available and then also restart every msys bash instance to make msys see it. -- robin ``` ## Johannes Sixt, 2010-01-23 19:00 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <201001232000.28896.j6t@kdbg.org> URL: https://gitlist.dev/e/201001232000.28896.j6t%40kdbg.org In-Reply-To: <201001231448.42721.robin.rosenberg@dewire.com> ``` On Samstag, 23. Januar 2010, Robin Rosenberg wrote: > lördagen den 23 januari 2010 14.09.29 skrev Johannes Sixt: > > The function returns either the input unmodified, or it strips also at > > least one directory component, except when base is only "/" (or "//" or > > "///"...). I said in practice, because on Windows it does not make sense > > to invoke git with (literally) > > > > git --git-dir=//server/share/repo.git --work-tree=/ ... > > > > i.e., without a drive prefix before the slash of --work-tree. > > Why not? //foo/bar/z is just as valid and useful a path as x:/z. Fortunately, make_relative_path() does not have the slightest problem with //foo/bar/z, either as value of abs (the path to make relative) or as base (the path to strip from abs). -- Hannes ``` ## Junio C Hamano, 2010-01-23 20:04 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <7vwrz8bnbj.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwrz8bnbj.fsf%40alter.siamese.dyndns.org In-Reply-To: <201001231409.30706.j6t@kdbg.org> ``` Johannes Sixt writes: > On Samstag, 23. Januar 2010, Robin Rosenberg wrote: >> It seems this function does something unhealthy when you pass a path of the >> form //server/share. On windows dropping the double // at the beginning >> makes it a different path since // is the UNC prefix. > > There is no problem in practice. > > The function returns either the input unmodified, or it strips also at least > one directory component, except when base is only "/" (or "//" or "///"...). > I said in practice, because on Windows it does not make sense to invoke git > with (literally) > > git --git-dir=//server/share/repo.git --work-tree=/ ... > > i.e., without a drive prefix before the slash of --work-tree. If you did this: git --git-dir=//reposerver/repo.git --work-tree=//buildserver/workarea then we would say "one is not a prefix of the other", so it would be fine. At least I don't think the "recover from unintentionally doubled slashes in user supplied path" fix is introducing any new problem in that case. If on the other hand, if you did this: git --git-dir=//server/repo.git --work-tree=//server/workarea that also would be Ok. I think one issue is what happens when you did this: cd //server git --git-dir=//server/repo/repo.git --work-tree=repo Does msysgit implementation figures out that the work tree is located at "//server/repo" when get_git_work_tree() is asked to produce an absolute path so that it can be compared with //server/repo/repo.git with the code? If it does (with the leading double slash), then "doubled slahses fix" is a regression we should do something about it. If it doesn't, then it probably doesn't matter. ``` ## Junio C Hamano, 2010-01-23 20:14 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <7viqasbmtc.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7viqasbmtc.fsf%40alter.siamese.dyndns.org In-Reply-To: <7vwrz8bnbj.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > I think one issue is what happens when you did this: > > cd //server > git --git-dir=//server/repo/repo.git --work-tree=repo > > Does msysgit implementation figures out that the work tree is located at > "//server/repo" when get_git_work_tree() is asked to produce an absolute > path so that it can be compared with //server/repo/repo.git with the code? > If it does (with the leading double slash), then "doubled slahses fix" is > a regression we should do something about it. If it doesn't, then it > probably doesn't matter. Nah, I wasn't thinking straight. What happens if you did this? git --git-dir=//git/repo/repo.git --work-tree=/git/repo where "//git/repo" is on the "git server" and you are working in local hierarchy "/git/repo"? ``` ## Johannes Sixt, 2010-01-23 20:41 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <201001232141.49556.j6t@kdbg.org> URL: https://gitlist.dev/e/201001232141.49556.j6t%40kdbg.org In-Reply-To: <7viqasbmtc.fsf@alter.siamese.dyndns.org> ``` On Samstag, 23. Januar 2010, Junio C Hamano wrote: > Junio C Hamano writes: > > I think one issue is what happens when you did this: > > > > cd //server > > git --git-dir=//server/repo/repo.git --work-tree=repo > > > > Does msysgit implementation figures out that the work tree is located at > > "//server/repo" when get_git_work_tree() is asked to produce an absolute > > path so that it can be compared with //server/repo/repo.git with the > > code? If it does (with the leading double slash), then "doubled slahses > > fix" is a regression we should do something about it. If it doesn't, > > then it probably doesn't matter. > > Nah, I wasn't thinking straight. What happens if you did this? > > git --git-dir=//git/repo/repo.git --work-tree=/git/repo > > where "//git/repo" is on the "git server" and you are working in local > hierarchy "/git/repo"? Ah, right, this would not do the right thing. (But I can't verify this claim right now.) The problem is that /git/repo without a drive prefix is a valid path and it means the path that begins at the same drive that CWD currently is. I would not dismiss this form of paths as too exotic, so we should care about them. OTOH, it can be worked around easily by the user (just insert the drive prefix). Dunno... -- Hannes ``` ## Sverre Rabbelier, 2010-01-23 21:01 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: URL: https://gitlist.dev/e/fabb9a1e1001231301o149bb13es236a7150f57ce161%40mail.gmail.com In-Reply-To: <201001232141.49556.j6t@kdbg.org> ``` Heya, On Sat, Jan 23, 2010 at 21:41, Johannes Sixt wrote: > OTOH, it can be worked around easily by the user (just insert the drive > prefix). Dunno... I think it's preferable to keep the old behavior where we fail if the user gives us an invalid argument, rather than fix a user error and break on a a valid argument instead. I think we should be correct first, and try and fix incorrect user behavior after. -- Cheers, Sverre Rabbelier ``` ## Thomas Rast, 2010-01-24 13:57 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <201001241457.43297.trast@student.ethz.ch> URL: https://gitlist.dev/e/201001241457.43297.trast%40student.ethz.ch In-Reply-To: ``` On Saturday 23 January 2010 22:01:38 Sverre Rabbelier wrote: > Heya, > > On Sat, Jan 23, 2010 at 21:41, Johannes Sixt wrote: > > > OTOH, it can be worked around easily by the user (just insert the drive > > prefix). Dunno... > > I think it's preferable to keep the old behavior where we fail if the > user gives us an invalid argument, rather than fix a user error and > break on a a valid argument instead. I think we should be correct > first, and try and fix incorrect user behavior after. I can't really comment on the Windows side of things, but I tried to come up with some more data points. POSIX specifies that multiple slashes must be treated as if they were a single slash (except as in the next bullet point). Leading _double_ slashes may be treated implementation-dependently. [1] Non-leading double slashes do not seem to be specified. There's a manpage path_resolution(7) on my system, which can also be found on the web quite easily, e.g. [2]. It doesn't say anything about multiple slashes, but experimentally my Linux resolves them as if they were single slashes (even a leading double slash). Junio's patch is already in maint, so I suppose we're in the somewhat unfortunate situation where the old version didn't work in all cases on Linux, but the current one breaks on Windows in some cases. Then again, shouldn't windows get special support to figure out that /c/foo [3] is a prefix of /foo and vice versa, assuming you're currently in C:? [1] http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html#tag_04_11 Actually the server doesn't work for me, but google has a cached copy: http://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 [2] http://www.kernel.org/doc/man-pages/online/pages/man7/path_resolution.7.html [3] Don't blame me if I didn't get that syntax right, I'm actively trying to forget I ever used Windows and it shows. -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Johannes Sixt, 2010-01-24 16:44 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <201001241744.57139.j6t@kdbg.org> URL: https://gitlist.dev/e/201001241744.57139.j6t%40kdbg.org In-Reply-To: <201001232141.49556.j6t@kdbg.org> ``` On Samstag, 23. Januar 2010, Johannes Sixt wrote: > On Samstag, 23. Januar 2010, Junio C Hamano wrote: > > What happens if you did this? > > > > git --git-dir=//git/repo/repo.git --work-tree=/git/repo > > > > where "//git/repo" is on the "git server" and you are working in local > > hierarchy "/git/repo"? > > Ah, right, this would not do the right thing. (But I can't verify this > claim right now.) I tested it, and it does the right thing. The reason is that before setup_work_tree() calls make_relative_path(), the --work-tree argument has been processed by make_absolute_path(), which adds the drive prefix. As long as setup_work_tree() remains the only caller of make_relative_path(), we are safe. -- Hannes ``` ## Junio C Hamano, 2010-01-24 18:31 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <7v8wbn8ie2.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8wbn8ie2.fsf%40alter.siamese.dyndns.org In-Reply-To: <201001241744.57139.j6t@kdbg.org> ``` Johannes Sixt writes: > On Samstag, 23. Januar 2010, Johannes Sixt wrote: >> On Samstag, 23. Januar 2010, Junio C Hamano wrote: >> > What happens if you did this? >> > >> > git --git-dir=//git/repo/repo.git --work-tree=/git/repo >> > >> > where "//git/repo" is on the "git server" and you are working in local >> > hierarchy "/git/repo"? >> >> Ah, right, this would not do the right thing. (But I can't verify this >> claim right now.) > > I tested it, and it does the right thing. The reason is that before > setup_work_tree() calls make_relative_path(), the --work-tree argument has > been processed by make_absolute_path(), which adds the drive prefix. > > As long as setup_work_tree() remains the only caller of make_relative_path(), > we are safe. Thanks; I think a more correct description of your findings is: - msysgit's make_absolute_path() does the right thing (i.e. adds "drive prefix" to "git/repo" given to --work-tree); and - as long as the callers feed what the platform considers absolute paths in abs and base, make_relative_path() does the right thing. So I think we are Ok. We _might_ want to add Windows-only test at the beginning of make_relative_path() to make sure that the both inputs have double-slashes at the beginning to catch future broken callers, but I think that is a separate topic, and we don't have to do that as long as setup_work_tree(0 remains the only caller, as you said. ``` ## Bernhard R. Link, 2010-01-24 19:04 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <20100124190424.GA30585@pcpool00.mathematik.uni-freiburg.de> URL: https://gitlist.dev/e/20100124190424.GA30585%40pcpool00.mathematik.uni-freiburg.de In-Reply-To: <201001241457.43297.trast@student.ethz.ch> ``` * Thomas Rast [100124 14:58]: > POSIX specifies that multiple slashes must be treated as if they were > a single slash (except as in the next bullet point). Leading _double_ > slashes may be treated implementation-dependently. [1] Non-leading > double slashes do not seem to be specified. http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266 | 3.266 Pathname | | A character string that is used to identify a file. In the context of | IEEE Std 1003.1-2001, a pathname consists of, at most, {PATH_MAX} bytes, | including the terminating null byte. It has an optional beginning | slash, followed by zero or more filenames separated by slashes. A | pathname may optionally contain one or more trailing slashes. | Multiple successive slashes are considered to be the same as one | slash. ``` ## Junio C Hamano, 2010-01-24 20:05 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <7vr5pf5kw4.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr5pf5kw4.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100124190424.GA30585@pcpool00.mathematik.uni-freiburg.de> ``` "Bernhard R. Link" writes: > http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266 > > | 3.266 Pathname > | > | A character string that is used to identify a file. In the context of > | IEEE Std 1003.1-2001, a pathname consists of, at most, {PATH_MAX} bytes, > | including the terminating null byte. It has an optional beginning > | slash, followed by zero or more filenames separated by slashes. A > | pathname may optionally contain one or more trailing slashes. > | Multiple successive slashes are considered to be the same as one > | slash. Which is a bit stale, and there is an update that is crucial when discussing the issue that is the topic of this thread. Try this one instead, especially the last ", except for" which is an important clarification. http://www.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html | 3.266 Pathname | | A character string that is used to identify a file. In the context of | POSIX.1-2008, a pathname may be limited to {PATH_MAX} bytes, including the | terminating null byte. It has an optional beginning , followed by | zero or more filenames separated by characters. A pathname may | optionally contain one or more trailing characters. Multiple | successive characters are considered to be the same as one | , except for the case of exactly two leading characters. And 4.12 in the same issue of POSIX says pathnames with exactly two leading slashes may be handled in an implemenation defined manner (i.e. three or more means the same thing as a single slash). Confusing ;-) ``` ## Robin Rosenberg, 2010-01-25 01:06 Subject: Re: [PATCH] Handle double slashes in make_relative_path() Message-ID: <201001250206.48138.robin.rosenberg@dewire.com> URL: https://gitlist.dev/e/201001250206.48138.robin.rosenberg%40dewire.com In-Reply-To: <7v8wbn8ie2.fsf@alter.siamese.dyndns.org> ``` söndagen den 24 januari 2010 19.31.01 skrev Junio C Hamano: > Johannes Sixt writes: > > On Samstag, 23. Januar 2010, Johannes Sixt wrote: > >> On Samstag, 23. Januar 2010, Junio C Hamano wrote: > >> > What happens if you did this? > >> > > >> > git --git-dir=//git/repo/repo.git --work-tree=/git/repo > >> > > >> > where "//git/repo" is on the "git server" and you are working in local > >> > hierarchy "/git/repo"? > >> > >> Ah, right, this would not do the right thing. (But I can't verify this > >> claim right now.) > > > > I tested it, and it does the right thing. The reason is that before > > setup_work_tree() calls make_relative_path(), the --work-tree argument > > has been processed by make_absolute_path(), which adds the drive prefix. > > > > As long as setup_work_tree() remains the only caller of > > make_relative_path(), we are safe. > > Thanks; I think a more correct description of your findings is: > > - msysgit's make_absolute_path() does the right thing (i.e. adds "drive > prefix" to "git/repo" given to --work-tree); and > > - as long as the callers feed what the platform considers absolute paths > in abs and base, make_relative_path() does the right thing. > > So I think we are Ok. We _might_ want to add Windows-only test at the > beginning of make_relative_path() to make sure that the both inputs have > double-slashes at the beginning to catch future broken callers, but I > think that is a separate topic, and we don't have to do that as long as > setup_work_tree(0 remains the only caller, as you said. Separate patch posted since this problem occurs in more than one place. -- robin ```