{"thread":{"id":"28585","subject":"[PATCHv3 2/5] Learn to handle gitfiles in enter_repo","startedAt":"2011-10-05T13:31:26Z","lastAt":"2011-10-06T19:16:31Z","messageCount":3,"participants":["Phil Hord","Nguyen Thai Ngoc Duy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"176918","messageId":"4E8C5C2E.50309@cisco.com","threadId":"28585","inReplyTo":null,"subject":"[PATCHv3 2/5] Learn to handle gitfiles in enter_repo","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2011-10-05T13:31:26Z","receivedAt":"2011-10-05T13:31:26Z","isPatch":false,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"The enter_repo() function is used to navigate into a .git\ndirectory. It knows how to find standard alternatives (DWIM) but\nit doesn't handle gitfiles created by git init --separate-git-dir.\nThis means that git-fetch and others do not work with repositories\nusing the separate-git-dir mechanism.\n\nTeach enter_repo() to deal with the gitfile mechanism by resolving\nthe path to the redirected path and continuing tests on that path\ninstead of the found file.\n\nSigned-off-by: Phil Hord \n\n---\n\nNot sure how to resolve this for the 'strict' case.\n\nThe actual path followed may be different than the version spelled\nout on the input path because of resolved symlinks and whatnot.\nThis function normally returns the unmolested \"original\" path\nthat was validated. In case of a gitfile, I think that means\nwe should return the url resolved from the gitfile contents.\n\nBut should we?\n\nThe returned path is only used in git-daemon where it gets\nfurther checks for path restrictions.\n\nA. If we return the gitfile-resolved path, this may cause these\n path restrictions to fail since the path gets canonicalized\n when the gitfile is created by git.\n\nB. If we do not return the gitfile-resolved path, this can create\n a security-hole by allowing remote users to access files they\n would otherwise have been restricted from. In effect it creates\n an alternate symlink mechanism of which the administator might\n not be aware.\n\n\ndiff --git a/path.c b/path.c\nindex f3d96aa..46ba326 100644\n--- a/path.c\n+++ b/path.c\n@@ -295,6 +295,7 @@ const char *enter_repo(const char *path, int strict)\n \t\tstatic const char *suffix[] = {\n \t\t\t\".git/.git\", \"/.git\", \".git\", \"\", NULL,\n \t\t};\n+\t\tconst char *gitfile;\n \t\tint len = strlen(path);\n \t\tint i;\n \t\twhile ((1 < len) && (path[len-1] == '/'))\n@@ -330,7 +331,12 @@ const char *enter_repo(const char *path, int strict)\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n-\t\tif (!suffix[i] || chdir(used_path))\n+\t\tif (!suffix[i])\n+\t\t\treturn NULL;\n+\t\tgitfile = read_gitfile(used_path) ;\n+\t\tif (gitfile)\n+\t\t\tstrcpy(used_path, gitfile);\n+\t\tif (chdir(used_path))\n \t\t\treturn NULL;\n \t\tpath = validated_path;\n \t}\n-- \n1.7.7.505.ga09f6\n"},{"id":"176997","messageId":"CACsJy8BiZ7Ey95BOf4p-zwysyYwEY6WectRaj-GqnFvgDNTtZw@mail.gmail.com","threadId":"28585","inReplyTo":"4E8C5C2E.50309@cisco.com","subject":"Re: [PATCHv3 2/5] Learn to handle gitfiles in enter_repo","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-06T03:11:03Z","receivedAt":"2011-10-06T03:11:03Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 6, 2011 at 12:31 AM, Phil Hord wrote:\n> -               if (!suffix[i] || chdir(used_path))\n> +               if (!suffix[i])\n> +                       return NULL;\n> +               gitfile = read_gitfile(used_path) ;\n> +               if (gitfile)\n> +                       strcpy(used_path, gitfile);\n> +               if (chdir(used_path))\n>                        return NULL;\n>                path = validated_path;\n>        }\n\nThis is room for improvement, the patch is fine as it is now. We could\nimprove error reporting here. If .git file points to nowhere, we get\n\"not a repository-kind of message. Except daemon.c, enter_repo()\ncallers always die() if enter_repo() returns NULL. We could move the\ndie() part (with improved error message) into enter_repo().\n\nWe could update enter_repo(const char *, int) to enter_repo(const char\n*, int, int gently). If gently is 1, we never die() nor report\nanything (ie. what we're doing now). daemon.c will need this, the rest\nof callers will be happy with gently = 0.\n-- \nDuy\n"},{"id":"177096","messageId":"4E8DFE8F.3060408@cisco.com","threadId":"28585","inReplyTo":"CACsJy8BiZ7Ey95BOf4p-zwysyYwEY6WectRaj-GqnFvgDNTtZw@mail.gmail.com","subject":"Re: [PATCHv3 2/5] Learn to handle gitfiles in enter_repo","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2011-10-06T19:16:31Z","receivedAt":"2011-10-06T19:16:31Z","isPatch":false,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"Nguyen Thai Ngoc Duy says,\n> On Thu, Oct 6, 2011 at 12:31 AM, Phil Hord wrote:\n>> - if (!suffix[i] || chdir(used_path))\n>> + if (!suffix[i])\n>> + return NULL;\n>> + gitfile = read_gitfile(used_path) ;\n>> + if (gitfile)\n>> + strcpy(used_path, gitfile);\n>> + if (chdir(used_path))\n>> return NULL;\n>> path = validated_path;\n>> }\n> \n> This is room for improvement, the patch is fine as it is now. We could\n> improve error reporting here. If .git file points to nowhere, we get\n> \"not a repository-kind of message. Except daemon.c, enter_repo()\n> callers always die() if enter_repo() returns NULL. We could move the\n> die() part (with improved error message) into enter_repo().\n> \n> We could update enter_repo(const char *, int) to enter_repo(const char\n> *, int, int gently). If gently is 1, we never die() nor report\n> anything (ie. what we're doing now). daemon.c will need this, the rest\n> of callers will be happy with gently = 0.\n\nI like that. It wasn't clear to me what the 'gently' moniker meant\nbefore, but now I understand it. It could easily apply to this function\nand the new is_gitfile() to help reduce code duplication.\n\nIn a different patch, though.\n\nPhil\n"}]}