{"thread":{"id":"7849","subject":"[PATCH] Change git-rev-parse --show-cdup to output an absolute path","startedAt":"2007-04-25T23:28:29Z","lastAt":"2007-04-26T12:14:51Z","messageCount":6,"participants":["koreth@midwinter.com","Alex Riesen","Steven Grimm"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"40470","messageId":"20070425232829.GA15930@midwinter.com","threadId":"7849","inReplyTo":null,"subject":"[PATCH] Change git-rev-parse --show-cdup to output an absolute path","fromName":"","fromEmail":"koreth@midwinter.com","sentAt":"2007-04-25T23:28:29Z","receivedAt":"2007-04-25T23:28:29Z","isPatch":true,"sender":{"key":"koreth@midwinter.com","avatar":"https://gravatar.com/avatar/71b4d2e8b62f168bdc9e9205341159e3567003b4f9e2127c617c5fa0a1f5bad2?d=mp&s=160"},"body":"Add a test case for deep symlinking into a repo, which breaks with the\nold relative output of --show-cdup.\n\nSigned-off-by: Steven Grimm <koreth@midwinter.com>\n---\n\nI have a symlink pointing to a subdirectory of a repo of mine. If I cd\nto that symlink, some shell-script git commands don't work.  Shell scripts\nuse the PWD environment variable as the working directory, and the chain\nof \"../\"s from git-rev-parse --show-cdup just peels entries off that\nvariable. That's fine when PWD is a real path, but not so great when it's\na reference to a symlink from an outside directory.\n\nA typical failure case:\n\n$ git clone git://whatever.git foobar\n$ ln -s foobar/src/tools/misc/myapp myapp\n$ cd myapp\n$ git pull\nfatal: Not a git repository\n\nThis patch adds a test for the problem (fails in the current git build)\nand a proposed fix: making git-rev-parse --show-cdup output an absolute\npath. It's possible that'll break something somewhere, but all the tests\nin the test suite pass, so I think the breakage can't be *too* severe if\nthere's any at all.\n\n builtin-rev-parse.c       |   18 +++++++++---------\n t/t6102-rev-parse-cdup.sh |   24 ++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 9 deletions(-)\n create mode 100755 t/t6102-rev-parse-cdup.sh\n\ndiff --git a/builtin-rev-parse.c b/builtin-rev-parse.c\nindex 37addb2..99a95eb 100644\n--- a/builtin-rev-parse.c\n+++ b/builtin-rev-parse.c\n@@ -320,15 +320,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--show-cdup\")) {\n-\t\t\t\tconst char *pfx = prefix;\n-\t\t\t\twhile (pfx) {\n-\t\t\t\t\tpfx = strchr(pfx, '/');\n-\t\t\t\t\tif (pfx) {\n-\t\t\t\t\t\tpfx++;\n-\t\t\t\t\t\tprintf(\"../\");\n-\t\t\t\t\t}\n-\t\t\t\t}\n-\t\t\t\tputchar('\\n');\n+\t\t\t\tchar cwd[PATH_MAX];\n+\t\t\t\t/*\n+\t\t\t\t * The prefix computation does chdir(\"..\") until it\n+\t\t\t\t * reaches the top level, so the current directory\n+\t\t\t\t * is actually the thing we want to output.\n+\t\t\t\t */\n+\t\t\t\tif (!getcwd(cwd, PATH_MAX))\n+\t\t\t\t\tdie(\"unable to get current working directory\");\n+\t\t\t\tprintf(\"%s/\\n\", cwd);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--git-dir\")) {\ndiff --git a/t/t6102-rev-parse-cdup.sh b/t/t6102-rev-parse-cdup.sh\nnew file mode 100755\nindex 0000000..233d9cf\n--- /dev/null\n+++ b/t/t6102-rev-parse-cdup.sh\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2007 Steven Grimm\n+#\n+\n+test_description='Test git-rev-parse --show-cdup'\n+\n+. ./test-lib.sh\n+. ../t6000lib.sh # t6xxx specific functions\n+\n+# Make a subdirectory and a symbolic link pointing to it from outside the repo.\n+mkdir subdir\n+ln -s `pwd`/subdir /tmp/git-test.$$\n+cd /tmp/git-test.$$\n+\n+test_expect_success 'shell pwd references symlink' \"pwd | grep git-test.$$\"\n+test_expect_success 'cdup goes to top level of repo' '\n+\tcd `git-rev-parse --show-cdup` &&\n+\ttest -d .git'\n+\n+rm /tmp/git-test.$$\n+\n+test_done\n+\n-- \n1.5.2.rc0.1.g2cc31\n"},{"id":"40496","messageId":"81b0412b0704260120mda8a2abhe343f5c127945939@mail.gmail.com","threadId":"7849","inReplyTo":"20070425232829.GA15930@midwinter.com","subject":"Re: [PATCH] Change git-rev-parse --show-cdup to output an absolute path","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-26T08:20:29Z","receivedAt":"2007-04-26T08:20:29Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 4/26/07, koreth@midwinter.com <koreth@midwinter.com> wrote:\n> I have a symlink pointing to a subdirectory of a repo of mine. If I cd\n> to that symlink, some shell-script git commands don't work.  Shell scripts\n> use the PWD environment variable as the working directory, and the chain\n> of \"../\"s from git-rev-parse --show-cdup just peels entries off that\n> variable. That's fine when PWD is a real path, but not so great when it's\n> a reference to a symlink from an outside directory.\n\nYour implementation will fail if cwd is longer than PATH_MAX.\nDoes not happen often, though.\n\n> A typical failure case:\n>\n> $ git clone git://whatever.git foobar\n> $ ln -s foobar/src/tools/misc/myapp myapp\n> $ cd myapp\n\nWhich is a strange thing to do. What is that for?\nmyapp is kind of outside the git repo foobar.\n"},{"id":"40500","messageId":"46306A29.4010608@midwinter.com","threadId":"7849","inReplyTo":"81b0412b0704260120mda8a2abhe343f5c127945939@mail.gmail.com","subject":"Re: [PATCH] Change git-rev-parse --show-cdup to output an absolute path","fromName":"Steven Grimm","fromEmail":"koreth@midwinter.com","sentAt":"2007-04-26T09:00:25Z","receivedAt":"2007-04-26T09:00:25Z","isPatch":true,"sender":{"key":"koreth@midwinter.com","avatar":"https://gravatar.com/avatar/71b4d2e8b62f168bdc9e9205341159e3567003b4f9e2127c617c5fa0a1f5bad2?d=mp&s=160"},"body":"Alex Riesen wrote:\n> Your implementation will fail if cwd is longer than PATH_MAX.\n> Does not happen often, though.\n\nThat limitation is already littered through the code, e.g. in setup.c \nwhich would already be failing in the existing implementation. Actually \nsetup.c goes one better: is_inside_git_dir() hardwires a 1024-character \nlimit into the code rather than using PATH_MAX. I didn't think I was \nintroducing a new limit here.\n\n>> A typical failure case:\n>>\n>> $ git clone git://whatever.git foobar\n>> $ ln -s foobar/src/tools/misc/myapp myapp\n>> $ cd myapp\n>\n> Which is a strange thing to do. What is that for?\n> myapp is kind of outside the git repo foobar.\n\nFor convenience, mostly; obviously that example was a bit contrived but \nI do have several symlinks to subdirectories of my repository and it's \nfaster to type \"cd ~/xyz\" than \"cd ~/repo/src/server/xyz\" all the time. \nAnd as you say, you're only \"kind of\" outside the repo when going \nthrough the symlink; one could argue that cd-ing into a symlink should \nbe the semantic equivalent of cd-ing into the thing the link points to, \nand that's certainly the way I use it.\n\nBut actually my big objection isn't that it fails, per se, but rather \nthat it fails inconsistently. All the C commands work just fine since \nthey do getcwd() which returns the real path. It's only the shell \nscripts that fail, e.g. git-pull. With the existing implementation I \nhave to remember which commands are shell scripts and which are C \nprograms, so I can do \"cd `/bin/pwd`\" to reset my $PWD before running \nany of the former.\n\nThat was actually my initial approach to fixing this -- I put \"cd \n`/bin/pwd`\" at the top of the \"cd_to_toplevel\" function in \ngit-sh-setup.sh. But it felt cleaner to make git-rev-parse return the \nactual correct path so it'd work for shell scripts that didn't happen to \nuse git-sh-setup.sh. I'm happy to go either way, or of course to keep \nthis as a local modification if folks find it too distasteful to include \nin the official source.\n\n-Steve\n"},{"id":"40503","messageId":"20070426091254.GA23586@midwinter.com","threadId":"7849","inReplyTo":"46306A29.4010608@midwinter.com","subject":"[PATCH] Use PATH_MAX rather than a hardwired constant maximum length.","fromName":"","fromEmail":"koreth@midwinter.com","sentAt":"2007-04-26T09:12:54Z","receivedAt":"2007-04-26T09:12:54Z","isPatch":true,"sender":{"key":"koreth@midwinter.com","avatar":"https://gravatar.com/avatar/71b4d2e8b62f168bdc9e9205341159e3567003b4f9e2127c617c5fa0a1f5bad2?d=mp&s=160"},"body":"Signed-off-by: Steven Grimm <koreth@midwinter.com>\n---\n\nIs the +1 really needed? The existing code is doing this in other places\nbut I'm not sure it's necessary since we're also doing sizeof(buffer)-1\nin the getcwd() call. I figured it was best to be consistent with the\nexisting code, e.g. setup_git_directory_gently().\n\n setup.c |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex a45ea83..84d3c4a 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -175,11 +175,11 @@ static int inside_git_dir = -1;\n int is_inside_git_dir(void)\n {\n \tif (inside_git_dir < 0) {\n-\t\tchar buffer[1024];\n+\t\tchar buffer[PATH_MAX+1];\n \n \t\tif (is_bare_repository())\n \t\t\treturn (inside_git_dir = 1);\n-\t\tif (getcwd(buffer, sizeof(buffer))) {\n+\t\tif (getcwd(buffer, sizeof(buffer)-1)) {\n \t\t\tconst char *git_dir = get_git_dir(), *cwd = buffer;\n \t\t\twhile (*git_dir && *git_dir == *cwd) {\n \t\t\t\tgit_dir++;\n-- \n1.5.2.rc0.35.gf41c8\n"},{"id":"40508","messageId":"81b0412b0704260457p2783ee9t6d08f0a9dca3b54e@mail.gmail.com","threadId":"7849","inReplyTo":"46306A29.4010608@midwinter.com","subject":"Re: [PATCH] Change git-rev-parse --show-cdup to output an absolute path","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-26T11:57:50Z","receivedAt":"2007-04-26T11:57:50Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 4/26/07, Steven Grimm <koreth@midwinter.com> wrote:\n> Alex Riesen wrote:\n> > Your implementation will fail if cwd is longer than PATH_MAX.\n> > Does not happen often, though.\n>\n> That limitation is already littered through the code, e.g. in setup.c\n> which would already be failing in the existing implementation. Actually\n> setup.c goes one better: is_inside_git_dir() hardwires a 1024-character\n> limit into the code rather than using PATH_MAX. I didn't think I was\n> introducing a new limit here.\n\nIt is yet another place to fix when it breaks.\n\n> >> A typical failure case:\n> >>\n> >> $ git clone git://whatever.git foobar\n> >> $ ln -s foobar/src/tools/misc/myapp myapp\n> >> $ cd myapp\n> >\n> > Which is a strange thing to do. What is that for?\n> > myapp is kind of outside the git repo foobar.\n>\n> For convenience, mostly; obviously that example was a bit contrived but\n> I do have several symlinks to subdirectories of my repository and it's\n> faster to type \"cd ~/xyz\" than \"cd ~/repo/src/server/xyz\" all the time.\n\nCan't you use your shells variables or aliases (which is even faster to\ntype)?\n\n> That was actually my initial approach to fixing this -- I put \"cd\n> `/bin/pwd`\" at the top of the \"cd_to_toplevel\" function in\n> git-sh-setup.sh. But it felt cleaner to make git-rev-parse return the\n> actual correct path so it'd work for shell scripts that didn't happen to\n> use git-sh-setup.sh. I'm happy to go either way, or of course to keep\n> this as a local modification if folks find it too distasteful to include\n> in the official source.\n\nI'm not really objecting. It's just a bit unusual so I suspect unusual\nproblems coming out of it. Kind of when you try running with your\neyes closed.\n"},{"id":"40510","messageId":"81b0412b0704260514k1eeec53ex97df3be600244305@mail.gmail.com","threadId":"7849","inReplyTo":"20070426091254.GA23586@midwinter.com","subject":"Re: [PATCH] Use PATH_MAX rather than a hardwired constant maximum length.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-26T12:14:51Z","receivedAt":"2007-04-26T12:14:51Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 4/26/07, koreth@midwinter.com <koreth@midwinter.com> wrote:\n> Signed-off-by: Steven Grimm <koreth@midwinter.com>\n> ---\n>\n> Is the +1 really needed? The existing code is doing this in other places\n\nNo, not according SUSv3, where PATH_MAX is defined as\n\"Maximum number of bytes in a pathname, including the terminating null\ncharacter.\"\nhttp://www.opengroup.org/onlinepubs/009695399/basedefs/limits.h.html#tag_13_24\n\ngetcwd expects the size of the buffer, so it can store the cwd _and_ NUL.\nSee ERANGE in in SUSv3:\nhttp://www.opengroup.org/onlinepubs/009695399/functions/getcwd.html\n\n> but I'm not sure it's necessary since we're also doing sizeof(buffer)-1\n\nThe -1 is wrong too.\n\n> in the getcwd() call. I figured it was best to be consistent with the\n> existing code, e.g. setup_git_directory_gently().\n\nsetup_git_directory_gently is wrong. But it does not really\nmatter in practice.\n"}]}