{"thread":{"id":"35664","subject":"git-mv with absolute path derefereces symlinks","startedAt":"2014-01-15T12:48:59Z","lastAt":"2014-02-04T20:07:23Z","messageCount":56,"participants":["Martin Erik Werner","Torsten Bögershausen","Duy Nguyen","Junio C Hamano","David Kastrup"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"233158","messageId":"20140115124859.GA4283@mule","threadId":"35664","inReplyTo":null,"subject":"git-mv with absolute path derefereces symlinks","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-01-15T12:48:59Z","receivedAt":"2014-01-15T12:48:59Z","isPatch":false,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"Hi,\n\nIf git-mv is provided absolute paths when moving symlinks, it tries to\ndereference them and (attempts to) move the symlink target rather than the\nsymlink itself, this seems like a quite odd behaviour since it's inconsistent\nwith how git-mv works with symlinks if given relative paths, and I'm thinking\nit might be a bug, since it not documented in the git-mv manpage.\n\n###\n$ git init linktest\nInitialized empty Git repository in /home/arand/tmp/linktest/.git/\n$ cd linktest/\n$ touch target\n$ ln -s target link\n$ ln -s /tmp/target link2\n$ git add .\n$ git commit -m1\n[master (root-commit) 3cfea66] 1\n 3 files changed, 2 insertions(+)\n create mode 120000 link\n create mode 120000 link2\n create mode 100644 target\n$ git mv \"$(pwd)/link\" \"$(pwd)/moved\"\n$ git status\nOn branch master\nChanges to be committed:\n (use \"git reset HEAD ...\" to unstage)\n\n renamed: target -> moved\n\n$ git mv \"$(pwd)/link2\" \"$(pwd)/moved2\"\nfatal: /home/arand/tmp/linktest/link2: '/home/arand/tmp/linktest/link2' is outside repository\n###\n\n--\nMartin Erik Werner \n"},{"id":"233769","messageId":"1390746146-9478-1-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"20140115124859.GA4283@mule","subject":"[PATCH 0/2] in-tree symlink handling with absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-01-26T14:22:24Z","receivedAt":"2014-01-26T14:22:24Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"On Wed, 2014-01-15 at 13:48 +0100, Martin Erik Werner wrote:\n> If git-mv is provided absolute paths when moving symlinks, it tries to\n> dereference them and (attempts to) move the symlink target rather than the\n> symlink itself, this seems like a quite odd behaviour since it's inconsistent\n> with how git-mv works with symlinks if given relative paths, and I'm thinking\n> it might be a bug, since it not documented in the git-mv manpage.\n\nI've done a bit more digging into this: The issue applies to pretty much all\ncommands which can be given paths to files which are present in the work tree,\nso add, cat-file, rev-list, etc.\n\nI've written a test and a fix which seems to do the right thing.\n\nMartin Erik Werner (2):\n t0060: Add test for manipulating symlinks via absolute paths\n setup: Don't dereference in-tree symlinks for absolute paths\n\n setup.c | 54 ++++++++++++++++++++++++++++++++-------------------\n t/t0060-path-utils.sh | 7 +++++++\n 2 files changed, 41 insertions(+), 20 deletions(-)\n"},{"id":"233771","messageId":"1390746146-9478-2-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1390746146-9478-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH 1/2] t0060: Add test for manipulating symlinks via absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-01-26T14:22:25Z","receivedAt":"2014-01-26T14:22:25Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"When symlinks in the working tree are manipulated using the absolute\npath, git dereferences them, and tries to manipulate the link target\ninstead.\n\nThis is a regression introduced by 18e051a:\n setup: translate symlinks in filename when using absolute paths\n(which did not take symlinks in the work tree into consideration).\n\nAdd a known-breakage tests using the prefix_path function, which\ncurrently uses real_path, causing the dereference.\n\nSigned-off-by: Martin Erik Werner \n---\n t/t0060-path-utils.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 07c10c8..3a0677a 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -190,6 +190,13 @@ test_expect_success SYMLINKS 'real path works on symlinks' '\n \ttest \"$sym\" = \"$(test-path-utils real_path \"$dir2/syml\")\"\n '\n \n+test_expect_failure SYMLINKS 'prefix_path works with work tree symlinks' '\n+\n+\tln -s target symlink &&\n+\ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n+\n+'\n+\n relative_path /foo/a/b/c/\t/foo/a/b/\tc/\n relative_path /foo/a/b/c/\t/foo/a/b\tc/\n relative_path /foo/a//b//c/\t///foo/a/b//\tc/\t\tPOSIX\n-- \n1.8.5.2\n"},{"id":"233770","messageId":"1390746146-9478-3-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1390746146-9478-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH 2/2] setup: Don't dereference in-tree symlinks for absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-01-26T14:22:26Z","receivedAt":"2014-01-26T14:22:26Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"The prefix_path_gently() function currently applies real_path to\neverything if given an absolute path, dereferencing symlinks both\noutside and inside the work tree. In order to manipulate symliks in the\nwork tree using absolute paths, symlinks should only be dereferenced\noutside the work tree.\n\nModify prefix_path_gently() to first normalize the path in order to\nmake sure path levels are separated by '/', then use this separator to\ncheck the real path of each level of the path until it has found the\nlength that corresponds to the work tree.\n\nFor absolute paths, the function did not, nor does now do, any actual\nprefixing, hence we simply remove the path corresponding to the work\ntree and return the remaining in-tree part of the path.\n\nFixes t0060-82.\n\nSigned-off-by: Martin Erik Werner \n---\n setup.c | 54 ++++++++++++++++++++++++++++++++-------------------\n t/t0060-path-utils.sh | 2 +-\n 2 files changed, 35 insertions(+), 21 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 6c3f85f..bec587e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -22,11 +22,41 @@ char *prefix_path_gently(const char *prefix, int len,\n \tconst char *orig = path;\n \tchar *sanitized;\n \tif (is_absolute_path(orig)) {\n-\t\tconst char *temp = real_path(path);\n-\t\tsanitized = xmalloc(len + strlen(temp) + 1);\n-\t\tstrcpy(sanitized, temp);\n+\t\tchar npath[strlen(path)];\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = 0;\n+\t\tif (normalize_path_copy_len(npath, path, remaining_prefix))\n+\t\t\treturn NULL;\n+\t\tconst char *work_tree = get_git_work_tree();\n+\t\tif (!work_tree)\n+\t\t\treturn NULL;\n+\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\t\tstring_list_split(&list, npath, '/', -1);\n+\n+\t\tchar wtpart[strlen(npath) + 1];\n+\t\tint i = 0;\n+\t\tint match = 0;\n+\t\tstrcpy(wtpart, list.items[i++].string);\n+\t\tstrcpy(wtpart, \"/\");\n+\t\tif (strcmp(real_path(wtpart), work_tree) == 0) {\n+\t\t\tmatch = 1;\n+\t\t} else {\n+\t\t\twhile (i < list.nr) {\n+\t\t\t\tstrcat(wtpart, list.items[i++].string);\n+\t\t\t\tif (strcmp(real_path(wtpart), work_tree) == 0) {\n+\t\t\t\t\tmatch = 1;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t\tstrcat(wtpart, \"/\");\n+\t\t\t}\n+\t\t}\n+\t\tstring_list_clear(&list, 0);\n+\t\tif (!match)\n+\t\t\treturn NULL;\n+\n+\t\tsize_t wtpartlen = strlen(wtpart);\n+\t\tsanitized = xmalloc(strlen(npath) - wtpartlen);\n+\t\tstrcpy(sanitized, npath + wtpartlen + 1);\n \t} else {\n \t\tsanitized = xmalloc(len + strlen(path) + 1);\n \t\tif (len)\n@@ -34,26 +64,10 @@ char *prefix_path_gently(const char *prefix, int len,\n \t\tstrcpy(sanitized + len, path);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = len;\n-\t}\n-\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))\n-\t\tgoto error_out;\n-\tif (is_absolute_path(orig)) {\n-\t\tsize_t root_len, len, total;\n-\t\tconst char *work_tree = get_git_work_tree();\n-\t\tif (!work_tree)\n-\t\t\tgoto error_out;\n-\t\tlen = strlen(work_tree);\n-\t\troot_len = offset_1st_component(work_tree);\n-\t\ttotal = strlen(sanitized) + 1;\n-\t\tif (strncmp(sanitized, work_tree, len) ||\n-\t\t (len > root_len && sanitized[len] != '\\0' && sanitized[len] != '/')) {\n-\t\terror_out:\n+\t\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {\n \t\t\tfree(sanitized);\n \t\t\treturn NULL;\n \t\t}\n-\t\tif (sanitized[len] == '/')\n-\t\t\tlen++;\n-\t\tmemmove(sanitized, sanitized + len, total - len);\n \t}\n \treturn sanitized;\n }\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 3a0677a..03a12ac 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '\n \ttest \"$sym\" = \"$(test-path-utils real_path \"$dir2/syml\")\"\n '\n \n-test_expect_failure SYMLINKS 'prefix_path works with work tree symlinks' '\n+test_expect_success SYMLINKS 'prefix_path works with work tree symlinks' '\n \n \tln -s target symlink &&\n \ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n-- \n1.8.5.2\n"},{"id":"233774","messageId":"52E5439D.7060002@web.de","threadId":"35664","inReplyTo":"1390746146-9478-3-git-send-email-martinerikwerner@gmail.com","subject":"Re: [PATCH 2/2] setup: Don't dereference in-tree symlinks for absolute paths","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-01-26T17:19:25Z","receivedAt":"2014-01-26T17:19:25Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-01-26 15.22, Martin Erik Werner wrote:\n> The prefix_path_gently() function currently applies real_path to\n> everything if given an absolute path, dereferencing symlinks both\n> outside and inside the work tree. In order to manipulate symliks in the\n> work tree using absolute paths, symlinks should only be dereferenced\n> outside the work tree.\n> \n> Modify prefix_path_gently() to first normalize the path in order to\n> make sure path levels are separated by '/', then use this separator to\n> check the real path of each level of the path until it has found the\n> length that corresponds to the work tree.\n> \n> For absolute paths, the function did not, nor does now do, any actual\n> prefixing, hence we simply remove the path corresponding to the work\n> tree and return the remaining in-tree part of the path.\n> \n> Fixes t0060-82.\n> \n> Signed-off-by: Martin Erik Werner \n> ---\n> setup.c | 54 ++++++++++++++++++++++++++++++++-------------------\n> t/t0060-path-utils.sh | 2 +-\n> 2 files changed, 35 insertions(+), 21 deletions(-)\n> \n> diff --git a/setup.c b/setup.c\n> index 6c3f85f..bec587e 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -22,11 +22,41 @@ char *prefix_path_gently(const char *prefix, int len,\n> \tconst char *orig = path;\n> \tchar *sanitized;\n> \tif (is_absolute_path(orig)) {\n> -\t\tconst char *temp = real_path(path);\n> -\t\tsanitized = xmalloc(len + strlen(temp) + 1);\n> -\t\tstrcpy(sanitized, temp);\n> +\t\tchar npath[strlen(path)];\nIs this portable ?\nThis is variable-length array, isn't it ?\nUsing xmalloc() may be better\n> \t\tif (remaining_prefix)\n> \t\t\t*remaining_prefix = 0;\n> +\t\tif (normalize_path_copy_len(npath, path, remaining_prefix))\n> +\t\t\treturn NULL;\n> +\t\tconst char *work_tree = get_git_work_tree();\ndeclaration after statements should be avoided (not only here)\n"},{"id":"233801","messageId":"1390781250-20389-1-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"52E5439D.7060002@web.de","subject":"Re: [PATCH 2/2] setup: Don't dereference in-tree symlinks for absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-01-27T00:07:29Z","receivedAt":"2014-01-27T00:07:29Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"On Sun, Jan 26, 2014 at 06:19:25PM +0100, Torsten Bögershausen wrote:\n> On 2014-01-26 15.22, Martin Erik Werner wrote:\n> > The prefix_path_gently() function currently applies real_path to\n> > everything if given an absolute path, dereferencing symlinks both\n> > outside and inside the work tree. In order to manipulate symliks in the\n> > work tree using absolute paths, symlinks should only be dereferenced\n> > outside the work tree.\n> > \n> > Modify prefix_path_gently() to first normalize the path in order to\n> > make sure path levels are separated by '/', then use this separator to\n> > check the real path of each level of the path until it has found the\n> > length that corresponds to the work tree.\n> > \n> > For absolute paths, the function did not, nor does now do, any actual\n> > prefixing, hence we simply remove the path corresponding to the work\n> > tree and return the remaining in-tree part of the path.\n> > \n> > Fixes t0060-82.\n> > \n> > Signed-off-by: Martin Erik Werner \n> > ---\n> > setup.c | 54 ++++++++++++++++++++++++++++++++-------------------\n> > t/t0060-path-utils.sh | 2 +-\n> > 2 files changed, 35 insertions(+), 21 deletions(-)\n> > \n> > diff --git a/setup.c b/setup.c\n> > index 6c3f85f..bec587e 100644\n> > --- a/setup.c\n> > +++ b/setup.c\n> > @@ -22,11 +22,41 @@ char *prefix_path_gently(const char *prefix, int len,\n> > \tconst char *orig = path;\n> > \tchar *sanitized;\n> > \tif (is_absolute_path(orig)) {\n> > -\t\tconst char *temp = real_path(path);\n> > -\t\tsanitized = xmalloc(len + strlen(temp) + 1);\n> > -\t\tstrcpy(sanitized, temp);\n> > +\t\tchar npath[strlen(path)];\n> Is this portable ?\n> This is variable-length array, isn't it ?\n> Using xmalloc() may be better\nAh, right, that looks bad now that you mention it.\n> > \t\tif (remaining_prefix)\n> > \t\t\t*remaining_prefix = 0;\n> > +\t\tif (normalize_path_copy_len(npath, path, remaining_prefix))\n> > +\t\t\treturn NULL;\n> > +\t\tconst char *work_tree = get_git_work_tree();\n> declaration after statements should be avoided (not only here)\n> \nIndeed, I somehow guessed that declaration-after-statement was ok and\ntried to keep them close to usage, bad guess, evidently.\n\nI've rerolled the last v1 patch accordingly:\n* Use xmalloc() when initializing char* from strlen()\n* Separate and move declarations to beginning of scope\n* Fix a strcpy that should've been a strcat (which would've nuked DOS\n prefixes, I think)\n\n [PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths\n\n setup.c | 64 +++++++++++++++++++++++++++++++++++----------------\n t/t0060-path-utils.sh | 2 +-\n 2 files changed, 45 insertions(+), 21 deletions(-)\n\n--\nMartin Erik Werner \n"},{"id":"233802","messageId":"1390781250-20389-2-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1390781250-20389-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-01-27T00:07:30Z","receivedAt":"2014-01-27T00:07:30Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"The prefix_path_gently() function currently applies real_path to\neverything if given an absolute path, dereferencing symlinks both\noutside and inside the work tree. In order to manipulate symliks in the\nwork tree using absolute paths, symlinks should only be dereferenced\noutside the work tree.\n\nModify prefix_path_gently() to first normalize the path in order to\nmake sure path levels are separated by '/', then use this separator to\ncheck the real path of each level of the path until it has found the\nlength that corresponds to the work tree.\n\nFor absolute paths, the function did not, nor does now do, any actual\nprefixing, hence we simply remove the path corresponding to the work\ntree and return the remaining in-tree part of the path.\n\nFixes t0060-82.\n\nSigned-off-by: Martin Erik Werner \n---\n setup.c | 64 +++++++++++++++++++++++++++++++++++----------------\n t/t0060-path-utils.sh | 2 +-\n 2 files changed, 45 insertions(+), 21 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 5432a31..0789a96 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -22,11 +22,51 @@ char *prefix_path_gently(const char *prefix, int len,\n \tconst char *orig = path;\n \tchar *sanitized;\n \tif (is_absolute_path(orig)) {\n-\t\tconst char *temp = real_path(path);\n-\t\tsanitized = xmalloc(len + strlen(temp) + 1);\n-\t\tstrcpy(sanitized, temp);\n+\t\tint i, match;\n+\t\tsize_t wtpartlen;\n+\t\tchar *npath, *wtpart;\n+\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\t\tconst char *work_tree = get_git_work_tree();\n+\t\tif (!work_tree)\n+\t\t\treturn NULL;\n+\t\tnpath = xmalloc(strlen(path) + 1);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = 0;\n+\t\tif (normalize_path_copy_len(npath, path, remaining_prefix)) {\n+\t\t\tfree(npath);\n+\t\t\treturn NULL;\n+\t\t}\n+\n+\t\tstring_list_split(&list, npath, '/', -1);\n+\t\twtpart = xmalloc(strlen(npath) + 1);\n+\t\ti = 0;\n+\t\tmatch = 0;\n+\t\tstrcpy(wtpart, list.items[i++].string);\n+\t\tstrcat(wtpart, \"/\");\n+\t\tif (strcmp(real_path(wtpart), work_tree) == 0) {\n+\t\t\tmatch = 1;\n+\t\t} else {\n+\t\t\twhile (i < list.nr) {\n+\t\t\t\tstrcat(wtpart, list.items[i++].string);\n+\t\t\t\tif (strcmp(real_path(wtpart), work_tree) == 0) {\n+\t\t\t\t\tmatch = 1;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t\tstrcat(wtpart, \"/\");\n+\t\t\t}\n+\t\t}\n+\t\tstring_list_clear(&list, 0);\n+\t\tif (!match) {\n+\t\t\tfree(npath);\n+\t\t\tfree(wtpart);\n+\t\t\treturn NULL;\n+\t\t}\n+\n+\t\twtpartlen = strlen(wtpart);\n+\t\tsanitized = xmalloc(strlen(npath) - wtpartlen);\n+\t\tstrcpy(sanitized, npath + wtpartlen + 1);\n+\t\tfree(npath);\n+\t\tfree(wtpart);\n \t} else {\n \t\tsanitized = xmalloc(len + strlen(path) + 1);\n \t\tif (len)\n@@ -34,26 +74,10 @@ char *prefix_path_gently(const char *prefix, int len,\n \t\tstrcpy(sanitized + len, path);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = len;\n-\t}\n-\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))\n-\t\tgoto error_out;\n-\tif (is_absolute_path(orig)) {\n-\t\tsize_t root_len, len, total;\n-\t\tconst char *work_tree = get_git_work_tree();\n-\t\tif (!work_tree)\n-\t\t\tgoto error_out;\n-\t\tlen = strlen(work_tree);\n-\t\troot_len = offset_1st_component(work_tree);\n-\t\ttotal = strlen(sanitized) + 1;\n-\t\tif (strncmp(sanitized, work_tree, len) ||\n-\t\t (len > root_len && sanitized[len] != '\\0' && sanitized[len] != '/')) {\n-\t\terror_out:\n+\t\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {\n \t\t\tfree(sanitized);\n \t\t\treturn NULL;\n \t\t}\n-\t\tif (sanitized[len] == '/')\n-\t\t\tlen++;\n-\t\tmemmove(sanitized, sanitized + len, total - len);\n \t}\n \treturn sanitized;\n }\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 3a0677a..03a12ac 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '\n \ttest \"$sym\" = \"$(test-path-utils real_path \"$dir2/syml\")\"\n '\n \n-test_expect_failure SYMLINKS 'prefix_path works with work tree symlinks' '\n+test_expect_success SYMLINKS 'prefix_path works with work tree symlinks' '\n \n \tln -s target symlink &&\n \ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n-- \n1.8.5.2\n"},{"id":"233805","messageId":"CACsJy8AwBXtYTbQhcAfYgwuEGrzbMG9SDo8JOpSVF945O7VPyQ@mail.gmail.com","threadId":"35664","inReplyTo":"1390781250-20389-2-git-send-email-martinerikwerner@gmail.com","subject":"Re: [PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-01-27T00:49:42Z","receivedAt":"2014-01-27T00:49:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jan 27, 2014 at 7:07 AM, Martin Erik Werner\n wrote:\n> diff --git a/setup.c b/setup.c\n> index 5432a31..0789a96 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -22,11 +22,51 @@ char *prefix_path_gently(const char *prefix, int len,\n> const char *orig = path;\n> char *sanitized;\n> if (is_absolute_path(orig)) {\n> - const char *temp = real_path(path);\n> - sanitized = xmalloc(len + strlen(temp) + 1);\n> - strcpy(sanitized, temp);\n> + int i, match;\n> + size_t wtpartlen;\n> + char *npath, *wtpart;\n> + struct string_list list = STRING_LIST_INIT_DUP;\n> + const char *work_tree = get_git_work_tree();\n> + if (!work_tree)\n> + return NULL;\n> + npath = xmalloc(strlen(path) + 1);\n> if (remaining_prefix)\n> *remaining_prefix = 0;\n> + if (normalize_path_copy_len(npath, path, remaining_prefix)) {\n> + free(npath);\n> + return NULL;\n> + }\n> +\n> + string_list_split(&list, npath, '/', -1);\n> + wtpart = xmalloc(strlen(npath) + 1);\n> + i = 0;\n> + match = 0;\n\n> + strcpy(wtpart, list.items[i++].string);\n> + strcat(wtpart, \"/\");\n> + if (strcmp(real_path(wtpart), work_tree) == 0) {\n> + match = 1;\n> + } else {\n\nCould we remove this part and let the while loop handle the first path\ncomponent too? The only difference I see is if this code matches, we\nhave a trailing slash, while the \"while\" loop does not have a trailing\nslash in wtpart.\n\n> + while (i < list.nr) {\n> + strcat(wtpart, list.items[i++].string);\n> + if (strcmp(real_path(wtpart), work_tree) == 0) {\n> + match = 1;\n> + break;\n> + }\n> + strcat(wtpart, \"/\");\n> + }\n> + }\n> + string_list_clear(&list, 0);\n> + if (!match) {\n> + free(npath);\n> + free(wtpart);\n> + return NULL;\n> + }\n> +\n> + wtpartlen = strlen(wtpart);\n> + sanitized = xmalloc(strlen(npath) - wtpartlen);\n> + strcpy(sanitized, npath + wtpartlen + 1);\n\nThis \"+ 1\" is to ignore '/', isn't it? If so we should not do if match\nis set 1 outside \"while\" loop.\n\n> + free(npath);\n> + free(wtpart);\n\nAll this new code looks long enough to be a separate function with a\nmeaningful name. And we could travese through each path component in\nnpath without wtpart (replacing '/' with '\\0' to terminate the string\ntemporarily for real_path()). But it's up to you. Whichever way is\neasier to read to you.\n\n> } else {\n> sanitized = xmalloc(len + strlen(path) + 1);\n> if (len)\n> @@ -34,26 +74,10 @@ char *prefix_path_gently(const char *prefix, int len,\n> strcpy(sanitized + len, path);\n> if (remaining_prefix)\n> *remaining_prefix = len;\n> - }\n> - if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))\n> - goto error_out;\n> - if (is_absolute_path(orig)) {\n> - size_t root_len, len, total;\n> - const char *work_tree = get_git_work_tree();\n> - if (!work_tree)\n> - goto error_out;\n> - len = strlen(work_tree);\n> - root_len = offset_1st_component(work_tree);\n> - total = strlen(sanitized) + 1;\n> - if (strncmp(sanitized, work_tree, len) ||\n> - (len > root_len && sanitized[len] != '\\0' && sanitized[len] != '/')) {\n> - error_out:\n> + if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {\n> free(sanitized);\n> return NULL;\n> }\n> - if (sanitized[len] == '/')\n> - len++;\n> - memmove(sanitized, sanitized + len, total - len);\n> }\n> return sanitized;\n> }\n-- \nDuy\n"},{"id":"233828","messageId":"xmqqd2jdpes6.fsf@gitster.dls.corp.google.com","threadId":"35664","inReplyTo":"1390781250-20389-2-git-send-email-martinerikwerner@gmail.com","subject":"Re: [PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-27T16:31:37Z","receivedAt":"2014-01-27T16:31:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Erik Werner writes:\n\n> In order to manipulate symliks in the\n> work tree using absolute paths, symlinks should only be dereferenced\n> outside the work tree.\n\nI agree 100% with this reasoning (modulo s/symliks/symlinks/).\n\nAs to the implementation, it looks a bit overly complicated,\nthough. I haven't tried writing the same myself, but \n\n * I suspect that strbuf would help simplifying the allocation and\n deallocation;\n\n * Also I suspect that use of string-list to split and then join is\n making the code unnecessarily complex. In Python/Perl, that would\n be a normal approach, but in C, it would be a lot simpler if you\n prepare a writable temporary in wtpart[], walk from left to right\n finding '/' and replacing it temporarily with NUL to terminate in\n order to check with real_path(), restore the NUL back to '/' to\n check deeper, and rinse and repeat.\n\n Having said that, I am not absolutely sure if the repeated\n calls to real_path() are doing the right thing, though ;-)\n\n> diff --git a/setup.c b/setup.c\n> index 5432a31..0789a96 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -22,11 +22,51 @@ char *prefix_path_gently(const char *prefix, int len,\n> \tconst char *orig = path;\n> \tchar *sanitized;\n> \tif (is_absolute_path(orig)) {\n> -\t\tconst char *temp = real_path(path);\n> -\t\tsanitized = xmalloc(len + strlen(temp) + 1);\n> -\t\tstrcpy(sanitized, temp);\n> +\t\tint i, match;\n> +\t\tsize_t wtpartlen;\n> +\t\tchar *npath, *wtpart;\n> +\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n> +\t\tconst char *work_tree = get_git_work_tree();\n> +\t\tif (!work_tree)\n> +\t\t\treturn NULL;\n> +\t\tnpath = xmalloc(strlen(path) + 1);\n> \t\tif (remaining_prefix)\n> \t\t\t*remaining_prefix = 0;\n> +\t\tif (normalize_path_copy_len(npath, path, remaining_prefix)) {\n> +\t\t\tfree(npath);\n> +\t\t\treturn NULL;\n> +\t\t}\n> +\n> +\t\tstring_list_split(&list, npath, '/', -1);\n> +\t\twtpart = xmalloc(strlen(npath) + 1);\n> +\t\ti = 0;\n> +\t\tmatch = 0;\n> +\t\tstrcpy(wtpart, list.items[i++].string);\n> +\t\tstrcat(wtpart, \"/\");\n> +\t\tif (strcmp(real_path(wtpart), work_tree) == 0) {\n> +\t\t\tmatch = 1;\n> +\t\t} else {\n> +\t\t\twhile (i < list.nr) {\n> +\t\t\t\tstrcat(wtpart, list.items[i++].string);\n> +\t\t\t\tif (strcmp(real_path(wtpart), work_tree) == 0) {\n> +\t\t\t\t\tmatch = 1;\n> +\t\t\t\t\tbreak;\n> +\t\t\t\t}\n> +\t\t\t\tstrcat(wtpart, \"/\");\n> +\t\t\t}\n> +\t\t}\n> +\t\tstring_list_clear(&list, 0);\n> +\t\tif (!match) {\n> +\t\t\tfree(npath);\n> +\t\t\tfree(wtpart);\n> +\t\t\treturn NULL;\n> +\t\t}\n> +\n> +\t\twtpartlen = strlen(wtpart);\n> +\t\tsanitized = xmalloc(strlen(npath) - wtpartlen);\n> +\t\tstrcpy(sanitized, npath + wtpartlen + 1);\n> +\t\tfree(npath);\n> +\t\tfree(wtpart);\n> \t} else {\n> \t\tsanitized = xmalloc(len + strlen(path) + 1);\n> \t\tif (len)\n> @@ -34,26 +74,10 @@ char *prefix_path_gently(const char *prefix, int len,\n> \t\tstrcpy(sanitized + len, path);\n> \t\tif (remaining_prefix)\n> \t\t\t*remaining_prefix = len;\n> -\t}\n> -\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))\n> -\t\tgoto error_out;\n> -\tif (is_absolute_path(orig)) {\n> -\t\tsize_t root_len, len, total;\n> -\t\tconst char *work_tree = get_git_work_tree();\n> -\t\tif (!work_tree)\n> -\t\t\tgoto error_out;\n> -\t\tlen = strlen(work_tree);\n> -\t\troot_len = offset_1st_component(work_tree);\n> -\t\ttotal = strlen(sanitized) + 1;\n> -\t\tif (strncmp(sanitized, work_tree, len) ||\n> -\t\t (len > root_len && sanitized[len] != '\\0' && sanitized[len] != '/')) {\n> -\t\terror_out:\n> +\t\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {\n> \t\t\tfree(sanitized);\n> \t\t\treturn NULL;\n> \t\t}\n> -\t\tif (sanitized[len] == '/')\n> -\t\t\tlen++;\n> -\t\tmemmove(sanitized, sanitized + len, total - len);\n> \t}\n> \treturn sanitized;\n> }\n> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\n> index 3a0677a..03a12ac 100755\n> --- a/t/t0060-path-utils.sh\n> +++ b/t/t0060-path-utils.sh\n> @@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '\n> \ttest \"$sym\" = \"$(test-path-utils real_path \"$dir2/syml\")\"\n> '\n> \n> -test_expect_failure SYMLINKS 'prefix_path works with work tree symlinks' '\n> +test_expect_success SYMLINKS 'prefix_path works with work tree symlinks' '\n> \n> \tln -s target symlink &&\n> \ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n"},{"id":"233991","messageId":"20140131202142.GA9731@mule","threadId":"35664","inReplyTo":"1390781250-20389-2-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v3 0/4] setup: Don't dereference in-tree symlinks for absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-01-31T20:21:42Z","receivedAt":"2014-01-31T20:21:42Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"On Mon, Jan 27, 2014 at 08:31:37AM -0800, Junio C Hamano wrote:\n> Martin Erik Werner writes:\n> \n> > In order to manipulate symliks in the\n> > work tree using absolute paths, symlinks should only be dereferenced\n> > outside the work tree.\n> \n> I agree 100% with this reasoning (modulo s/symliks/symlinks/).\n> \n> As to the implementation, it looks a bit overly complicated,\n> though. I haven't tried writing the same myself, but \n> \n> * I suspect that strbuf would help simplifying the allocation and\n> deallocation;\n> \n> * Also I suspect that use of string-list to split and then join is\n> making the code unnecessarily complex. In Python/Perl, that would\n> be a normal approach, but in C, it would be a lot simpler if you\n> prepare a writable temporary in wtpart[], walk from left to right\n> finding '/' and replacing it temporarily with NUL to terminate in\n> order to check with real_path(), restore the NUL back to '/' to\n> check deeper, and rinse and repeat.\n> \n> Having said that, I am not absolutely sure if the repeated\n> calls to real_path() are doing the right thing, though ;-)\n> \n> > diff --git a/setup.c b/setup.c\n> > index 5432a31..0789a96 100644\n> > --- a/setup.c\n> > +++ b/setup.c\n> > @@ -22,11 +22,51 @@ char *prefix_path_gently(const char *prefix, int len,\n> > const char *orig = path;\n> > char *sanitized;\n> > if (is_absolute_path(orig)) {\n> > - const char *temp = real_path(path);\n> > - sanitized = xmalloc(len + strlen(temp) + 1);\n> > - strcpy(sanitized, temp);\n> > + int i, match;\n> > + size_t wtpartlen;\n> > + char *npath, *wtpart;\n> > + struct string_list list = STRING_LIST_INIT_DUP;\n> > + const char *work_tree = get_git_work_tree();\n> > + if (!work_tree)\n> > + return NULL;\n> > + npath = xmalloc(strlen(path) + 1);\n> > if (remaining_prefix)\n> > *remaining_prefix = 0;\n> > + if (normalize_path_copy_len(npath, path, remaining_prefix)) {\n> > + free(npath);\n> > + return NULL;\n> > + }\n> > +\n> > + string_list_split(&list, npath, '/', -1);\n> > + wtpart = xmalloc(strlen(npath) + 1);\n> > + i = 0;\n> > + match = 0;\n> > + strcpy(wtpart, list.items[i++].string);\n> > + strcat(wtpart, \"/\");\n> > + if (strcmp(real_path(wtpart), work_tree) == 0) {\n> > + match = 1;\n> > + } else {\n> > + while (i < list.nr) {\n> > + strcat(wtpart, list.items[i++].string);\n> > + if (strcmp(real_path(wtpart), work_tree) == 0) {\n> > + match = 1;\n> > + break;\n> > + }\n> > + strcat(wtpart, \"/\");\n> > + }\n> > + }\n> > + string_list_clear(&list, 0);\n> > + if (!match) {\n> > + free(npath);\n> > + free(wtpart);\n> > + return NULL;\n> > + }\n> > +\n> > + wtpartlen = strlen(wtpart);\n> > + sanitized = xmalloc(strlen(npath) - wtpartlen);\n> > + strcpy(sanitized, npath + wtpartlen + 1);\n> > + free(npath);\n> > + free(wtpart);\n> > } else {\n> > sanitized = xmalloc(len + strlen(path) + 1);\n> > if (len)\n> > @@ -34,26 +74,10 @@ char *prefix_path_gently(const char *prefix, int len,\n> > strcpy(sanitized + len, path);\n> > if (remaining_prefix)\n> > *remaining_prefix = len;\n> > - }\n> > - if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))\n> > - goto error_out;\n> > - if (is_absolute_path(orig)) {\n> > - size_t root_len, len, total;\n> > - const char *work_tree = get_git_work_tree();\n> > - if (!work_tree)\n> > - goto error_out;\n> > - len = strlen(work_tree);\n> > - root_len = offset_1st_component(work_tree);\n> > - total = strlen(sanitized) + 1;\n> > - if (strncmp(sanitized, work_tree, len) ||\n> > - (len > root_len && sanitized[len] != '\\0' && sanitized[len] != '/')) {\n> > - error_out:\n> > + if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {\n> > free(sanitized);\n> > return NULL;\n> > }\n> > - if (sanitized[len] == '/')\n> > - len++;\n> > - memmove(sanitized, sanitized + len, total - len);\n> > }\n> > return sanitized;\n> > }\n> > diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\n> > index 3a0677a..03a12ac 100755\n> > --- a/t/t0060-path-utils.sh\n> > +++ b/t/t0060-path-utils.sh\n> > @@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '\n> > test \"$sym\" = \"$(test-path-utils real_path \"$dir2/syml\")\"\n> > '\n> > \n> > -test_expect_failure SYMLINKS 'prefix_path works with work tree symlinks' '\n> > +test_expect_success SYMLINKS 'prefix_path works with work tree symlinks' '\n> > \n> > ln -s target symlink &&\n> > test \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n\nOn Mon, Jan 27, 2014 at 07:49:42AM +0700, Duy Nguyen wrote:\n(...)\n> All this new code looks long enough to be a separate function with a\n> meaningful name. And we could travese through each path component in\n> npath without wtpart (replacing '/' with '\\0' to terminate the string\n> temporarily for real_path()). But it's up to you. Whichever way is\n> easier to read to you.\n(...)\n\nI've reworked my patchset to add a specialised 'abspath_part_inside_repo'\nfunction which does traversing and temporary NUL-separation as suggested.\n\nI've also added a new test for a regression which I managed to hit.\n\nI'm not particularly happy with the name of the function, but since\npath_inside_repo already exists for something quite different, it seemed\nimportant to distinguish it.\n\nI've not added any test cases specific to the function, since some of it is\nalready covered thought prefix_path, and some things I'm unsure if possible to\ntest sanely (e.g. work tree is /).\n\nRepeatedly calling 'real_path' feels somewhat clunky, but I haven't managed\nto come up with an alternative method that would work, since symlinks change\nthe path arbitrarily there's no way to separate the result of dereferencing two\nsymlinks at once.\n\nMartin Erik Werner (4):\n t0060: Add test for manipulating symlinks via absolute paths\n t0060: Add test for prefix_path when path == work tree\n setup: Add 'abspath_part_inside_repo' function\n setup: Don't dereference in-tree symlinks for absolute paths\n\n cache.h | 1 +\n setup.c | 99 ++++++++++++++++++++++++++++++++++++++++-----------\n t/t0060-path-utils.sh | 11 ++++++\n 3 files changed, 91 insertions(+), 20 deletions(-)\n\n-- \n1.8.5.2\n"},{"id":"233992","messageId":"20140131202201.GB9731@mule","threadId":"35664","inReplyTo":"1390781250-20389-2-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v3 1/4] t0060: Add test for manipulating symlinks via absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-01-31T20:22:01Z","receivedAt":"2014-01-31T20:22:01Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"When symlinks in the working tree are manipulated using the absolute\npath, git dereferences them, and tries to manipulate the link target\ninstead.\n\nThis is a regression introduced by 18e051a:\n setup: translate symlinks in filename when using absolute paths\n(which did not take symlinks in the work tree into consideration).\n\nAdd a known-breakage tests using the prefix_path function, which\ncurrently uses real_path, causing the dereference.\n\nSigned-off-by: Martin Erik Werner \n---\n t/t0060-path-utils.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 07c10c8..0bba988 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -190,6 +190,11 @@ test_expect_success SYMLINKS 'real path works on symlinks' '\n \ttest \"$sym\" = \"$(test-path-utils real_path \"$dir2/syml\")\"\n '\n \n+test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' '\n+\tln -s target symlink &&\n+\ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n+'\n+\n relative_path /foo/a/b/c/\t/foo/a/b/\tc/\n relative_path /foo/a/b/c/\t/foo/a/b\tc/\n relative_path /foo/a//b//c/\t///foo/a/b//\tc/\t\tPOSIX\n-- \n1.8.5.2\n"},{"id":"233993","messageId":"20140131202228.GC9731@mule","threadId":"35664","inReplyTo":"1390781250-20389-2-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v3 2/4] t0060: Add test for prefix_path when path == work tree","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-01-31T20:22:28Z","receivedAt":"2014-01-31T20:22:28Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"The current behaviour of prefix_path is to return an empty string if\nprefixing and absolute path that only contains exactly the work tree.\nThis behaviour is a potential regression point.\n\nSigned-off-by: Martin Erik Werner \n---\n t/t0060-path-utils.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 0bba988..b8e92e1 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree\n \ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n '\n \n+test_expect_success 'prefix_path works with only absolute path to work tree' '\n+\techo \"\" >expected &&\n+\ttest-path-utils prefix_path prefix \"$(pwd)\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n relative_path /foo/a/b/c/\t/foo/a/b/\tc/\n relative_path /foo/a/b/c/\t/foo/a/b\tc/\n relative_path /foo/a//b//c/\t///foo/a/b//\tc/\t\tPOSIX\n-- \n1.8.5.2\n"},{"id":"233994","messageId":"20140131202248.GD9731@mule","threadId":"35664","inReplyTo":"1390781250-20389-2-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-01-31T20:22:48Z","receivedAt":"2014-01-31T20:22:48Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"In order to extract the part of an absolute path which lies inside the\nrepo, it is not possible to directly use real_path, since that would\ndereference symlinks both outside and inside the work tree.\n\nAdd an 'abspath_part_inside_repo' function which incrementally checks\neach path level by temporarily NUL-terminating at each '/' and comparing\nagainst the work tree path. When a match is found, it copies the\nremainder (which will be the in-repo part) to a destination\nbuffer.\n\nThe path being the filesystem root or exactly equal to the work tree are\nspecial cases handled separately, since then there is no directory\nseparator between the work tree and in-repo part.\n\nSigned-off-by: Martin Erik Werner \n---\n cache.h | 1 +\n setup.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 64 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex ce377e1..242f27d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -426,6 +426,7 @@ extern void verify_filename(const char *prefix,\n \t\t\t int diagnose_misspelt_rev);\n extern void verify_non_filename(const char *prefix, const char *name);\n extern int path_inside_repo(const char *prefix, const char *path);\n+extern int abspath_part_inside_repo(char *dst, const char *path);\n \n #define INIT_DB_QUIET 0x0001\n \ndiff --git a/setup.c b/setup.c\nindex 5432a31..e606846 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -77,6 +77,69 @@ int path_inside_repo(const char *prefix, const char *path)\n \treturn 0;\n }\n \n+/*\n+ * It is ok if dst == src, but they should not overlap otherwise.\n+ * No checking if the path is in fact an absolute path is done, and it must\n+ * already be normalized.\n+ *\n+ * Find the part of an absolute path that lies inside the work tree by\n+ * dereferencing symlinks outside the work tree, for example:\n+ * /dir1/repo/dir2/file (work tree is /dir1/repo) -> dir2/file\n+ * /dir/file (work tree is /) -> dir/file\n+ * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2\n+ * /dir/repo (exactly equal to work tree) -> (empty string)\n+ */\n+int abspath_part_inside_repo(char *dst, const char* src)\n+{\n+\tsize_t len;\n+\tchar *dst0;\n+\tchar temp;\n+\n+\tconst char* work_tree = get_git_work_tree();\n+\tif (!work_tree)\n+\t\treturn -1;\n+\tlen = strlen(src);\n+\tdst0 = dst;\n+\n+\t// check root level\n+\tif (has_dos_drive_prefix(src)) {\n+\t\t*dst++ = *src++;\n+\t\t*dst++ = *src++;\n+\t\t*dst++ = *src++;\n+\t} else {\n+\t\t*dst++ = *src++;\n+\t}\n+\ttemp = *dst;\n+\t*dst = '\\0';\n+\tif (strcmp(real_path(dst0), work_tree) == 0) {\n+\t\t*dst = temp;\n+\t\tmemmove(dst0, src, len - (dst - dst0) + 1);\n+\t\treturn 0;\n+\t}\n+\t*dst = temp;\n+\n+\t// check each level\n+\twhile (*dst != '\\0') {\n+\t\t*dst++ = *src++;\n+\t\tif (*dst == '/') {\n+\t\t\t*dst = '\\0';\n+\t\t\tif (strcmp(real_path(dst0), work_tree) == 0) {\n+\t\t\t\tmemmove(dst0, src + 1, len - (dst - dst0));\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t\t*dst = '/';\n+\t\t}\n+\t}\n+\n+\t// check whole path\n+\tif (strcmp(real_path(dst0), work_tree) == 0) {\n+\t\t*dst0 = '\\0';\n+\t\treturn 0;\n+\t}\n+\n+\treturn -1;\n+}\n+\n int check_filename(const char *prefix, const char *arg)\n {\n \tconst char *name;\n-- \n1.8.5.2\n"},{"id":"233995","messageId":"20140131202313.GE9731@mule","threadId":"35664","inReplyTo":"1390781250-20389-2-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v3 4/4] setup: Don't dereference in-tree symlinks for absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-01-31T20:23:13Z","receivedAt":"2014-01-31T20:23:13Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"The 'prefix_path_gently' function currently applies real_path to\neverything if given an absolute path, dereferencing symlinks both\noutside and inside the work tree. In order to manipulate symlinks in the\nwork tree using absolute paths, symlinks should only be dereferenced\noutside the work tree.\n\nModify the 'prefix_path_gently' to first normalize the path in order to\nmake sure path levels are separated by '/', then pass the result to\n'abspath_part_inside_repo' to find the in-repo part (without dereferencing\nany symlinks inside the work tree).\n\nFor absolute paths, 'prefix_path_gently' did not, nor does now do, any\nactual prefixing, hence the result from 'abspath_part_in_repo' is\nreturned as-is.\n\nFixes t0060-82.\n\nSigned-off-by: Martin Erik Werner \n---\n setup.c | 36 ++++++++++++++++--------------------\n t/t0060-path-utils.sh | 2 +-\n 2 files changed, 17 insertions(+), 21 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex e606846..2e65b48 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -22,11 +22,23 @@ char *prefix_path_gently(const char *prefix, int len,\n \tconst char *orig = path;\n \tchar *sanitized;\n \tif (is_absolute_path(orig)) {\n-\t\tconst char *temp = real_path(path);\n-\t\tsanitized = xmalloc(len + strlen(temp) + 1);\n-\t\tstrcpy(sanitized, temp);\n+\t\tchar *npath;\n+\n+\t\tnpath = xmalloc(strlen(path) + 1);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = 0;\n+\t\tif (normalize_path_copy_len(npath, path, remaining_prefix)) {\n+\t\t\tfree(npath);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tif (abspath_part_inside_repo(npath, npath)) {\n+\t\t\tfree(npath);\n+\t\t\treturn NULL;\n+\t\t}\n+\n+\t\tsanitized = xmalloc(strlen(npath) + 1);\n+\t\tstrcpy(sanitized, npath);\n+\t\tfree(npath);\n \t} else {\n \t\tsanitized = xmalloc(len + strlen(path) + 1);\n \t\tif (len)\n@@ -34,26 +46,10 @@ char *prefix_path_gently(const char *prefix, int len,\n \t\tstrcpy(sanitized + len, path);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = len;\n-\t}\n-\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))\n-\t\tgoto error_out;\n-\tif (is_absolute_path(orig)) {\n-\t\tsize_t root_len, len, total;\n-\t\tconst char *work_tree = get_git_work_tree();\n-\t\tif (!work_tree)\n-\t\t\tgoto error_out;\n-\t\tlen = strlen(work_tree);\n-\t\troot_len = offset_1st_component(work_tree);\n-\t\ttotal = strlen(sanitized) + 1;\n-\t\tif (strncmp(sanitized, work_tree, len) ||\n-\t\t (len > root_len && sanitized[len] != '\\0' && sanitized[len] != '/')) {\n-\t\terror_out:\n+\t\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {\n \t\t\tfree(sanitized);\n \t\t\treturn NULL;\n \t\t}\n-\t\tif (sanitized[len] == '/')\n-\t\t\tlen++;\n-\t\tmemmove(sanitized, sanitized + len, total - len);\n \t}\n \treturn sanitized;\n }\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex b8e92e1..f6f378b 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '\n \ttest \"$sym\" = \"$(test-path-utils real_path \"$dir2/syml\")\"\n '\n \n-test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' '\n+test_expect_success SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' '\n \tln -s target symlink &&\n \ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n '\n-- \n1.8.5.2\n"},{"id":"233999","messageId":"52EC25A9.1050807@web.de","threadId":"35664","inReplyTo":"20140131202248.GD9731@mule","subject":"Re: [PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-01-31T22:37:29Z","receivedAt":"2014-01-31T22:37:29Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-01-31 21.22, Martin Erik Werner wrote:\n> In order to extract the part of an absolute path which lies inside the\n> repo, it is not possible to directly use real_path, since that would\n> dereference symlinks both outside and inside the work tree.\n>\n> Add an 'abspath_part_inside_repo' function which incrementally checks\n> each path level by temporarily NUL-terminating at each '/' and comparing\n> against the work tree path. When a match is found, it copies the\n> remainder (which will be the in-repo part) to a destination\n> buffer.\n>\n> The path being the filesystem root or exactly equal to the work tree are\n> special cases handled separately, since then there is no directory\n> separator between the work tree and in-repo part.\n>\n> Signed-off-by: Martin Erik Werner \n> ---\n> cache.h | 1 +\n> setup.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 64 insertions(+)\n>\n> diff --git a/cache.h b/cache.h\n> index ce377e1..242f27d 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -426,6 +426,7 @@ extern void verify_filename(const char *prefix,\n> \t\t\t int diagnose_misspelt_rev);\n> extern void verify_non_filename(const char *prefix, const char *name);\n> extern int path_inside_repo(const char *prefix, const char *path);\n> +extern int abspath_part_inside_repo(char *dst, const char *path);\nabspath_part_inside_repo() is only used in setup.c, isn't it?\nIn this case we don't need it in cache.h, it can be declared inside setup.c as\n\nstatic int abspath_part_inside_repo(char *dst, const char *path);\n(or \"static inline\" )\n\n-----------------\n(And not in this patch: see the final setup.c:)\n\n if (g) {\n free(npath);\n return NULL;\n }\n\nIf this is the only caller of abspath_part_inside_repo(),\nthen do we need npath 2 times as a parameter ?\nOr can we re-write it to look like this:\n\nstatic inline int abspath_part_inside_repo(char *path)\n[\n]\n"},{"id":"234005","messageId":"20140201013121.GA14729@mule","threadId":"35664","inReplyTo":"52EC25A9.1050807@web.de","subject":"Re: [PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-01T01:31:21Z","receivedAt":"2014-02-01T01:31:21Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"On Fri, Jan 31, 2014 at 11:37:29PM +0100, Torsten Bögershausen wrote:\n> On 2014-01-31 21.22, Martin Erik Werner wrote:\n> > In order to extract the part of an absolute path which lies inside the\n> > repo, it is not possible to directly use real_path, since that would\n> > dereference symlinks both outside and inside the work tree.\n> >\n> > Add an 'abspath_part_inside_repo' function which incrementally checks\n> > each path level by temporarily NUL-terminating at each '/' and comparing\n> > against the work tree path. When a match is found, it copies the\n> > remainder (which will be the in-repo part) to a destination\n> > buffer.\n> >\n> > The path being the filesystem root or exactly equal to the work tree are\n> > special cases handled separately, since then there is no directory\n> > separator between the work tree and in-repo part.\n> >\n> > Signed-off-by: Martin Erik Werner \n> > ---\n> > cache.h | 1 +\n> > setup.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 64 insertions(+)\n> >\n> > diff --git a/cache.h b/cache.h\n> > index ce377e1..242f27d 100644\n> > --- a/cache.h\n> > +++ b/cache.h\n> > @@ -426,6 +426,7 @@ extern void verify_filename(const char *prefix,\n> > \t\t\t int diagnose_misspelt_rev);\n> > extern void verify_non_filename(const char *prefix, const char *name);\n> > extern int path_inside_repo(const char *prefix, const char *path);\n> > +extern int abspath_part_inside_repo(char *dst, const char *path);\n> abspath_part_inside_repo() is only used in setup.c, isn't it?\n> In this case we don't need it in cache.h, it can be declared inside setup.c as\n> \n> static int abspath_part_inside_repo(char *dst, const char *path);\n> (or \"static inline\" )\n> \n> -----------------\n> (And not in this patch: see the final setup.c:)\n> \n> if (g) {\n> free(npath);\n> return NULL;\n> }\n> \n> If this is the only caller of abspath_part_inside_repo(),\n> then do we need npath 2 times as a parameter ?\n> Or can we re-write it to look like this:\n> \n> static inline int abspath_part_inside_repo(char *path)\n> [\n> ]\n\nI guess I've over-generalised it a bit too much, that should rather be\ndone if-and-when, I presume?\n\nIt is indeed only used in setup.c and only by the prefix_path_gently\nfunction so static inline then?\n\nHmm, for single-parameter it should suffice to simply move the parameter\ndown into the function, like so?:\n const char* src;\n src = dst;\nand carry on as before (obviously also renaming the variables sensibly),\nor did you have something else in mind?\n\n(I added two parameters since I was glancing at 'normalize_path_copy_len'\nfor inspiration, and was thinking about (purely theoretical) re-use in\nother cases rather than minimizing it for the time being.)\n\nWhat do you mean with the \"(And not in this patch\"... bit; what \"final\nsetup.c\"?\n\n--\nMartin Erik Werner \n"},{"id":"234016","messageId":"CACsJy8CaA6-4qc3BjNi0BCKPLBzh7Ttu6n7wpcj1ZknmEZW-MA@mail.gmail.com","threadId":"35664","inReplyTo":"20140131202248.GD9731@mule","subject":"Re: [PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-01T02:31:26Z","receivedAt":"2014-02-01T02:31:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 1, 2014 at 3:22 AM, Martin Erik Werner\n wrote:\n> diff --git a/setup.c b/setup.c\n> index 5432a31..e606846 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -77,6 +77,69 @@ int path_inside_repo(const char *prefix, const char *path)\n> return 0;\n> }\n>\n> +/*\n> + * It is ok if dst == src, but they should not overlap otherwise.\n> + * No checking if the path is in fact an absolute path is done, and it must\n> + * already be normalized.\n> + *\n> + * Find the part of an absolute path that lies inside the work tree by\n> + * dereferencing symlinks outside the work tree, for example:\n> + * /dir1/repo/dir2/file (work tree is /dir1/repo) -> dir2/file\n> + * /dir/file (work tree is /) -> dir/file\n> + * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2\n> + * /dir/repo (exactly equal to work tree) -> (empty string)\n> + */\n> +int abspath_part_inside_repo(char *dst, const char* src)\n> +{\n> + size_t len;\n> + char *dst0;\n> + char temp;\n> +\n> + const char* work_tree = get_git_work_tree();\n> + if (!work_tree)\n> + return -1;\n> + len = strlen(src);\n> + dst0 = dst;\n> +\n> + // check root level\n\nUm.. no C++ style comments. And there should be a test that work_tree\nis the prefix of src (common case). If so we can return early and do\nnot need to do real_path() on every path component.\n\n> + if (has_dos_drive_prefix(src)) {\n> + *dst++ = *src++;\n> + *dst++ = *src++;\n> + *dst++ = *src++;\n> + } else {\n> + *dst++ = *src++;\n> + }\n> + temp = *dst;\n> + *dst = '\\0';\n> + if (strcmp(real_path(dst0), work_tree) == 0) {\n> + *dst = temp;\n> + memmove(dst0, src, len - (dst - dst0) + 1);\n> + return 0;\n> + }\n> + *dst = temp;\n> +\n> + // check each level\n> + while (*dst != '\\0') {\n> + *dst++ = *src++;\n> + if (*dst == '/') {\n> + *dst = '\\0';\n> + if (strcmp(real_path(dst0), work_tree) == 0) {\n> + memmove(dst0, src + 1, len - (dst - dst0));\n> + return 0;\n> + }\n> + *dst = '/';\n> + }\n> + }\n> +\n> + // check whole path\n> + if (strcmp(real_path(dst0), work_tree) == 0) {\n> + *dst0 = '\\0';\n> + return 0;\n> + }\n> +\n> + return -1;\n> +}\n> +\n> int check_filename(const char *prefix, const char *arg)\n> {\n> const char *name;\n> --\n> 1.8.5.2\n\n\n\n-- \nDuy\n"},{"id":"234022","messageId":"1391306351-13237-1-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"20140131202142.GA9731@mule","subject":"[PATCH v4 0/4] Handling of in-tree symlinks for absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-02T01:59:07Z","receivedAt":"2014-02-02T01:59:07Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"Hmm, maybe fourth time's the ch...nevermind.\n\nOn Sat, Feb 01, 2014 at 02:31:21AM +0100, Martin Erik Werner wrote:\n> On Fri, Jan 31, 2014 at 11:37:29PM +0100, Torsten Bögershausen wrote:\n> > On 2014-01-31 21.22, Martin Erik Werner wrote:\n(...)\n> > > diff --git a/cache.h b/cache.h\n> > > index ce377e1..242f27d 100644\n> > > --- a/cache.h\n> > > +++ b/cache.h\n> > > @@ -426,6 +426,7 @@ extern void verify_filename(const char *prefix,\n> > > int diagnose_misspelt_rev);\n> > > extern void verify_non_filename(const char *prefix, const char *name);\n> > > extern int path_inside_repo(const char *prefix, const char *path);\n> > > +extern int abspath_part_inside_repo(char *dst, const char *path);\n> > abspath_part_inside_repo() is only used in setup.c, isn't it?\n> > In this case we don't need it in cache.h, it can be declared inside setup.c as\n> > \n> > static int abspath_part_inside_repo(char *dst, const char *path);\n> > (or \"static inline\" )\n> > \n> > -----------------\n> > (And not in this patch: see the final setup.c:)\n> > \n> > if (g) {\n> > free(npath);\n> > return NULL;\n> > }\n> > \n> > If this is the only caller of abspath_part_inside_repo(),\n> > then do we need npath 2 times as a parameter ?\n> > Or can we re-write it to look like this:\n> > \n> > static inline int abspath_part_inside_repo(char *path)\n> > [\n> > ]\n> \n> I guess I've over-generalised it a bit too much, that should rather be\n> done if-and-when, I presume?\n> \n> It is indeed only used in setup.c and only by the prefix_path_gently\n> function so static inline then?\n> \n> Hmm, for single-parameter it should suffice to simply move the parameter\n> down into the function, like so?:\n> const char* src;\n> src = dst;\n> and carry on as before (obviously also renaming the variables sensibly),\n> or did you have something else in mind?\n> \n> (I added two parameters since I was glancing at 'normalize_path_copy_len'\n> for inspiration, and was thinking about (purely theoretical) re-use in\n> other cases rather than minimizing it for the time being.)\n> \n> What do you mean with the \"(And not in this patch\"... bit; what \"final\n> setup.c\"?\n\nAs per Torsten's suggestions I've re-worked abspath_part_inside_repo function\nto only take one parameter and also put it as 'static inline' before\n'prefix_path_gently' since currently only used once. (The change turned\nout larger/nicer than I first guessed, since the 'src' pointer and copying\ncould be dropped completely.)\n\nOn Sat, Feb 01, 2014 at 09:31:26AM +0700, Duy Nguyen wrote:\n> On Sat, Feb 1, 2014 at 3:22 AM, Martin Erik Werner\n> wrote:\n(...)\n> > + // check root level\n> \n> Um.. no C++ style comments. And there should be a test that work_tree\n> is the prefix of src (common case). If so we can return early and do\n> not need to do real_path() on every path component.\n(...)\n\nOops, comments fixed.\n\nI've added the check for work tree as existing prefix, which also had the nice\nside-effect of checking the case of the work tree being the root of the\nfilesystem as a bonus.\n\nThis new single-buffer version also uses 'offset_1st_component' to move past\nthe root (since not having to worry about copying).\n\nMartin Erik Werner (4):\n t0060: Add test for manipulating symlinks via absolute paths\n t0060: Add test for prefix_path when path == work tree\n setup: Add 'abspath_part_inside_repo' function\n setup: Don't dereference in-tree symlinks for absolute paths\n\n setup.c | 93 ++++++++++++++++++++++++++++++++++++++++-----------\n t/t0060-path-utils.sh | 11 ++++++\n 2 files changed, 84 insertions(+), 20 deletions(-)\n\n-- \n1.8.5.2\n"},{"id":"234023","messageId":"1391306351-13237-2-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391306351-13237-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v4 1/4] t0060: Add test for manipulating symlinks via absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-02T01:59:08Z","receivedAt":"2014-02-02T01:59:08Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"When symlinks in the working tree are manipulated using the absolute\npath, git dereferences them, and tries to manipulate the link target\ninstead.\n\nThis is a regression introduced by 18e051a:\n setup: translate symlinks in filename when using absolute paths\n(which did not take symlinks in the work tree into consideration).\n\nAdd a known-breakage tests using the prefix_path function, which\ncurrently uses real_path, causing the dereference.\n\nSigned-off-by: Martin Erik Werner \n---\n t/t0060-path-utils.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 07c10c8..0bba988 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -190,6 +190,11 @@ test_expect_success SYMLINKS 'real path works on symlinks' '\n \ttest \"$sym\" = \"$(test-path-utils real_path \"$dir2/syml\")\"\n '\n \n+test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' '\n+\tln -s target symlink &&\n+\ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n+'\n+\n relative_path /foo/a/b/c/\t/foo/a/b/\tc/\n relative_path /foo/a/b/c/\t/foo/a/b\tc/\n relative_path /foo/a//b//c/\t///foo/a/b//\tc/\t\tPOSIX\n-- \n1.8.5.2\n"},{"id":"234024","messageId":"1391306351-13237-3-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391306351-13237-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v4 2/4] t0060: Add test for prefix_path when path == work tree","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-02T01:59:09Z","receivedAt":"2014-02-02T01:59:09Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"The current behaviour of prefix_path is to return an empty string if\nprefixing and absolute path that only contains exactly the work tree.\nThis behaviour is a potential regression point.\n\nSigned-off-by: Martin Erik Werner \n---\n t/t0060-path-utils.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 0bba988..b8e92e1 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree\n \ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n '\n \n+test_expect_success 'prefix_path works with only absolute path to work tree' '\n+\techo \"\" >expected &&\n+\ttest-path-utils prefix_path prefix \"$(pwd)\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n relative_path /foo/a/b/c/\t/foo/a/b/\tc/\n relative_path /foo/a/b/c/\t/foo/a/b\tc/\n relative_path /foo/a//b//c/\t///foo/a/b//\tc/\t\tPOSIX\n-- \n1.8.5.2\n"},{"id":"234025","messageId":"1391306351-13237-4-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391306351-13237-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-02T01:59:10Z","receivedAt":"2014-02-02T01:59:10Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"In order to extract the part of an absolute path which lies inside the\nrepo, it is not possible to directly use real_path, since that would\ndereference symlinks both outside and inside the work tree.\n\nAdd an 'abspath_part_inside_repo' function which first checks if the\nwork tree is already the prefix, then incrementally checks each path\nlevel by temporarily NUL-terminating at each '/' and comparing against\nthe work tree path. If a match is found, it overwrites the input path\nwith the remainder past the work tree (which will be the in-repo part).\n\nThe path being exactly equal to the work tree is handled separately,\nsince then there is no directory separator between the work tree and\nin-repo part.\n\nThis function is currently only intended for use in\n'prefix_path_gently'.\n\nSigned-off-by: Martin Erik Werner \n---\n setup.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 57 insertions(+)\n\ndiff --git a/setup.c b/setup.c\nindex 5432a31..e938785 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -6,6 +6,63 @@ static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n \n /*\n+ * No checking if the path is in fact an absolute path is done, and it must\n+ * already be normalized.\n+ *\n+ * Find the part of an absolute path that lies inside the work tree by\n+ * dereferencing symlinks outside the work tree, for example:\n+ * /dir1/repo/dir2/file (work tree is /dir1/repo) -> dir2/file\n+ * /dir/file (work tree is /) -> dir/file\n+ * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2\n+ * /dir/repo (exactly equal to work tree) -> (empty string)\n+ */\n+static inline int abspath_part_inside_repo(char *path)\n+{\n+\tsize_t len;\n+\tsize_t wtlen;\n+\tchar *path0;\n+\n+\tconst char* work_tree = get_git_work_tree();\n+\tif (!work_tree)\n+\t\treturn -1;\n+\twtlen = strlen(work_tree);\n+\tlen = strlen(path);\n+\n+\t/* check if work tree is already the prefix */\n+\tif (strncmp(path, work_tree, wtlen) == 0) {\n+\t\tif (path[wtlen] == '/')\n+\t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n+\t\telse\n+\t\t\t/* work tree is the root, or the whole path */\n+\t\t\tmemmove(path, path + wtlen, len - wtlen + 1);\n+\t\treturn 0;\n+\t}\n+\tpath0 = path;\n+\tpath += offset_1st_component(path);\n+\n+\t/* check each level */\n+\twhile (*path != '\\0') {\n+\t\tpath++;\n+\t\tif (*path == '/') {\n+\t\t\t*path = '\\0';\n+\t\t\tif (strcmp(real_path(path0), work_tree) == 0) {\n+\t\t\t\tmemmove(path0, path + 1, len - (path - path0));\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t\t*path = '/';\n+\t\t}\n+\t}\n+\n+\t/* check whole path */\n+\tif (strcmp(real_path(path0), work_tree) == 0) {\n+\t\t*path0 = '\\0';\n+\t\treturn 0;\n+\t}\n+\n+\treturn -1;\n+}\n+\n+/*\n * Normalize \"path\", prepending the \"prefix\" for relative paths. If\n * remaining_prefix is not NULL, return the actual prefix still\n * remains in the path. For example, prefix = sub1/sub2/ and path is\n-- \n1.8.5.2\n"},{"id":"234026","messageId":"1391306351-13237-5-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391306351-13237-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v4 4/4] setup: Don't dereference in-tree symlinks for absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-02T01:59:11Z","receivedAt":"2014-02-02T01:59:11Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"The 'prefix_path_gently' function currently applies real_path to\neverything if given an absolute path, dereferencing symlinks both\noutside and inside the work tree. In order to manipulate symlinks in the\nwork tree using absolute paths, symlinks should only be dereferenced\noutside the work tree.\n\nModify the 'prefix_path_gently' to first normalize the path in order to\nmake sure path levels are separated by '/', then pass the result to\n'abspath_part_inside_repo' to find the in-repo part (without dereferencing\nany symlinks inside the work tree).\n\nFor absolute paths, 'prefix_path_gently' did not, nor does now do, any\nactual prefixing, hence the result from 'abspath_part_in_repo' is\nreturned as-is.\n\nFixes t0060-82.\n\nSigned-off-by: Martin Erik Werner \n---\n setup.c | 36 ++++++++++++++++--------------------\n 1 file changed, 16 insertions(+), 20 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex e938785..2270bd4 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -79,11 +79,23 @@ char *prefix_path_gently(const char *prefix, int len,\n \tconst char *orig = path;\n \tchar *sanitized;\n \tif (is_absolute_path(orig)) {\n-\t\tconst char *temp = real_path(path);\n-\t\tsanitized = xmalloc(len + strlen(temp) + 1);\n-\t\tstrcpy(sanitized, temp);\n+\t\tchar *npath;\n+\n+\t\tnpath = xmalloc(strlen(path) + 1);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = 0;\n+\t\tif (normalize_path_copy_len(npath, path, remaining_prefix)) {\n+\t\t\tfree(npath);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tif (abspath_part_inside_repo(npath)) {\n+\t\t\tfree(npath);\n+\t\t\treturn NULL;\n+\t\t}\n+\n+\t\tsanitized = xmalloc(strlen(npath) + 1);\n+\t\tstrcpy(sanitized, npath);\n+\t\tfree(npath);\n \t} else {\n \t\tsanitized = xmalloc(len + strlen(path) + 1);\n \t\tif (len)\n@@ -91,26 +103,10 @@ char *prefix_path_gently(const char *prefix, int len,\n \t\tstrcpy(sanitized + len, path);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = len;\n-\t}\n-\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))\n-\t\tgoto error_out;\n-\tif (is_absolute_path(orig)) {\n-\t\tsize_t root_len, len, total;\n-\t\tconst char *work_tree = get_git_work_tree();\n-\t\tif (!work_tree)\n-\t\t\tgoto error_out;\n-\t\tlen = strlen(work_tree);\n-\t\troot_len = offset_1st_component(work_tree);\n-\t\ttotal = strlen(sanitized) + 1;\n-\t\tif (strncmp(sanitized, work_tree, len) ||\n-\t\t (len > root_len && sanitized[len] != '\\0' && sanitized[len] != '/')) {\n-\t\terror_out:\n+\t\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {\n \t\t\tfree(sanitized);\n \t\t\treturn NULL;\n \t\t}\n-\t\tif (sanitized[len] == '/')\n-\t\t\tlen++;\n-\t\tmemmove(sanitized, sanitized + len, total - len);\n \t}\n \treturn sanitized;\n }\n-- \n1.8.5.2\n"},{"id":"234027","messageId":"CACsJy8A8hardH7EaopY2Xu5Ob50mew3pJdqAr6fJsqyyhLQYMg@mail.gmail.com","threadId":"35664","inReplyTo":"1391306351-13237-4-git-send-email-martinerikwerner@gmail.com","subject":"Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-02T02:19:04Z","receivedAt":"2014-02-02T02:19:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner\n wrote:\n> + /* check if work tree is already the prefix */\n> + if (strncmp(path, work_tree, wtlen) == 0) {\n> + if (path[wtlen] == '/')\n> + memmove(path, path + wtlen + 1, len - wtlen);\n> + else\n> + /* work tree is the root, or the whole path */\n> + memmove(path, path + wtlen, len - wtlen + 1);\n> + return 0;\n> + }\n\nNo the 4th time is not the charm yet :) if path is \"/abc/defghi\" and\nwork_tree is \"/abc/def\" you don't want to return \"ghi\" as the prefix\nhere.\n\n> + path0 = path;\n> + path += offset_1st_component(path);\n> +\n> + /* check each level */\n> + while (*path != '\\0') {\n> + path++;\n\nTo me it looks like we could write\n\nfor (; *path; path++) {\n\nor even\n\nfor (path += offset_1st_component(path); *path; path++) {\n\nbut it's personal taste..\n\n> + if (*path == '/') {\n> + *path = '\\0';\n> + if (strcmp(real_path(path0), work_tree) == 0) {\n> + memmove(path0, path + 1, len - (path - path0));\n> + return 0;\n> + }\n> + *path = '/';\n> + }\n> + }\n> +\n> + /* check whole path */\n> + if (strcmp(real_path(path0), work_tree) == 0) {\n> + *path0 = '\\0';\n> + return 0;\n> + }\n\nI think this is already handled by the \"check if work tree is already\nthe prefix\" block.\n\n> +\n> + return -1;\n> +}\n> +\n> +/*\n> * Normalize \"path\", prepending the \"prefix\" for relative paths. If\n> * remaining_prefix is not NULL, return the actual prefix still\n> * remains in the path. For example, prefix = sub1/sub2/ and path is\n> --\n> 1.8.5.2\n>\n\n\n\n-- \nDuy\n"},{"id":"234028","messageId":"CACsJy8Drf5D273Tfaxpqjg==8jcB3iAaPKM4P0-ErF7mPCC1JQ@mail.gmail.com","threadId":"35664","inReplyTo":"CACsJy8A8hardH7EaopY2Xu5Ob50mew3pJdqAr6fJsqyyhLQYMg@mail.gmail.com","subject":"Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-02T02:23:32Z","receivedAt":"2014-02-02T02:23:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Feb 2, 2014 at 9:19 AM, Duy Nguyen wrote:\n>> + /* check whole path */\n>> + if (strcmp(real_path(path0), work_tree) == 0) {\n>> + *path0 = '\\0';\n>> + return 0;\n>> + }\n>\n> I think this is already handled by the \"check if work tree is already\n> the prefix\" block.\n\nNo, the \"check if..\" block does not do real_path() so we still need it\nhere. Sorry for the noise.\n-- \nDuy\n"},{"id":"234035","messageId":"20140202111330.GB14729@mule","threadId":"35664","inReplyTo":"CACsJy8A8hardH7EaopY2Xu5Ob50mew3pJdqAr6fJsqyyhLQYMg@mail.gmail.com","subject":"Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-02T11:13:30Z","receivedAt":"2014-02-02T11:13:30Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"On Sun, Feb 02, 2014 at 09:19:04AM +0700, Duy Nguyen wrote:\n> On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner\n> wrote:\n> > + /* check if work tree is already the prefix */\n> > + if (strncmp(path, work_tree, wtlen) == 0) {\n> > + if (path[wtlen] == '/')\n> > + memmove(path, path + wtlen + 1, len - wtlen);\n> > + else\n> > + /* work tree is the root, or the whole path */\n> > + memmove(path, path + wtlen, len - wtlen + 1);\n> > + return 0;\n> > + }\n> \n> No the 4th time is not the charm yet :) if path is \"/abc/defghi\" and\n> work_tree is \"/abc/def\" you don't want to return \"ghi\" as the prefix\n> here.\n\nAh indeed, this should catch that:\n\ndiff --git a/setup.c b/setup.c\nindex 2270bd4..5817875 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -32,9 +32,11 @@ static inline int abspath_part_inside_repo(char *path)\n \tif (strncmp(path, work_tree, wtlen) == 0) {\n \t\tif (path[wtlen] == '/')\n \t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n-\t\telse\n+\t\telse if (path[wtlen - 1] == '/' || path[wtlen] == '\\0')\n \t\t\t/* work tree is the root, or the whole path */\n \t\t\tmemmove(path, path + wtlen, len - wtlen + 1);\n+\t\telse\n+\t\t\treturn -1;\n \t\treturn 0;\n \t}\n \tpath0 = path;\n\nIs it worth adding a test for this as well?:\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex f6f378b..05d3366 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -201,6 +201,10 @@ test_expect_success 'prefix_path works with only absolute path to work tree' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'prefix_path rejects absolute path to dir with same beginning as work tree' '\n+\ttest_must_fail test-path-utils prefix_path prefix \"$(pwd)a\"\n+'\n+\n relative_path /foo/a/b/c/\t/foo/a/b/\tc/\n relative_path /foo/a/b/c/\t/foo/a/b\tc/\n relative_path /foo/a//b//c/\t///foo/a/b//\tc/\t\tPOSIX\n\n> > + path0 = path;\n> > + path += offset_1st_component(path);\n> > +\n> > + /* check each level */\n> > + while (*path != '\\0') {\n> > + path++;\n> \n> To me it looks like we could write\n> \n> for (; *path; path++) {\n> \n> or even\n> \n> for (path += offset_1st_component(path); *path; path++) {\n> \n> but it's personal taste..\n\nYeah, I think aesthetically I don't like cramming too much into the for loop:\n\nfor (path += offset_1st_component(path0) + 1; *path; path++) {\n\nneither leaving the init expression unused. So as long as it's just personal\ntaste I think I'll stick with the current while loop format. But I'll exchange\n(*path == '\\0') for (*path) though.\n\n--\nMartin Erik Werner \n"},{"id":"234037","messageId":"871tzl2209.fsf@fencepost.gnu.org","threadId":"35664","inReplyTo":"20140202111330.GB14729@mule","subject":"Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-02-02T11:21:58Z","receivedAt":"2014-02-02T11:21:58Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Martin Erik Werner writes:\n\n> On Sun, Feb 02, 2014 at 09:19:04AM +0700, Duy Nguyen wrote:\n>> On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner\n>> wrote:\n>> > + /* check if work tree is already the prefix */\n>> > + if (strncmp(path, work_tree, wtlen) == 0) {\n>> > + if (path[wtlen] == '/')\n>> > + memmove(path, path + wtlen + 1, len - wtlen);\n>> > + else\n>> > + /* work tree is the root, or the whole path */\n>> > + memmove(path, path + wtlen, len - wtlen + 1);\n>> > + return 0;\n>> > + }\n>> \n>> No the 4th time is not the charm yet :) if path is \"/abc/defghi\" and\n>> work_tree is \"/abc/def\" you don't want to return \"ghi\" as the prefix\n>> here.\n>\n> Ah indeed, this should catch that:\n>\n> diff --git a/setup.c b/setup.c\n> index 2270bd4..5817875 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -32,9 +32,11 @@ static inline int abspath_part_inside_repo(char *path)\n> \tif (strncmp(path, work_tree, wtlen) == 0) {\n> \t\tif (path[wtlen] == '/')\n> \t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n> -\t\telse\n> +\t\telse if (path[wtlen - 1] == '/' || path[wtlen] == '\\0')\n\nIs wtlen guaranteed to be nonzero?\n\n-- \nDavid Kastrup\n"},{"id":"234040","messageId":"52EE2DEC.4030400@web.de","threadId":"35664","inReplyTo":"871tzl2209.fsf@fencepost.gnu.org","subject":"Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-02-02T11:37:16Z","receivedAt":"2014-02-02T11:37:16Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-02-02 12.21, David Kastrup wrote:\n> Martin Erik Werner writes:\n> \n>> On Sun, Feb 02, 2014 at 09:19:04AM +0700, Duy Nguyen wrote:\n>>> On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner\n>>> wrote:\n>>>> + /* check if work tree is already the prefix */\n>>>> + if (strncmp(path, work_tree, wtlen) == 0) {\n>>>> + if (path[wtlen] == '/')\n>>>> + memmove(path, path + wtlen + 1, len - wtlen);\n>>>> + else\n>>>> + /* work tree is the root, or the whole path */\n>>>> + memmove(path, path + wtlen, len - wtlen + 1);\n>>>> + return 0;\n>>>> + }\n>>>\n>>> No the 4th time is not the charm yet :) if path is \"/abc/defghi\" and\n>>> work_tree is \"/abc/def\" you don't want to return \"ghi\" as the prefix\n>>> here.\n>>\n>> Ah indeed, this should catch that:\n>>\n>> diff --git a/setup.c b/setup.c\n>> index 2270bd4..5817875 100644\n>> --- a/setup.c\n>> +++ b/setup.c\n>> @@ -32,9 +32,11 @@ static inline int abspath_part_inside_repo(char *path)\n>> \tif (strncmp(path, work_tree, wtlen) == 0) {\n>> \t\tif (path[wtlen] == '/')\n>> \t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n>> -\t\telse\n>> +\t\telse if (path[wtlen - 1] == '/' || path[wtlen] == '\\0')\n> \n> Is wtlen guaranteed to be nonzero?\n> \nAnother comment:\nThe \"raw\" comparison with '/' is probably working well on all\nPOSIX/Linux/Unix systems.\n\nTo be more portable, the macro\nis_dir_sep()\ncan be used:\n\nif (is_dir_sep(path[wtlen]))\n"},{"id":"234043","messageId":"20140202120943.GA27787@mule","threadId":"35664","inReplyTo":"52EE2DEC.4030400@web.de","subject":"Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-02T12:09:43Z","receivedAt":"2014-02-02T12:09:43Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"On Sun, Feb 02, 2014 at 12:37:16PM +0100, Torsten Bögershausen wrote:\n> On 2014-02-02 12.21, David Kastrup wrote:\n> > Martin Erik Werner writes:\n> > \n> >> On Sun, Feb 02, 2014 at 09:19:04AM +0700, Duy Nguyen wrote:\n> >>> On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner\n> >>> wrote:\n> >>>> + /* check if work tree is already the prefix */\n> >>>> + if (strncmp(path, work_tree, wtlen) == 0) {\n> >>>> + if (path[wtlen] == '/')\n> >>>> + memmove(path, path + wtlen + 1, len - wtlen);\n> >>>> + else\n> >>>> + /* work tree is the root, or the whole path */\n> >>>> + memmove(path, path + wtlen, len - wtlen + 1);\n> >>>> + return 0;\n> >>>> + }\n> >>>\n> >>> No the 4th time is not the charm yet :) if path is \"/abc/defghi\" and\n> >>> work_tree is \"/abc/def\" you don't want to return \"ghi\" as the prefix\n> >>> here.\n> >>\n> >> Ah indeed, this should catch that:\n> >>\n> >> diff --git a/setup.c b/setup.c\n> >> index 2270bd4..5817875 100644\n> >> --- a/setup.c\n> >> +++ b/setup.c\n> >> @@ -32,9 +32,11 @@ static inline int abspath_part_inside_repo(char *path)\n> >> \tif (strncmp(path, work_tree, wtlen) == 0) {\n> >> \t\tif (path[wtlen] == '/')\n> >> \t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n> >> -\t\telse\n> >> +\t\telse if (path[wtlen - 1] == '/' || path[wtlen] == '\\0')\n> > \n> > Is wtlen guaranteed to be nonzero?\n\nHmm, am I incorrect in thinking\nif (!work_tree)\ntakes care of that?\n\n> Another comment:\n> The \"raw\" comparison with '/' is probably working well on all\n> POSIX/Linux/Unix systems.\n> \n> To be more portable, the macro\n> is_dir_sep()\n> can be used:\n> \n> if (is_dir_sep(path[wtlen]))\n\nSince the path is already normalized by 'normalize_path_copy_len' which\nseems to guarantee '/'-separation, I have assumed that this was\nunnecessary?\n\n--\nMartin Erik Werner \n"},{"id":"234045","messageId":"CACsJy8AWMr95fTS_TRkPiU_ety-KWYuimKj+SUAt-BRoAU0JTA@mail.gmail.com","threadId":"35664","inReplyTo":"20140202111330.GB14729@mule","subject":"Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-02T12:15:44Z","receivedAt":"2014-02-02T12:15:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Feb 2, 2014 at 6:13 PM, Martin Erik Werner\n wrote:\n> diff --git a/setup.c b/setup.c\n> index 2270bd4..5817875 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -32,9 +32,11 @@ static inline int abspath_part_inside_repo(char *path)\n> if (strncmp(path, work_tree, wtlen) == 0) {\n> if (path[wtlen] == '/')\n> memmove(path, path + wtlen + 1, len - wtlen);\n> - else\n> + else if (path[wtlen - 1] == '/' || path[wtlen] == '\\0')\n> /* work tree is the root, or the whole path */\n> memmove(path, path + wtlen, len - wtlen + 1);\n> + else\n> + return -1;\n\nNo, you can't return here. \"/abc/defghi\" might actually be a symlink\nto \"/abc/def\". If it does not match, let the following loop handle it.\n-- \nDuy\n"},{"id":"234047","messageId":"52EE39AD.80801@web.de","threadId":"35664","inReplyTo":"20140202120943.GA27787@mule","subject":"Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-02-02T12:27:25Z","receivedAt":"2014-02-02T12:27:25Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"> \n>> Another comment:\n>> The \"raw\" comparison with '/' is probably working well on all\n>> POSIX/Linux/Unix systems.\n>>\n>> To be more portable, the macro\n>> is_dir_sep()\n>> can be used:\n>>\n>> if (is_dir_sep(path[wtlen]))\n> \n> Since the path is already normalized by 'normalize_path_copy_len' which\n> seems to guarantee '/'-separation, I have assumed that this was\n> unnecessary?\n\nSo true, sorry for the noise.\n"},{"id":"234050","messageId":"1391358940-17373-1-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391306351-13237-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v5 0/5] Handling of in-tree symlinks for absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-02T16:35:35Z","receivedAt":"2014-02-02T16:35:35Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"Again!\n\n(It seems you missed to CC me in your first reply David, please do :)\n\nNew reroll, fixing the /dir/repoa and /dir/repolink -> /dir/repo issues noted by\nDuy, and adding corresponding tests. If work tree matches beginning of path but\nneeds further checking, it starts from the end of the work tree length, so as\nminimize the 'real_path' calls.\n\nMartin Erik Werner (5):\n t0060: Add test for manipulating symlinks via absolute paths\n t0060: Add test for prefix_path when path == work tree\n t0060: Add tests for prefix_path when path begins with work tree\n setup: Add 'abspath_part_inside_repo' function\n setup: Don't dereference in-tree symlinks for absolute paths\n\n setup.c | 100 ++++++++++++++++++++++++++++++++++++++++----------\n t/t0060-path-utils.sh | 21 +++++++++++\n 2 files changed, 101 insertions(+), 20 deletions(-)\n\n-- \n1.8.5.2\n"},{"id":"234051","messageId":"1391358940-17373-2-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391358940-17373-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v5 1/5] t0060: Add test for manipulating symlinks via absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-02T16:35:36Z","receivedAt":"2014-02-02T16:35:36Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"When symlinks in the working tree are manipulated using the absolute\npath, git dereferences them, and tries to manipulate the link target\ninstead.\n\nThis is a regression introduced by 18e051a:\n setup: translate symlinks in filename when using absolute paths\n(which did not take symlinks in the work tree into consideration).\n\nAdd a known-breakage tests using the prefix_path function, which\ncurrently uses real_path, causing the dereference.\n\nSigned-off-by: Martin Erik Werner \n---\n t/t0060-path-utils.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 07c10c8..0bba988 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -190,6 +190,11 @@ test_expect_success SYMLINKS 'real path works on symlinks' '\n \ttest \"$sym\" = \"$(test-path-utils real_path \"$dir2/syml\")\"\n '\n \n+test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' '\n+\tln -s target symlink &&\n+\ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n+'\n+\n relative_path /foo/a/b/c/\t/foo/a/b/\tc/\n relative_path /foo/a/b/c/\t/foo/a/b\tc/\n relative_path /foo/a//b//c/\t///foo/a/b//\tc/\t\tPOSIX\n-- \n1.8.5.2\n"},{"id":"234054","messageId":"1391358940-17373-3-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391358940-17373-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v5 2/5] t0060: Add test for prefix_path when path == work tree","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-02T16:35:37Z","receivedAt":"2014-02-02T16:35:37Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"The current behaviour of prefix_path is to return an empty string if\nprefixing and absolute path that only contains exactly the work tree.\nThis behaviour is a potential regression point.\n\nSigned-off-by: Martin Erik Werner \n---\n t/t0060-path-utils.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 0bba988..b8e92e1 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree\n \ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n '\n \n+test_expect_success 'prefix_path works with only absolute path to work tree' '\n+\techo \"\" >expected &&\n+\ttest-path-utils prefix_path prefix \"$(pwd)\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n relative_path /foo/a/b/c/\t/foo/a/b/\tc/\n relative_path /foo/a/b/c/\t/foo/a/b\tc/\n relative_path /foo/a//b//c/\t///foo/a/b//\tc/\t\tPOSIX\n-- \n1.8.5.2\n"},{"id":"234053","messageId":"1391358940-17373-4-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391358940-17373-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v5 3/5] t0060: Add tests for prefix_path when path begins with work tree","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-02T16:35:38Z","receivedAt":"2014-02-02T16:35:38Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"One edge-case that isn't currently checked in the tests is the beginning\nof the path matching the work tree, despite the target not actually\nbeing the work tree, for example:\n\n path = /dir/repoa\n work_tree = /dir/repo\n\nshould fail since the path is outside the repo. However, if /dir/repoa\nis in fact a symlink that points to /dir/repo, it should instead\nsucceed.\n\nAdd two tests covering these cases, since they might be potential\nregression points.\n---\n t/t0060-path-utils.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex b8e92e1..c0a14f6 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'prefix_path rejects absolute path to dir with same beginning as work tree' '\n+\ttest_must_fail test-path-utils prefix_path prefix \"$(pwd)a\"\n+'\n+\n+test_expect_success 'prefix_path works with absolute path to a symlink to work tree having same beginning as work tree' '\n+\tgit init repo &&\n+\tln -s repo repolink &&\n+\ttest \"a\" = \"$(cd repo && test-path-utils prefix_path prefix \"$(pwd)/../repolink/a\")\"\n+'\n+\n relative_path /foo/a/b/c/\t/foo/a/b/\tc/\n relative_path /foo/a/b/c/\t/foo/a/b\tc/\n relative_path /foo/a//b//c/\t///foo/a/b//\tc/\t\tPOSIX\n-- \n1.8.5.2\n"},{"id":"234055","messageId":"1391358940-17373-5-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391358940-17373-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-02T16:35:39Z","receivedAt":"2014-02-02T16:35:39Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"In order to extract the part of an absolute path which lies inside the\nrepo, it is not possible to directly use real_path, since that would\ndereference symlinks both outside and inside the work tree.\n\nAdd an 'abspath_part_inside_repo' function which first checks if the\nwork tree is already the prefix, then incrementally checks each path\nlevel by temporarily NUL-terminating at each '/' and comparing against\nthe work tree path. If a match is found, it overwrites the input path\nwith the remainder past the work tree (which will be the in-repo part).\n\nThe path being exactly equal to the work tree is handled separately,\nsince then there is no directory separator between the work tree and\nin-repo part.\n\nThis function is currently only intended for use in\n'prefix_path_gently'.\n\nSigned-off-by: Martin Erik Werner \n---\n setup.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 64 insertions(+)\n\ndiff --git a/setup.c b/setup.c\nindex 5432a31..a2e60ab 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -6,6 +6,70 @@ static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n \n /*\n+ * No checking if the path is in fact an absolute path is done, and it must\n+ * already be normalized.\n+ *\n+ * Find the part of an absolute path that lies inside the work tree by\n+ * dereferencing symlinks outside the work tree, for example:\n+ * /dir1/repo/dir2/file (work tree is /dir1/repo) -> dir2/file\n+ * /dir/file (work tree is /) -> dir/file\n+ * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2\n+ * /dir/repolink/file (repolink points to /dir/repo) -> file\n+ * /dir/repo (exactly equal to work tree) -> (empty string)\n+ */\n+static inline int abspath_part_inside_repo(char *path)\n+{\n+\tsize_t len;\n+\tsize_t wtlen;\n+\tchar *path0;\n+\tint off;\n+\n+\tconst char* work_tree = get_git_work_tree();\n+\tif (!work_tree)\n+\t\treturn -1;\n+\twtlen = strlen(work_tree);\n+\tlen = strlen(path);\n+\toff = 0;\n+\n+\t/* check if work tree is already the prefix */\n+\tif (strncmp(path, work_tree, wtlen) == 0) {\n+\t\tif (path[wtlen] == '/') {\n+\t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n+\t\t\treturn 0;\n+\t\t} else if (path[wtlen - 1] == '/' || path[wtlen] == '\\0') {\n+\t\t\t/* work tree is the root, or the whole path */\n+\t\t\tmemmove(path, path + wtlen, len - wtlen + 1);\n+\t\t\treturn 0;\n+\t\t}\n+\t\t/* work tree might match beginning of a symlink to work tree */\n+\t\toff = wtlen;\n+\t}\n+\tpath0 = path;\n+\tpath += offset_1st_component(path) + off;\n+\n+\t/* check each level */\n+\twhile (*path) {\n+\t\tpath++;\n+\t\tif (*path == '/') {\n+\t\t\t*path = '\\0';\n+\t\t\tif (strcmp(real_path(path0), work_tree) == 0) {\n+\t\t\t\tmemmove(path0, path + 1, len - (path - path0));\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t\t*path = '/';\n+\t\t}\n+\t}\n+\n+\t/* check whole path */\n+\tif (strcmp(real_path(path0), work_tree) == 0) {\n+\t\t*path0 = '\\0';\n+\t\treturn 0;\n+\t}\n+\n+\treturn -1;\n+}\n+\n+/*\n * Normalize \"path\", prepending the \"prefix\" for relative paths. If\n * remaining_prefix is not NULL, return the actual prefix still\n * remains in the path. For example, prefix = sub1/sub2/ and path is\n-- \n1.8.5.2\n"},{"id":"234052","messageId":"1391358940-17373-6-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391358940-17373-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v5 5/5] setup: Don't dereference in-tree symlinks for absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-02T16:35:40Z","receivedAt":"2014-02-02T16:35:40Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"The 'prefix_path_gently' function currently applies real_path to\neverything if given an absolute path, dereferencing symlinks both\noutside and inside the work tree. In order to manipulate symlinks in the\nwork tree using absolute paths, symlinks should only be dereferenced\noutside the work tree.\n\nModify the 'prefix_path_gently' to first normalize the path in order to\nmake sure path levels are separated by '/', then pass the result to\n'abspath_part_inside_repo' to find the in-repo part (without dereferencing\nany symlinks inside the work tree).\n\nFor absolute paths, 'prefix_path_gently' did not, nor does now do, any\nactual prefixing, hence the result from 'abspath_part_in_repo' is\nreturned as-is.\n\nFixes t0060-82.\n\nSigned-off-by: Martin Erik Werner \n---\n setup.c | 36 ++++++++++++++++--------------------\n t/t0060-path-utils.sh | 2 +-\n 2 files changed, 17 insertions(+), 21 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex a2e60ab..230505c 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -86,11 +86,23 @@ char *prefix_path_gently(const char *prefix, int len,\n \tconst char *orig = path;\n \tchar *sanitized;\n \tif (is_absolute_path(orig)) {\n-\t\tconst char *temp = real_path(path);\n-\t\tsanitized = xmalloc(len + strlen(temp) + 1);\n-\t\tstrcpy(sanitized, temp);\n+\t\tchar *npath;\n+\n+\t\tnpath = xmalloc(strlen(path) + 1);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = 0;\n+\t\tif (normalize_path_copy_len(npath, path, remaining_prefix)) {\n+\t\t\tfree(npath);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tif (abspath_part_inside_repo(npath)) {\n+\t\t\tfree(npath);\n+\t\t\treturn NULL;\n+\t\t}\n+\n+\t\tsanitized = xmalloc(strlen(npath) + 1);\n+\t\tstrcpy(sanitized, npath);\n+\t\tfree(npath);\n \t} else {\n \t\tsanitized = xmalloc(len + strlen(path) + 1);\n \t\tif (len)\n@@ -98,26 +110,10 @@ char *prefix_path_gently(const char *prefix, int len,\n \t\tstrcpy(sanitized + len, path);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = len;\n-\t}\n-\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))\n-\t\tgoto error_out;\n-\tif (is_absolute_path(orig)) {\n-\t\tsize_t root_len, len, total;\n-\t\tconst char *work_tree = get_git_work_tree();\n-\t\tif (!work_tree)\n-\t\t\tgoto error_out;\n-\t\tlen = strlen(work_tree);\n-\t\troot_len = offset_1st_component(work_tree);\n-\t\ttotal = strlen(sanitized) + 1;\n-\t\tif (strncmp(sanitized, work_tree, len) ||\n-\t\t (len > root_len && sanitized[len] != '\\0' && sanitized[len] != '/')) {\n-\t\terror_out:\n+\t\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {\n \t\t\tfree(sanitized);\n \t\t\treturn NULL;\n \t\t}\n-\t\tif (sanitized[len] == '/')\n-\t\t\tlen++;\n-\t\tmemmove(sanitized, sanitized + len, total - len);\n \t}\n \treturn sanitized;\n }\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex c0a14f6..f04b82d 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '\n \ttest \"$sym\" = \"$(test-path-utils real_path \"$dir2/syml\")\"\n '\n \n-test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' '\n+test_expect_success SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' '\n \tln -s target symlink &&\n \ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n '\n-- \n1.8.5.2\n"},{"id":"234070","messageId":"CACsJy8DX8bh2cAx+a_cJafAOYB7Ly=y28jAGo1L8NEmkWaZv=Q@mail.gmail.com","threadId":"35664","inReplyTo":"1391358940-17373-6-git-send-email-martinerikwerner@gmail.com","subject":"Re: [PATCH v5 5/5] setup: Don't dereference in-tree symlinks for absolute paths","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-02-03T04:15:57Z","receivedAt":"2014-02-03T04:15:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Feb 2, 2014 at 11:35 PM, Martin Erik Werner\n wrote:\n> diff --git a/setup.c b/setup.c\n> index a2e60ab..230505c 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -86,11 +86,23 @@ char *prefix_path_gently(const char *prefix, int len,\n> const char *orig = path;\n> char *sanitized;\n> if (is_absolute_path(orig)) {\n> - const char *temp = real_path(path);\n> - sanitized = xmalloc(len + strlen(temp) + 1);\n> - strcpy(sanitized, temp);\n> + char *npath;\n> +\n> + npath = xmalloc(strlen(path) + 1);\n> if (remaining_prefix)\n> *remaining_prefix = 0;\n> + if (normalize_path_copy_len(npath, path, remaining_prefix)) {\n> + free(npath);\n> + return NULL;\n> + }\n> + if (abspath_part_inside_repo(npath)) {\n> + free(npath);\n> + return NULL;\n> + }\n> +\n> + sanitized = xmalloc(strlen(npath) + 1);\n> + strcpy(sanitized, npath);\n> + free(npath);\n\nWe could replace these three lines with \"sanitized = npath;\". But it's\nnot a big deal imo. The rest of the series looks good.\n\nReviewed-by: Duy Nguyen \n\n> } else {\n> sanitized = xmalloc(len + strlen(path) + 1);\n> if (len)\n-- \nDuy\n"},{"id":"234111","messageId":"20140203131700.GA15607@mule","threadId":"35664","inReplyTo":"CACsJy8DX8bh2cAx+a_cJafAOYB7Ly=y28jAGo1L8NEmkWaZv=Q@mail.gmail.com","subject":"Re: [PATCH v5 5/5] setup: Don't dereference in-tree symlinks for absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-03T13:17:00Z","receivedAt":"2014-02-03T13:17:00Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"On Mon, Feb 03, 2014 at 11:15:57AM +0700, Duy Nguyen wrote:\n> On Sun, Feb 2, 2014 at 11:35 PM, Martin Erik Werner\n> wrote:\n> > diff --git a/setup.c b/setup.c\n> > index a2e60ab..230505c 100644\n> > --- a/setup.c\n> > +++ b/setup.c\n> > @@ -86,11 +86,23 @@ char *prefix_path_gently(const char *prefix, int len,\n> > const char *orig = path;\n> > char *sanitized;\n> > if (is_absolute_path(orig)) {\n> > - const char *temp = real_path(path);\n> > - sanitized = xmalloc(len + strlen(temp) + 1);\n> > - strcpy(sanitized, temp);\n> > + char *npath;\n> > +\n> > + npath = xmalloc(strlen(path) + 1);\n> > if (remaining_prefix)\n> > *remaining_prefix = 0;\n> > + if (normalize_path_copy_len(npath, path, remaining_prefix)) {\n> > + free(npath);\n> > + return NULL;\n> > + }\n> > + if (abspath_part_inside_repo(npath)) {\n> > + free(npath);\n> > + return NULL;\n> > + }\n> > +\n> > + sanitized = xmalloc(strlen(npath) + 1);\n> > + strcpy(sanitized, npath);\n> > + free(npath);\n> \n> We could replace these three lines with \"sanitized = npath;\". But it's\n> not a big deal imo. The rest of the series looks good.\n> \n> Reviewed-by: Duy Nguyen \n> \n> > } else {\n> > sanitized = xmalloc(len + strlen(path) + 1);\n> > if (len)\n> -- \n> Duy\n\nThank you for reviewing! And thanks Torsten and Junio Likewise. (And\nthanks Richard for initial triggering and brief discussion of the bug :)\n\nHmm, yeah I don't really know what to prefer out of a. Two mallocs with\nonly a minimal one returned or 2. Single, potentially non-minimal, malloc\nreturned, if it makes little difference, for simplicity the latter seems nicer.\n\nThen it seems like one could get rid of npath completely:\n\ndiff --git a/setup.c b/setup.c\nindex 230505c..dd120cd 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -88,21 +88,17 @@ char *prefix_path_gently(const char *prefix, int len,\n \tif (is_absolute_path(orig)) {\n \t\tchar *npath;\n \n-\t\tnpath = xmalloc(strlen(path) + 1);\n+\t\tsanitized = xmalloc(strlen(path) + 1);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = 0;\n-\t\tif (normalize_path_copy_len(npath, path, remaining_prefix)) {\n-\t\t\tfree(npath);\n+\t\tif (normalize_path_copy_len(sanitized, path, remaining_prefix)) {\n+\t\t\tfree(sanitized);\n \t\t\treturn NULL;\n \t\t}\n-\t\tif (abspath_part_inside_repo(npath)) {\n-\t\t\tfree(npath);\n+\t\tif (abspath_part_inside_repo(sanitized)) {\n+\t\t\tfree(sanitized);\n \t\t\treturn NULL;\n \t\t}\n-\n-\t\tsanitized = xmalloc(strlen(npath) + 1);\n-\t\tstrcpy(sanitized, npath);\n-\t\tfree(npath);\n \t} else {\n \t\tsanitized = xmalloc(len + strlen(path) + 1);\n \t\tif (len)\n\nat the cost of 'sanitized' always being the length of path, regardless\nif it's shorter, or even a NUL string.\n\n--\nMartin Erik Werner \n"},{"id":"234123","messageId":"xmqq7g9cf2ty.fsf@gitster.dls.corp.google.com","threadId":"35664","inReplyTo":"1391358940-17373-2-git-send-email-martinerikwerner@gmail.com","subject":"Re: [PATCH v5 1/5] t0060: Add test for manipulating symlinks via absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-03T18:50:17Z","receivedAt":"2014-02-03T18:50:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Erik Werner writes:\n\n> When symlinks in the working tree are manipulated using the absolute\n> path, git dereferences them, and tries to manipulate the link target\n> instead.\n\nThe above may a very good description of the root cause, but\ncan we have description of a symptom that is visible by end-users\nthat is caused by the bug being fixed with the series here, by\nending the above like so:\n\n\t... link target instead. This causes \"git foo bar\" to\n\tmisbehave in this and that way.\n\nTesting the low-level underlying machinery like this patch does is\nnot wrong per-se, but I suspect that this series was triggered by\nsomebody noticing breakage in a end-user command \"git foo $path\"\nwith $path being a full pathname to an in-tree symbolic link. It\nwouldn't be like somebody who was bored and ran \"test-path-utils\"\nfor fun noticed the root cause without realizing how the fix would\naffect the behaviour that would be visible to end-users, right?\n\nCan we have that \"git foo $path\" to the testsuite as well? That is\nthe breakage we do not want to repeat in the future by regressing.\n\nI am guessing \"foo\" is \"add\", but I wasn't closely following the\nprogression of the series.\n\nThanks.\n"},{"id":"234138","messageId":"xmqqtxcgdldq.fsf@gitster.dls.corp.google.com","threadId":"35664","inReplyTo":"xmqq7g9cf2ty.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 1/5] t0060: Add test for manipulating symlinks via absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-03T19:52:33Z","receivedAt":"2014-02-03T19:52:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Can we have that \"git foo $path\" to the testsuite as well? That is\n> the breakage we do not want to repeat in the future by regressing.\n\nSomething like this, perhaps?\n\n t/t3004-ls-files-basic.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh\nindex 8d9bc3c..d93089d 100755\n--- a/t/t3004-ls-files-basic.sh\n+++ b/t/t3004-ls-files-basic.sh\n@@ -36,4 +36,21 @@ test_expect_success 'ls-files -h in corrupt repository' '\n \ttest_i18ngrep \"[Uu]sage: git ls-files \" broken/usage\n '\n \n+test_expect_success SYMLINKS 'ls-files with symlinks' '\n+\tmkdir subs &&\n+\tln -s nosuch link &&\n+\tln -s ../nosuch subs/link &&\n+\tgit add link subs/link &&\n+\tgit ls-files -s link subs/link >expect &&\n+\tgit ls-files -s \"$(pwd)/link\" \"$(pwd)/subs/link\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t(\n+\t\tcd subs &&\n+\t\tgit ls-files -s link >../expect &&\n+\t\tgit ls-files -s \"$(pwd)/link\" >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"234147","messageId":"20140203201241.GB15607@mule","threadId":"35664","inReplyTo":"xmqq7g9cf2ty.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 1/5] t0060: Add test for manipulating symlinks via absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-03T20:12:41Z","receivedAt":"2014-02-03T20:12:41Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"On Mon, Feb 03, 2014 at 10:50:17AM -0800, Junio C Hamano wrote:\n> Martin Erik Werner writes:\n> \n> > When symlinks in the working tree are manipulated using the absolute\n> > path, git dereferences them, and tries to manipulate the link target\n> > instead.\n> \n> The above may a very good description of the root cause, but\n> can we have description of a symptom that is visible by end-users\n> that is caused by the bug being fixed with the series here, by\n> ending the above like so:\n> \n> \t... link target instead. This causes \"git foo bar\" to\n> \tmisbehave in this and that way.\n> \n> Testing the low-level underlying machinery like this patch does is\n> not wrong per-se, but I suspect that this series was triggered by\n> somebody noticing breakage in a end-user command \"git foo $path\"\n> with $path being a full pathname to an in-tree symbolic link. It\n> wouldn't be like somebody who was bored and ran \"test-path-utils\"\n> for fun noticed the root cause without realizing how the fix would\n> affect the behaviour that would be visible to end-users, right?\n> \n> Can we have that \"git foo $path\" to the testsuite as well? That is\n> the breakage we do not want to repeat in the future by regressing.\n> \n> I am guessing \"foo\" is \"add\", but I wasn't closely following the\n> progression of the series.\n> \n> Thanks.\n\nIndeed, it was first discovered via git-mv, (by Richard, using\ngit-annex) and me reproducing and reporting it was the start of the\nthread: http://thread.gmane.org/gmane.comp.version-control.git/240467\n\nIn going further (PATCHv0):\n> I've done a bit more digging into this: The issue applies to pretty\n> much all commands which can be given paths to files which are present\n> in the work tree, so add, cat-file, rev-list, etc.\n\nAt this stage I kind of dropped the reference to any specific top-level\ncommand since it seemed to apply to all of them in some way, and I\nfigured it made more sense with a generic explanation that would apply\nto all commands. But it might definitely be worth to mention it in order\nfor the commit messages to be less technical, and add at least one test\nwhich would actually trigger it in a user-manner. So for the\nexplanation, something like that?:\n\n\tThis causes for example 'git add /dir/repo/symlink' to attempt\n\tto add the target of the symlink rather than the symlink itself,\n\twhich is usually not what the user intends to do.\n\nHmm, come to think of it, I even made some of those tests back before I\nfound it could be narrowly tested via prefix_path... I guess I'll pick\nout the git-add one since it's the simplest, should that be added to\nt0060-path-utils.sh as well, or would it fit better in t3700-add.sh?:\n\n>From 910d8c9f51c3b3f2c03dbf15ce3cf7ea94de8d27 Mon Sep 17 00:00:00 2001\nFrom: Martin Erik Werner \nDate: Thu, 16 Jan 2014 00:24:43 +0100\nSubject: [PATCH] Add test for manipulating symlinks via absolute paths\n\nWhen symlinks in the working tree are manipulated using the absolute\npath, git derferences them, and tries to manipulate the link target\ninstead.\n\nAdd three known-breakage tests using add, mv, and rev-list which\nchecks this behaviour.\n\nThe failure of the git-add test is a regression introduced by 18e051a:\n setup: translate symlinks in filename when using absolute paths\n(which did not take symlinks in the work tree into consideration).\n---\n t/t0054-symlinks-via-abs-path.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 38 insertions(+)\n create mode 100755 t/t0054-symlinks-via-abs-path.sh\n\ndiff --git a/t/t0054-symlinks-via-abs-path.sh b/t/t0054-symlinks-via-abs-path.sh\nnew file mode 100755\nindex 0000000..0b3c91e\n--- /dev/null\n+++ b/t/t0054-symlinks-via-abs-path.sh\n@@ -0,0 +1,38 @@\n+#!/bin/sh\n+\n+test_description='symlinks via sbsolute paths\n+\n+This test checks the behavior when symlinks in the working tree are manipulated\n+via absolute path arguments.\n+'\n+. ./test-lib.sh\n+\n+test_expect_failure SYMLINKS 'git add symlink with absolute path' '\n+\n+\tln -s target symlink &&\n+\tgit add \"$(pwd)/symlink\"\n+\n+'\n+\n+rm -f symlink\n+\n+test_expect_failure SYMLINKS 'git mv symlink with absolute path' '\n+\n+\tln -s target symlink &&\n+\tgit add symlink &&\n+\tgit mv \"$(pwd)\"/symlink moved\n+\n+'\n+\n+rm -f symlink moved\n+\n+test_expect_failure 'git rev-list symlink with absolute path' '\n+\n+\tln -s target symlink &&\n+\tgit add symlink &&\n+\tgit commit -m show &&\n+\ttest \"$(git rev-list HEAD -- symlink)\" = \"$(git rev-list HEAD -- $(pwd)/symlink)\"\n+\n+'\n+\n+test_done\n-- \n1.8.5.2\n"},{"id":"234154","messageId":"xmqq1tzjewsf.fsf@gitster.dls.corp.google.com","threadId":"35664","inReplyTo":"1391358940-17373-5-git-send-email-martinerikwerner@gmail.com","subject":"Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-03T21:00:48Z","receivedAt":"2014-02-03T21:00:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Erik Werner writes:\n\n> The path being exactly equal to the work tree is handled separately,\n> since then there is no directory separator between the work tree and\n> in-repo part.\n\nWhat is an \"in-repo part\"? Whatever it is, I am not sure if I\nfollow that logic. After the while (*path) loop checks each level,\nyou check the whole path---wouldn't that code handle that special\ncase already?\n\nBecause it is probably the normal case not to have any symbolic link\nin the leading part (hence not having to dereference them), I think\nchecking \"is work_tree[] a prefix of path[]\" early is justified from\nperformance point of view, though.\n\n> /*\n> + * No checking if the path is in fact an absolute path is done, and it must\n> + * already be normalized.\n\nThis is not quite parsable to me.\n\n> + * Find the part of an absolute path that lies inside the work tree by\n> + * dereferencing symlinks outside the work tree, for example:\n> + * /dir1/repo/dir2/file (work tree is /dir1/repo) -> dir2/file\n> + * /dir/file (work tree is /) -> dir/file\n> + * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2\n> + * /dir/repolink/file (repolink points to /dir/repo) -> file\n> + * /dir/repo (exactly equal to work tree) -> (empty string)\n> + */\n> +static inline int abspath_part_inside_repo(char *path)\n\nIt looks a bit too big to be marked \"inline\"; better leave it to the\ncompiler to notice that there is only a single callsite and decide\nto (or not to) inline it.\n\n> +{\n> +\tsize_t len;\n> +\tsize_t wtlen;\n> +\tchar *path0;\n> +\tint off;\n> +\n> +\tconst char* work_tree = get_git_work_tree();\n> +\tif (!work_tree)\n> +\t\treturn -1;\n> +\twtlen = strlen(work_tree);\n> +\tlen = strlen(path);\n\nI expect that this is called from a callsite that _knows_ how long\npath[] is. Shouldn't len be a parameter to this function instead?\n\n> +\toff = 0;\n> +\n> +\t/* check if work tree is already the prefix */\n> +\tif (strncmp(path, work_tree, wtlen) == 0) {\n\nI wonder if we want to be explicit and compare wtlen and len before\neven attempting strncmp(). If work_tree is longer than path, it\ncannot be a prefix of path, right?\n\nIn other words, also with a style-fix to prefer !XXcmp() over\nXXcmp() == 0:\n\n\tif (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n\nperhaps? That would make it clear why referring to path[wtlen] on\nthe next line is permissible (it is obvious that it won't access\npast the end of path[]):\n\n> +\t\tif (path[wtlen] == '/') {\n> +\t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n> +\t\t\treturn 0;\n> +\t\t} else if (path[wtlen - 1] == '/' || path[wtlen] == '\\0') {\n> +\t\t\t/* work tree is the root, or the whole path */\n> +\t\t\tmemmove(path, path + wtlen, len - wtlen + 1);\n\nIf work_tree[] == \"/\", path[] == \"/a\", then len == 2, wtlen == 1,\npath[wtlen - 1] == '/' and this shifts path[] to the left by one,\nleaving path[] = \"a\", which is what we want. OK.\n\nIf work_tree[] == path[] == \"/a\", then len == wtlen == 2,\npath[wtlen] == '\\0', and this becomes equivalent to path[0] = '\\0'.\nHmph.... How do our callers treat an empty path? In other words,\nshould these three be equivalent?\n\n\tcd /a && git ls-files /a\n\tcd /a && git ls-files \"\"\n\tcd /a && git ls-files .\n\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t\t/* work tree might match beginning of a symlink to work tree */\n> +\t\toff = wtlen;\n> +\t}\n> +\tpath0 = path;\n> +\tpath += offset_1st_component(path) + off;\n> +\n> +\t/* check each level */\n> +\twhile (*path) {\n> +\t\tpath++;\n> +\t\tif (*path == '/') {\n> +\t\t\t*path = '\\0';\n> +\t\t\tif (strcmp(real_path(path0), work_tree) == 0) {\n> +\t\t\t\tmemmove(path0, path + 1, len - (path - path0));\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n> +\t\t\t*path = '/';\n> +\t\t}\n> +\t}\n> +\n> +\t/* check whole path */\n> +\tif (strcmp(real_path(path0), work_tree) == 0) {\n> +\t\t*path0 = '\\0';\n> +\t\treturn 0;\n> +\t}\n> +\n> +\treturn -1;\n> +}\n> +\n> +/*\n> * Normalize \"path\", prepending the \"prefix\" for relative paths. If\n> * remaining_prefix is not NULL, return the actual prefix still\n> * remains in the path. For example, prefix = sub1/sub2/ and path is\n"},{"id":"234168","messageId":"20140203231659.GC15607@mule","threadId":"35664","inReplyTo":"xmqq1tzjewsf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-03T23:16:59Z","receivedAt":"2014-02-03T23:16:59Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"On Mon, Feb 03, 2014 at 11:52:33AM -0800, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Can we have that \"git foo $path\" to the testsuite as well? That is\n> > the breakage we do not want to repeat in the future by regressing.\n> \n> Something like this, perhaps?\n> \n> t/t3004-ls-files-basic.sh | 17 +++++++++++++++++\n> 1 file changed, 17 insertions(+)\n> \n> diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh\n> index 8d9bc3c..d93089d 100755\n> --- a/t/t3004-ls-files-basic.sh\n> +++ b/t/t3004-ls-files-basic.sh\n> @@ -36,4 +36,21 @@ test_expect_success 'ls-files -h in corrupt repository' '\n> \ttest_i18ngrep \"[Uu]sage: git ls-files \" broken/usage\n> '\n> \n> +test_expect_success SYMLINKS 'ls-files with symlinks' '\n> +\tmkdir subs &&\n> +\tln -s nosuch link &&\n> +\tln -s ../nosuch subs/link &&\n> +\tgit add link subs/link &&\n> +\tgit ls-files -s link subs/link >expect &&\n> +\tgit ls-files -s \"$(pwd)/link\" \"$(pwd)/subs/link\" >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\t(\n> +\t\tcd subs &&\n> +\t\tgit ls-files -s link >../expect &&\n> +\t\tgit ls-files -s \"$(pwd)/link\" >../actual\n> +\t) &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n\nYour test looks preferrable to the simple git-add one that I proposed,\nsince it covers some more ground than the simple:\n\ntest_expect_success SYMLINKS 'add with abs path to link...' '\n\tln -s target link &&\n\tgit add link\n'\n\nWill you add that test or should I place it in the series with you as\nauthor?\n\nOn Mon, Feb 03, 2014 at 01:00:48PM -0800, Junio C Hamano wrote:\n> Martin Erik Werner writes:\n> \n> > The path being exactly equal to the work tree is handled separately,\n> > since then there is no directory separator between the work tree and\n> > in-repo part.\n> \n> What is an \"in-repo part\"? Whatever it is, I am not sure if I\n> follow that logic. After the while (*path) loop checks each level,\n> you check the whole path---wouldn't that code handle that special\n> case already?\n\nGiven \"/dir1/repo/dir2/file\" I've used 'in-repo' to refer to\n\"dir2/file\", sometimes interchangably with \"part inside the work tree\"\nwhich might be better terminology, and should replace it?\n\nI was trying to convey that if path is simply \"/dir/repo\", then the while\nloop method of replacing a '/' and checking from the beginning won't\nwork for the last level, since it has no terminating '/' to replace, so\nhence it's a special case, mentioning the \"part inside the work tree\"\nis arguably confusing in that case, since there isn't really one, maybe\nit should be left out completely, since the \"check each level\"\nexplanation covers it already?\n\n> \n> Because it is probably the normal case not to have any symbolic link\n> in the leading part (hence not having to dereference them), I think\n> checking \"is work_tree[] a prefix of path[]\" early is justified from\n> performance point of view, though.\n> \n> > /*\n> > + * No checking if the path is in fact an absolute path is done, and it must\n> > + * already be normalized.\n> \n> This is not quite parsable to me.\nHmm, what about\n\tThe input parameter must contain an absolute path, and it must\n\talready be normalized.\n?\n\n> > + * Find the part of an absolute path that lies inside the work tree by\n> > + * dereferencing symlinks outside the work tree, for example:\n> > + * /dir1/repo/dir2/file (work tree is /dir1/repo) -> dir2/file\n> > + * /dir/file (work tree is /) -> dir/file\n> > + * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2\n> > + * /dir/repolink/file (repolink points to /dir/repo) -> file\n> > + * /dir/repo (exactly equal to work tree) -> (empty string)\n> > + */\n> > +static inline int abspath_part_inside_repo(char *path)\n> \n> It looks a bit too big to be marked \"inline\"; better leave it to the\n> compiler to notice that there is only a single callsite and decide\n> to (or not to) inline it.\n\nOk, will do.\n\n> > +{\n> > +\tsize_t len;\n> > +\tsize_t wtlen;\n> > +\tchar *path0;\n> > +\tint off;\n> > +\n> > +\tconst char* work_tree = get_git_work_tree();\n> > +\tif (!work_tree)\n> > +\t\treturn -1;\n> > +\twtlen = strlen(work_tree);\n> > +\tlen = strlen(path);\n> \n> I expect that this is called from a callsite that _knows_ how long\n> path[] is. Shouldn't len be a parameter to this function instead?\n\nCurrently, it actually doesn't, since 'normalize_path_copy_len' is\ncalled on it prior, which can mess with the string length. Do you think\nthe 'strlen' call should still be moved up a step into\n'prefix_path_gently'?\n\n> > +\toff = 0;\n> > +\n> > +\t/* check if work tree is already the prefix */\n> > +\tif (strncmp(path, work_tree, wtlen) == 0) {\n> \n> I wonder if we want to be explicit and compare wtlen and len before\n> even attempting strncmp(). If work_tree is longer than path, it\n> cannot be a prefix of path, right?\n> \n> In other words, also with a style-fix to prefer !XXcmp() over\n> XXcmp() == 0:\n> \n> \tif (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n> \n> perhaps? That would make it clear why referring to path[wtlen] on\n> the next line is permissible (it is obvious that it won't access\n> past the end of path[]):\n\nDefinitely sounds like a good idea.\n\n> > +\t\tif (path[wtlen] == '/') {\n> > +\t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n> > +\t\t\treturn 0;\n> > +\t\t} else if (path[wtlen - 1] == '/' || path[wtlen] == '\\0') {\n> > +\t\t\t/* work tree is the root, or the whole path */\n> > +\t\t\tmemmove(path, path + wtlen, len - wtlen + 1);\n> \n> If work_tree[] == \"/\", path[] == \"/a\", then len == 2, wtlen == 1,\n> path[wtlen - 1] == '/' and this shifts path[] to the left by one,\n> leaving path[] = \"a\", which is what we want. OK.\n> \n> If work_tree[] == path[] == \"/a\", then len == wtlen == 2,\n> path[wtlen] == '\\0', and this becomes equivalent to path[0] = '\\0'.\n> Hmph.... How do our callers treat an empty path? In other words,\n> should these three be equivalent?\n> \n> \tcd /a && git ls-files /a\n> \tcd /a && git ls-files \"\"\n> \tcd /a && git ls-files .\n\nHere I have only gone by the assumption that previous code did the right\nthing, whether or not it *did*, I'm not sure I'm able to figure out at\nthe moment.\n\n> > +\t\t\treturn 0;\n> > +\t\t}\n> > +\t\t/* work tree might match beginning of a symlink to work tree */\n> > +\t\toff = wtlen;\n> > +\t}\n> > +\tpath0 = path;\n> > +\tpath += offset_1st_component(path) + off;\n> > +\n> > +\t/* check each level */\n> > +\twhile (*path) {\n> > +\t\tpath++;\n> > +\t\tif (*path == '/') {\n> > +\t\t\t*path = '\\0';\n> > +\t\t\tif (strcmp(real_path(path0), work_tree) == 0) {\n> > +\t\t\t\tmemmove(path0, path + 1, len - (path - path0));\n> > +\t\t\t\treturn 0;\n> > +\t\t\t}\n> > +\t\t\t*path = '/';\n> > +\t\t}\n> > +\t}\n> > +\n> > +\t/* check whole path */\n> > +\tif (strcmp(real_path(path0), work_tree) == 0) {\n> > +\t\t*path0 = '\\0';\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\treturn -1;\n> > +}\n> > +\n> > +/*\n> > * Normalize \"path\", prepending the \"prefix\" for relative paths. If\n> > * remaining_prefix is not NULL, return the actual prefix still\n> > * remains in the path. For example, prefix = sub1/sub2/ and path is\n"},{"id":"234177","messageId":"xmqq1tzjbv4b.fsf@gitster.dls.corp.google.com","threadId":"35664","inReplyTo":"20140203131700.GA15607@mule","subject":"Re: [PATCH v5 5/5] setup: Don't dereference in-tree symlinks for absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-04T00:05:08Z","receivedAt":"2014-02-04T00:05:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Erik Werner writes:\n\n> Then it seems like one could get rid of npath completely:\n\nYes. And you need to remove its definition as well to avoid \"unused\nvariable\" warning.\n\nWill queue with an obvious fix-up.\n\nThanks.\n\n>\n> diff --git a/setup.c b/setup.c\n> index 230505c..dd120cd 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -88,21 +88,17 @@ char *prefix_path_gently(const char *prefix, int len,\n> \tif (is_absolute_path(orig)) {\n> \t\tchar *npath;\n> \n> -\t\tnpath = xmalloc(strlen(path) + 1);\n> +\t\tsanitized = xmalloc(strlen(path) + 1);\n> \t\tif (remaining_prefix)\n> \t\t\t*remaining_prefix = 0;\n> -\t\tif (normalize_path_copy_len(npath, path, remaining_prefix)) {\n> -\t\t\tfree(npath);\n> +\t\tif (normalize_path_copy_len(sanitized, path, remaining_prefix)) {\n> +\t\t\tfree(sanitized);\n> \t\t\treturn NULL;\n> \t\t}\n> -\t\tif (abspath_part_inside_repo(npath)) {\n> -\t\t\tfree(npath);\n> +\t\tif (abspath_part_inside_repo(sanitized)) {\n> +\t\t\tfree(sanitized);\n> \t\t\treturn NULL;\n> \t\t}\n> -\n> -\t\tsanitized = xmalloc(strlen(npath) + 1);\n> -\t\tstrcpy(sanitized, npath);\n> -\t\tfree(npath);\n> \t} else {\n> \t\tsanitized = xmalloc(len + strlen(path) + 1);\n> \t\tif (len)\n>\n> at the cost of 'sanitized' always being the length of path, regardless\n> if it's shorter, or even a NUL string.\n>\n> --\n> Martin Erik Werner \n"},{"id":"234200","messageId":"1391523920-26946-1-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391358940-17373-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v6 0/6] Handling of in-tree symlinks for absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-04T14:25:14Z","receivedAt":"2014-02-04T14:25:14Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"The amount of tweaks seemed deserving of a reroll, so here it is:\n\n* Added your test Junio, with what I figured was an appropriate commit\n message describing the user-visible effect (to git-add since I think it's the\n simplest to explain), the commit message for the second commit has been\n reduced somewhat, to not duplicate the message completely.\n\n* Duplicated quite a bit of the explanation from this first commit into\n the last commit, in order to be more obvious about the issue it fixes, I'm\n not sure if it's a bit too much?\n\n* Reworded the last commit to not mention the full-path special case, and\n replaced all occurences of in-repo with \"inside work tree\" or similar.\n\n* Content-wise compared to 'pu' I've tweaked a few comments, un-inlined and\n added the wtlen <= len check (and the ls-files test is new of course):\n\ndiff --git a/setup.c b/setup.c\nindex 32a6f6b..ba08885 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -6,8 +6,8 @@ static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n \n /*\n- * No checking if the path is in fact an absolute path is done, and it must\n- * already be normalized.\n+ * The input parameter must contain an absolute path, and it must already be\n+ * normalized.\n *\n * Find the part of an absolute path that lies inside the work tree by\n * dereferencing symlinks outside the work tree, for example:\n@@ -17,7 +17,7 @@ static int inside_work_tree = -1;\n * /dir/repolink/file (repolink points to /dir/repo) -> file\n * /dir/repo (exactly equal to work tree) -> (empty string)\n */\n-static inline int abspath_part_inside_repo(char *path)\n+static int abspath_part_inside_repo(char *path)\n {\n \tsize_t len;\n \tsize_t wtlen;\n@@ -32,7 +32,7 @@ static inline int abspath_part_inside_repo(char *path)\n \toff = 0;\n \n \t/* check if work tree is already the prefix */\n-\tif (strncmp(path, work_tree, wtlen) == 0) {\n+\tif (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n \t\tif (path[wtlen] == '/') {\n \t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n \t\t\treturn 0;\n@@ -47,7 +47,7 @@ static inline int abspath_part_inside_repo(char *path)\n \tpath0 = path;\n \tpath += offset_1st_component(path) + off;\n \n-\t/* check each level */\n+\t/* check each '/'-terminated level */\n \twhile (*path) {\n \t\tpath++;\n \t\tif (*path == '/') {\n\n\nJunio C Hamano (1):\n t3004: Add test for ls-files on symlinks via absolute paths\n\nMartin Erik Werner (5):\n t0060: Add test for prefix_path on symlinks via absolute paths\n t0060: Add test for prefix_path when path == work tree\n t0060: Add tests for prefix_path when path begins with work tree\n setup: Add 'abspath_part_inside_repo' function\n setup: Don't dereference in-tree symlinks for absolute paths\n\n setup.c | 94 +++++++++++++++++++++++++++++++++++++----------\n t/t0060-path-utils.sh | 21 +++++++++++\n t/t3004-ls-files-basic.sh | 17 +++++++++\n 3 files changed, 112 insertions(+), 20 deletions(-)\n\n-- \n1.8.5.2\n"},{"id":"234198","messageId":"1391523920-26946-2-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391523920-26946-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v6 1/6] t3004: Add test for ls-files on symlinks via absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-04T14:25:15Z","receivedAt":"2014-02-04T14:25:15Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"From: Junio C Hamano \n\nWhen symlinks in the working tree are manipulated using the absolute\npath, git dereferences them, and tries to manipulate the link target\ninstead.\n\nThis causes most high-level functions to misbehave when acting on\nsymlinks given via absolute paths. For example\n\n $ git add /dir/repo/symlink\n\nattempts to add the target of the symlink rather than the symlink\nitself, which is usually not what the user intends to do.\n\nThis is a regression introduced by 18e051a:\n setup: translate symlinks in filename when using absolute paths\n(which did not take symlinks inside the work tree into consideration).\n\nAdd a known-breakage test using the ls-files function, checking both if\nthe symlink leads to a target in the same directory, and a target in the\nabove directory.\n\nSigned-off-by: Martin Erik Werner \nTested-by: Martin Erik Werner \n---\n t/t3004-ls-files-basic.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh\nindex 8d9bc3c..e20c077 100755\n--- a/t/t3004-ls-files-basic.sh\n+++ b/t/t3004-ls-files-basic.sh\n@@ -36,4 +36,21 @@ test_expect_success 'ls-files -h in corrupt repository' '\n \ttest_i18ngrep \"[Uu]sage: git ls-files \" broken/usage\n '\n \n+test_expect_failure SYMLINKS 'ls-files with absolute paths to symlinks' '\n+\tmkdir subs &&\n+\tln -s nosuch link &&\n+\tln -s ../nosuch subs/link &&\n+\tgit add link subs/link &&\n+\tgit ls-files -s link subs/link >expect &&\n+\tgit ls-files -s \"$(pwd)/link\" \"$(pwd)/subs/link\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t(\n+\t\tcd subs &&\n+\t\tgit ls-files -s link >../expect &&\n+\t\tgit ls-files -s \"$(pwd)/link\" >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.5.2\n"},{"id":"234201","messageId":"1391523920-26946-3-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391523920-26946-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v6 2/6] t0060: Add test for prefix_path on symlinks via absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-04T14:25:16Z","receivedAt":"2014-02-04T14:25:16Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"When symlinks in the working tree are manipulated using the absolute\npath, git dereferences them, and tries to manipulate the link target\ninstead.\n\nThis applies to most high-level commands but prefix_path is the common\ndenominator for all of them.\n\nAdd a known-breakage tests using the prefix_path function, which\ncurrently uses real_path, causing the dereference.\n\nSigned-off-by: Martin Erik Werner \nReviewed-by: Duy Nguyen \n---\n t/t0060-path-utils.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 07c10c8..0bba988 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -190,6 +190,11 @@ test_expect_success SYMLINKS 'real path works on symlinks' '\n \ttest \"$sym\" = \"$(test-path-utils real_path \"$dir2/syml\")\"\n '\n \n+test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' '\n+\tln -s target symlink &&\n+\ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n+'\n+\n relative_path /foo/a/b/c/\t/foo/a/b/\tc/\n relative_path /foo/a/b/c/\t/foo/a/b\tc/\n relative_path /foo/a//b//c/\t///foo/a/b//\tc/\t\tPOSIX\n-- \n1.8.5.2\n"},{"id":"234203","messageId":"1391523920-26946-4-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391523920-26946-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v6 3/6] t0060: Add test for prefix_path when path == work tree","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-04T14:25:17Z","receivedAt":"2014-02-04T14:25:17Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"The current behaviour of prefix_path is to return an empty string if\nprefixing and absolute path that only contains exactly the work tree.\nThis behaviour is a potential regression point.\n\nSigned-off-by: Martin Erik Werner \nReviewed-by: Duy Nguyen \n---\n t/t0060-path-utils.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 0bba988..b8e92e1 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree\n \ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n '\n \n+test_expect_success 'prefix_path works with only absolute path to work tree' '\n+\techo \"\" >expected &&\n+\ttest-path-utils prefix_path prefix \"$(pwd)\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n relative_path /foo/a/b/c/\t/foo/a/b/\tc/\n relative_path /foo/a/b/c/\t/foo/a/b\tc/\n relative_path /foo/a//b//c/\t///foo/a/b//\tc/\t\tPOSIX\n-- \n1.8.5.2\n"},{"id":"234204","messageId":"1391523920-26946-5-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391523920-26946-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v6 4/6] t0060: Add tests for prefix_path when path begins with work tree","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-04T14:25:18Z","receivedAt":"2014-02-04T14:25:18Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"One edge-case that isn't currently checked in the tests is the beginning\nof the path matching the work tree, despite the target not actually\nbeing the work tree, for example:\n\n path = /dir/repoa\n work_tree = /dir/repo\n\nshould fail since the path is outside the repo. However, if /dir/repoa\nis in fact a symlink that points to /dir/repo, it should instead\nsucceed.\n\nAdd two tests covering these cases, since they might be potential\nregression points.\n\nSigned-off-by: Martin Erik Werner \nReviewed-by: Duy Nguyen \n---\n t/t0060-path-utils.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex b8e92e1..c0a14f6 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'prefix_path rejects absolute path to dir with same beginning as work tree' '\n+\ttest_must_fail test-path-utils prefix_path prefix \"$(pwd)a\"\n+'\n+\n+test_expect_success 'prefix_path works with absolute path to a symlink to work tree having same beginning as work tree' '\n+\tgit init repo &&\n+\tln -s repo repolink &&\n+\ttest \"a\" = \"$(cd repo && test-path-utils prefix_path prefix \"$(pwd)/../repolink/a\")\"\n+'\n+\n relative_path /foo/a/b/c/\t/foo/a/b/\tc/\n relative_path /foo/a/b/c/\t/foo/a/b\tc/\n relative_path /foo/a//b//c/\t///foo/a/b//\tc/\t\tPOSIX\n-- \n1.8.5.2\n"},{"id":"234199","messageId":"1391523920-26946-6-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391523920-26946-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v6 5/6] setup: Add 'abspath_part_inside_repo' function","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-04T14:25:19Z","receivedAt":"2014-02-04T14:25:19Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"In order to extract the part of an absolute path which lies inside the\nrepo, it is not possible to directly use real_path, since that would\ndereference symlinks both outside and inside the work tree.\n\nAdd an 'abspath_part_inside_repo' function which first checks if the\nwork tree is already the prefix, then incrementally checks each path\nlevel by temporarily NUL-terminating at each '/' and comparing against\nthe work tree path. If a match is found, it overwrites the input path\nwith the remainder past the work tree (which will be the part inside the\nwork tree).\n\nThis function is currently only intended for use in\n'prefix_path_gently'.\n\nSigned-off-by: Martin Erik Werner \nReviewed-by: Duy Nguyen \n---\n setup.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 64 insertions(+)\n\ndiff --git a/setup.c b/setup.c\nindex 5432a31..906c8e2 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -6,6 +6,70 @@ static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n \n /*\n+ * The input parameter must contain an absolute path, and it must already be\n+ * normalized.\n+ *\n+ * Find the part of an absolute path that lies inside the work tree by\n+ * dereferencing symlinks outside the work tree, for example:\n+ * /dir1/repo/dir2/file (work tree is /dir1/repo) -> dir2/file\n+ * /dir/file (work tree is /) -> dir/file\n+ * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2\n+ * /dir/repolink/file (repolink points to /dir/repo) -> file\n+ * /dir/repo (exactly equal to work tree) -> (empty string)\n+ */\n+static int abspath_part_inside_repo(char *path)\n+{\n+\tsize_t len;\n+\tsize_t wtlen;\n+\tchar *path0;\n+\tint off;\n+\n+\tconst char* work_tree = get_git_work_tree();\n+\tif (!work_tree)\n+\t\treturn -1;\n+\twtlen = strlen(work_tree);\n+\tlen = strlen(path);\n+\toff = 0;\n+\n+\t/* check if work tree is already the prefix */\n+\tif (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n+\t\tif (path[wtlen] == '/') {\n+\t\t\tmemmove(path, path + wtlen + 1, len - wtlen);\n+\t\t\treturn 0;\n+\t\t} else if (path[wtlen - 1] == '/' || path[wtlen] == '\\0') {\n+\t\t\t/* work tree is the root, or the whole path */\n+\t\t\tmemmove(path, path + wtlen, len - wtlen + 1);\n+\t\t\treturn 0;\n+\t\t}\n+\t\t/* work tree might match beginning of a symlink to work tree */\n+\t\toff = wtlen;\n+\t}\n+\tpath0 = path;\n+\tpath += offset_1st_component(path) + off;\n+\n+\t/* check each '/'-terminated level */\n+\twhile (*path) {\n+\t\tpath++;\n+\t\tif (*path == '/') {\n+\t\t\t*path = '\\0';\n+\t\t\tif (strcmp(real_path(path0), work_tree) == 0) {\n+\t\t\t\tmemmove(path0, path + 1, len - (path - path0));\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t\t*path = '/';\n+\t\t}\n+\t}\n+\n+\t/* check whole path */\n+\tif (strcmp(real_path(path0), work_tree) == 0) {\n+\t\t*path0 = '\\0';\n+\t\treturn 0;\n+\t}\n+\n+\treturn -1;\n+}\n+\n+/*\n * Normalize \"path\", prepending the \"prefix\" for relative paths. If\n * remaining_prefix is not NULL, return the actual prefix still\n * remains in the path. For example, prefix = sub1/sub2/ and path is\n-- \n1.8.5.2\n"},{"id":"234202","messageId":"1391523920-26946-7-git-send-email-martinerikwerner@gmail.com","threadId":"35664","inReplyTo":"1391523920-26946-1-git-send-email-martinerikwerner@gmail.com","subject":"[PATCH v6 6/6] setup: Don't dereference in-tree symlinks for absolute paths","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-04T14:25:20Z","receivedAt":"2014-02-04T14:25:20Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"The 'prefix_path_gently' function currently applies real_path to\neverything if given an absolute path, dereferencing symlinks both\noutside and inside the work tree.\n\nThis causes most high-level functions to misbehave when acting on\nsymlinks given via absolute paths. For example\n\n\t$ git add /dir/repo/symlink\n\nattempts to add the target of the symlink rather than the symlink\nitself, which is usually not what the user intends to do.\n\nIn order to manipulate symlinks in the work tree using absolute paths,\nsymlinks should only be dereferenced outside the work tree.\n\nModify the 'prefix_path_gently' to first normalize the path in order to\nmake sure path levels are separated by '/', then pass the result to\n'abspath_part_inside_repo' to find the part inside the work tree\n(without dereferencing any symlinks inside the work tree).\n\nFor absolute paths, 'prefix_path_gently' did not, nor does now do, any\nactual prefixing, hence the result from 'abspath_part_in_repo' is\nreturned as-is.\n\nFixes t0060-82 and t3004-5.\n\nSigned-off-by: Martin Erik Werner \nReviewed-by: Duy Nguyen \n---\n setup.c | 30 ++++++++++--------------------\n t/t0060-path-utils.sh | 2 +-\n t/t3004-ls-files-basic.sh | 2 +-\n 3 files changed, 12 insertions(+), 22 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 906c8e2..ba08885 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -86,11 +86,17 @@ char *prefix_path_gently(const char *prefix, int len,\n \tconst char *orig = path;\n \tchar *sanitized;\n \tif (is_absolute_path(orig)) {\n-\t\tconst char *temp = real_path(path);\n-\t\tsanitized = xmalloc(len + strlen(temp) + 1);\n-\t\tstrcpy(sanitized, temp);\n+\t\tsanitized = xmalloc(strlen(path) + 1);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = 0;\n+\t\tif (normalize_path_copy_len(sanitized, path, remaining_prefix)) {\n+\t\t\tfree(sanitized);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tif (abspath_part_inside_repo(sanitized)) {\n+\t\t\tfree(sanitized);\n+\t\t\treturn NULL;\n+\t\t}\n \t} else {\n \t\tsanitized = xmalloc(len + strlen(path) + 1);\n \t\tif (len)\n@@ -98,26 +104,10 @@ char *prefix_path_gently(const char *prefix, int len,\n \t\tstrcpy(sanitized + len, path);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = len;\n-\t}\n-\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))\n-\t\tgoto error_out;\n-\tif (is_absolute_path(orig)) {\n-\t\tsize_t root_len, len, total;\n-\t\tconst char *work_tree = get_git_work_tree();\n-\t\tif (!work_tree)\n-\t\t\tgoto error_out;\n-\t\tlen = strlen(work_tree);\n-\t\troot_len = offset_1st_component(work_tree);\n-\t\ttotal = strlen(sanitized) + 1;\n-\t\tif (strncmp(sanitized, work_tree, len) ||\n-\t\t (len > root_len && sanitized[len] != '\\0' && sanitized[len] != '/')) {\n-\t\terror_out:\n+\t\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {\n \t\t\tfree(sanitized);\n \t\t\treturn NULL;\n \t\t}\n-\t\tif (sanitized[len] == '/')\n-\t\t\tlen++;\n-\t\tmemmove(sanitized, sanitized + len, total - len);\n \t}\n \treturn sanitized;\n }\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex c0a14f6..f04b82d 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '\n \ttest \"$sym\" = \"$(test-path-utils real_path \"$dir2/syml\")\"\n '\n \n-test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' '\n+test_expect_success SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' '\n \tln -s target symlink &&\n \ttest \"$(test-path-utils prefix_path prefix \"$(pwd)/symlink\")\" = \"symlink\"\n '\ndiff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh\nindex e20c077..9c7adbd 100755\n--- a/t/t3004-ls-files-basic.sh\n+++ b/t/t3004-ls-files-basic.sh\n@@ -36,7 +36,7 @@ test_expect_success 'ls-files -h in corrupt repository' '\n \ttest_i18ngrep \"[Uu]sage: git ls-files \" broken/usage\n '\n \n-test_expect_failure SYMLINKS 'ls-files with absolute paths to symlinks' '\n+test_expect_success SYMLINKS 'ls-files with absolute paths to symlinks' '\n \tmkdir subs &&\n \tln -s nosuch link &&\n \tln -s ../nosuch subs/link &&\n-- \n1.8.5.2\n"},{"id":"234208","messageId":"xmqqlhxqagwt.fsf@gitster.dls.corp.google.com","threadId":"35664","inReplyTo":"20140203231659.GC15607@mule","subject":"Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-04T18:09:38Z","receivedAt":"2014-02-04T18:09:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Erik Werner writes:\n\n> Will you add that test or should I place it in the series with you as\n> author?\n\nEither is fine. Thanks.\n\n> On Mon, Feb 03, 2014 at 01:00:48PM -0800, Junio C Hamano wrote:\n>> Martin Erik Werner writes:\n>> \n>> > The path being exactly equal to the work tree is handled separately,\n>> > since then there is no directory separator between the work tree and\n>> > in-repo part.\n>> \n>> What is an \"in-repo part\"? Whatever it is, I am not sure if I\n>> follow that logic. After the while (*path) loop checks each level,\n>> you check the whole path---wouldn't that code handle that special\n>> case already?\n>\n> Given \"/dir1/repo/dir2/file\" I've used 'in-repo' to refer to\n> \"dir2/file\", sometimes interchangably with \"part inside the work tree\"\n> which might be better terminology, and should replace it?\n\nYes, \"inside the working tree\" is much clearer than \"inside repo\",\nbecause the word \"repository\" often is used to mean only the stuff\ninside $GIT_DIR, i.e. what controls the working tree files.\n\n> I was trying to convey that if path is simply \"/dir/repo\", then the while\n> loop method of replacing a '/' and checking from the beginning won't\n> work for the last level, since it has no terminating '/' to replace, so\n> hence it's a special case, mentioning the \"part inside the work tree\"\n> is arguably confusing in that case, since there isn't really one, maybe\n> it should be left out completely, since the \"check each level\"\n> explanation covers it already?\n\nI dunno about the explanation, but it still looks strange to have\nthe special case to deal with \"/dir/repo\" before you enter the while\nloop, and then also have code immediately after the loop that seems\nto handle the same case. Isn't the latter one redundant?\n\n>> Because it is probably the normal case not to have any symbolic link\n>> in the leading part (hence not having to dereference them), I think\n>> checking \"is work_tree[] a prefix of path[]\" early is justified from\n>> performance point of view, though.\n>> \n>> > /*\n>> > + * No checking if the path is in fact an absolute path is done, and it must\n>> > + * already be normalized.\n>> \n>> This is not quite parsable to me.\n> Hmm, what about\n> \tThe input parameter must contain an absolute path, and it must\n> \talready be normalized.\n\nOK.\n\n>> > +\tconst char* work_tree = get_git_work_tree();\n>> > +\tif (!work_tree)\n>> > +\t\treturn -1;\n>> > +\twtlen = strlen(work_tree);\n>> > +\tlen = strlen(path);\n>> \n>> I expect that this is called from a callsite that _knows_ how long\n>> path[] is. Shouldn't len be a parameter to this function instead?\n>\n> Currently, it actually doesn't, since 'normalize_path_copy_len' is\n> called on it prior, which can mess with the string length.\n\nOK, strlen() here is perfectly fine, then.\n\n>> Hmph.... How do our callers treat an empty path? In other words,\n>> should these three be equivalent?\n>> \n>> \tcd /a && git ls-files /a\n>> \tcd /a && git ls-files \"\"\n>> \tcd /a && git ls-files .\n>\n> Here I have only gone by the assumption that previous code did the right\n> thing,...\n\nGood to know. And the answer to the above question I think is yes,\nthese should be equivalent.\n\nThanks.\n"},{"id":"234210","messageId":"1391538767.4905.30.camel@mule","threadId":"35664","inReplyTo":"xmqqlhxqagwt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-02-04T18:32:47Z","receivedAt":"2014-02-04T18:32:47Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"On Tue, 2014-02-04 at 10:09 -0800, Junio C Hamano wrote:\n> Martin Erik Werner writes:\n(...)\n> > I was trying to convey that if path is simply \"/dir/repo\", then the while\n> > loop method of replacing a '/' and checking from the beginning won't\n> > work for the last level, since it has no terminating '/' to replace, so\n> > hence it's a special case, mentioning the \"part inside the work tree\"\n> > is arguably confusing in that case, since there isn't really one, maybe\n> > it should be left out completely, since the \"check each level\"\n> > explanation covers it already?\n> \n> I dunno about the explanation, but it still looks strange to have\n> the special case to deal with \"/dir/repo\" before you enter the while\n> loop, and then also have code immediately after the loop that seems\n> to handle the same case. Isn't the latter one redundant?\n\nThe check before the loop doesn't use 'real_path', the one after does:\n\"/dir/repo\" vs \"/dir/repolink\"\n\n-- \nMartin Erik Werner \n"},{"id":"234216","messageId":"xmqqr47i8z5m.fsf@gitster.dls.corp.google.com","threadId":"35664","inReplyTo":"1391523920-26946-6-git-send-email-martinerikwerner@gmail.com","subject":"Re: [PATCH v6 5/6] setup: Add 'abspath_part_inside_repo' function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-04T19:18:29Z","receivedAt":"2014-02-04T19:18:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Erik Werner writes:\n\n> +\tconst char* work_tree = get_git_work_tree();\n\nStyle: asterisk sticks to the variable, not type.\n\nNo need to resend---all patches looked reasonable.\n\nThanks, will queue.\n"},{"id":"234218","messageId":"52F146E1.4000604@web.de","threadId":"35664","inReplyTo":"1391523920-26946-5-git-send-email-martinerikwerner@gmail.com","subject":"Re: [PATCH v6 4/6] t0060: Add tests for prefix_path when path begins with work tree","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-02-04T20:00:33Z","receivedAt":"2014-02-04T20:00:33Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-02-04 15.25, Martin Erik Werner wrote:\n\n> t/t0060-path-utils.sh | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n> \n> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\n> index b8e92e1..c0a14f6 100755\n> --- a/t/t0060-path-utils.sh\n> +++ b/t/t0060-path-utils.sh\n> @@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' '\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success 'prefix_path rejects absolute path to dir with same beginning as work tree' '\n> +\ttest_must_fail test-path-utils prefix_path prefix \"$(pwd)a\"\n> +'\n> +\n> +test_expect_success 'prefix_path works with absolute path to a symlink to work tree having same beginning as work tree' '\n> +\tgit init repo &&\n> +\tln -s repo repolink &&\n> +\ttest \"a\" = \"$(cd repo && test-path-utils prefix_path prefix \"$(pwd)/../repolink/a\")\"\n> +'\nI think we need SYMLINKS here.\n"},{"id":"234222","messageId":"xmqqmwi68ww4.fsf@gitster.dls.corp.google.com","threadId":"35664","inReplyTo":"52F146E1.4000604@web.de","subject":"Re: [PATCH v6 4/6] t0060: Add tests for prefix_path when path begins with work tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-04T20:07:23Z","receivedAt":"2014-02-04T20:07:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> On 2014-02-04 15.25, Martin Erik Werner wrote:\n>\n>> t/t0060-path-utils.sh | 10 ++++++++++\n>> 1 file changed, 10 insertions(+)\n>> \n>> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\n>> index b8e92e1..c0a14f6 100755\n>> --- a/t/t0060-path-utils.sh\n>> +++ b/t/t0060-path-utils.sh\n>> @@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' '\n>> \ttest_cmp expected actual\n>> '\n>> \n>> +test_expect_success 'prefix_path rejects absolute path to dir with same beginning as work tree' '\n>> +\ttest_must_fail test-path-utils prefix_path prefix \"$(pwd)a\"\n>> +'\n>> +\n>> +test_expect_success 'prefix_path works with absolute path to a symlink to work tree having same beginning as work tree' '\n>> +\tgit init repo &&\n>> +\tln -s repo repolink &&\n>> +\ttest \"a\" = \"$(cd repo && test-path-utils prefix_path prefix \"$(pwd)/../repolink/a\")\"\n>> +'\n> I think we need SYMLINKS here.\n\nYes.\n"}]}