{"thread":{"id":"26143","subject":"[PATCH] setup: translate symlinks in filename when using absolute paths","startedAt":"2010-12-27T10:54:37Z","lastAt":"2010-12-29T13:44:11Z","messageCount":4,"participants":["Carlo Marcelo Arenas Belon","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"158617","messageId":"1293447277-30598-1-git-send-email-carenas@sajinet.com.pe","threadId":"26143","inReplyTo":null,"subject":"[PATCH] setup: translate symlinks in filename when using absolute paths","fromName":"Carlo Marcelo Arenas Belon","fromEmail":"carenas@sajinet.com.pe","sentAt":"2010-12-27T10:54:37Z","receivedAt":"2010-12-27T10:54:37Z","isPatch":true,"sender":{"key":"carenas@sajinet.com.pe","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"otherwise, comparison to validate against work tree will fail when\nthe path includes a symlink and the name passed is not canonical.\n\nSigned-off-by: Carlo Marcelo Arenas Belon \n---\n setup.c | 11 +++++++----\n 1 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 91887a4..e7c0d4d 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -7,10 +7,13 @@ static int inside_work_tree = -1;\n char *prefix_path(const char *prefix, int len, const char *path)\n {\n \tconst char *orig = path;\n-\tchar *sanitized = xmalloc(len + strlen(path) + 1);\n-\tif (is_absolute_path(orig))\n-\t\tstrcpy(sanitized, path);\n-\telse {\n+\tchar *sanitized;\n+\tif (is_absolute_path(orig)) {\n+\t\tconst char *temp = make_absolute_path(path);\n+\t\tsanitized = xmalloc(len + strlen(temp) + 1);\n+\t\tstrcpy(sanitized, temp);\n+\t} else {\n+\t\tsanitized = xmalloc(len + strlen(path) + 1);\n \t\tif (len)\n \t\t\tmemcpy(sanitized, prefix, len);\n \t\tstrcpy(sanitized + len, path);\n-- \n1.7.3.4.626.g73e7b.dirty\n"},{"id":"158686","messageId":"7vr5d1wuh0.fsf@alter.siamese.dyndns.org","threadId":"26143","inReplyTo":"1293447277-30598-1-git-send-email-carenas@sajinet.com.pe","subject":"Re: [PATCH] setup: translate symlinks in filename when using absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-28T19:47:07Z","receivedAt":"2010-12-28T19:47:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belon writes:\n\n> otherwise, comparison to validate against work tree will fail when\n> the path includes a symlink and the name passed is not canonical.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belon \n\nI take that \"path\" and \"name passed\" refer to the same thing (i.e. \"path\"\nparameter) in the above.\n\nI think you are trying to handle the case where:\n\n - you give \"/home/carenas/one\" from the command line;\n - $PWD is \"/home/carenas\"; and\n - \"/home/carenas\" is a symlink to \"/net/host/home/carenas\"\n\nand the scan-from-the-beginning-of-string check done between\n\"/home/carenas/one\" and the return value of get_git_work_tree() which\npresumably is \"/net/host/home/carenas\" disagrees. I wonder if a more\ncorrect solution might be to help get_git_work_tree() to match the notion\nof where the repository and its worktree are to the idea of where the user\nthinks they are, i.e. not \"/net/host/home/carenas\" but \"/home/carenas\", a\nbit better?\n\nThat would involve tweaking make_absolute_path() I guess?\n\nNote that your patch is the right thing to do either case, i.e. with or\nwithout such a change to make_absolute_path(), as the function is used to\nset up the return value from get_git_work_tree(). Anything we compare\nwith it should have passed make_absolute_path() at least once.\n\nNguyễn?\n\n> ---\n> setup.c | 11 +++++++----\n> 1 files changed, 7 insertions(+), 4 deletions(-)\n>\n> diff --git a/setup.c b/setup.c\n> index 91887a4..e7c0d4d 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -7,10 +7,13 @@ static int inside_work_tree = -1;\n> char *prefix_path(const char *prefix, int len, const char *path)\n> {\n> \tconst char *orig = path;\n> -\tchar *sanitized = xmalloc(len + strlen(path) + 1);\n> -\tif (is_absolute_path(orig))\n> -\t\tstrcpy(sanitized, path);\n> -\telse {\n> +\tchar *sanitized;\n> +\tif (is_absolute_path(orig)) {\n> +\t\tconst char *temp = make_absolute_path(path);\n> +\t\tsanitized = xmalloc(len + strlen(temp) + 1);\n> +\t\tstrcpy(sanitized, temp);\n> +\t} else {\n> +\t\tsanitized = xmalloc(len + strlen(path) + 1);\n> \t\tif (len)\n> \t\t\tmemcpy(sanitized, prefix, len);\n> \t\tstrcpy(sanitized + len, path);\n> -- \n> 1.7.3.4.626.g73e7b.dirty\n"},{"id":"158705","messageId":"20101229093512.GA22963@sajinet.com.pe","threadId":"26143","inReplyTo":"7vr5d1wuh0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] setup: translate symlinks in filename when using absolute paths","fromName":"Carlo Marcelo Arenas Belon","fromEmail":"carenas@sajinet.com.pe","sentAt":"2010-12-29T09:35:12Z","receivedAt":"2010-12-29T09:35:12Z","isPatch":true,"sender":{"key":"carenas@sajinet.com.pe","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Dec 28, 2010 at 11:47:07AM -0800, Junio C Hamano wrote:\n> Carlo Marcelo Arenas Belon writes:\n> \n> > otherwise, comparison to validate against work tree will fail when\n> > the path includes a symlink and the name passed is not canonical.\n> >\n> > Signed-off-by: Carlo Marcelo Arenas Belon \n> \n> I take that \"path\" and \"name passed\" refer to the same thing (i.e. \"path\"\n> parameter) in the above.\n\nyes, and sorry for the cryptic description; you detailed below though this\nis triggered by the fact that when using an absolute path filename as a\nparameter detection for worktree is failing because it was normalized\nthrough make_absolute_path.\n\n> I think you are trying to handle the case where:\n> \n> - you give \"/home/carenas/one\" from the command line;\n> - $PWD is \"/home/carenas\"; and\n> - \"/home/carenas\" is a symlink to \"/net/host/home/carenas\"\n\nthis will be a valid scenario, but the issue (with a different use case)\nwas reported in (which I missed to refer to when running git send-email):\n\n http://thread.gmane.org/gmane.comp.version-control.git/164212\n\n> and the scan-from-the-beginning-of-string check done between\n> \"/home/carenas/one\" and the return value of get_git_work_tree() which\n> presumably is \"/net/host/home/carenas\" disagrees. I wonder if a more\n> correct solution might be to help get_git_work_tree() to match the notion\n> of where the repository and its worktree are to the idea of where the user\n> thinks they are, i.e. not \"/net/host/home/carenas\" but \"/home/carenas\", a\n> bit better?\n\nare you suggesting symlinks would be left untouched at least during\nresolution for work_dir?, why is even necesary to resolve the links for\nother users of that function?\n\nCarlo\n"},{"id":"158707","messageId":"AANLkTi=VQZH01i9bpem7AhSaNv3BMr3__rLKsQ3bO1Rz@mail.gmail.com","threadId":"26143","inReplyTo":"7vr5d1wuh0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] setup: translate symlinks in filename when using absolute paths","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-12-29T13:44:11Z","receivedAt":"2010-12-29T13:44:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2010/12/29 Junio C Hamano :\n> Carlo Marcelo Arenas Belon writes:\n>\n>> otherwise, comparison to validate against work tree will fail when\n>> the path includes a symlink and the name passed is not canonical.\n>>\n>> Signed-off-by: Carlo Marcelo Arenas Belon \n>\n> I take that \"path\" and \"name passed\" refer to the same thing (i.e. \"path\"\n> parameter) in the above.\n>\n> I think you are trying to handle the case where:\n>\n>  - you give \"/home/carenas/one\" from the command line;\n>  - $PWD is \"/home/carenas\"; and\n>  - \"/home/carenas\" is a symlink to \"/net/host/home/carenas\"\n>\n> and the scan-from-the-beginning-of-string check done between\n> \"/home/carenas/one\" and the return value of get_git_work_tree() which\n> presumably is \"/net/host/home/carenas\" disagrees.  I wonder if a more\n> correct solution might be to help get_git_work_tree() to match the notion\n> of where the repository and its worktree are to the idea of where the user\n> thinks they are, i.e. not \"/net/host/home/carenas\" but \"/home/carenas\", a\n> bit better?\n\nI tend to agree. Will cause less surprises (such as this one).\n\n> That would involve tweaking make_absolute_path() I guess?\n\nHm.. can we avoid converting work_tree to absolute path unless people\nexplicitly set it (via --work-tree and GIT_WORK_TREE)? Basically\nworktree will be relative to cwd. Usually it's just \".\". When people\nrun commands outside worktree, it's the relative \"cwd/to/worktree\".\nI'm wondering if we can just avoid the use of make_absolute_path()\ncompletely in get_git_work_tree()..\n\n> Note that your patch is the right thing to do either case, i.e. with or\n> without such a change to make_absolute_path(), as the function is used to\n> set up the return value from get_git_work_tree().  Anything we compare\n> with it should have passed make_absolute_path() at least once.\n\nYes, I think that should that be done inside normalize_path_copy(),\nnot prefix_path().\n\n>>  setup.c |   11 +++++++----\n>>  1 files changed, 7 insertions(+), 4 deletions(-)\n\nAlso Carlo, tests should be good for illustration and regression\npurposes. I know you described in detail in another mail. But mails\ntend to get lost.\n-- \nDuy\n"}]}