{"thread":{"id":"9472","subject":"[PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR","startedAt":"2007-08-10T14:02:25Z","lastAt":"2007-08-12T18:44:45Z","messageCount":15,"participants":["David Kastrup","Junio C Hamano","Steven Grimm"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"50412","messageId":"868x8j7aj2.fsf@lola.quinscape.zz","threadId":"9472","inReplyTo":null,"subject":"[PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-08-10T14:02:25Z","receivedAt":"2007-08-10T14:02:25Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"If filter-branch is entered with an unset GIT_DIR, things are rather\nfragile. The GIT_DIR variable setting then points to something like\n$(pwd)/../.. which is neither guaranteed to be a git directory\n(depends on where filter-branch is started), nor will it continue to\nwork once the temporary directory (for which the pwd is output) ceases\nto exist.\n\nSo we just call git-name-rev in order to get the correct setting here\nfor exporting.\n\nSigned-off-by: David Kastrup \n---\n git-filter-branch.sh | 12 ++++--------\n 1 files changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex b5fa449..6cd489e 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -170,14 +170,10 @@ do\n \tesac\n done < \"$tempdir\"/backup-refs\n \n-case \"$GIT_DIR\" in\n-/*)\n-\t;;\n-*)\n-\tGIT_DIR=\"$(pwd)/../../$GIT_DIR\"\n-\t;;\n-esac\n-export GIT_DIR GIT_WORK_TREE=.\n+GIT_DIR=$(cd ../..;git-name-rev --git-dir)\n+GIT_WORK_TREE=.\n+\n+export GIT_DIR GIT_WORK_TREE\n \n # These refs should be updated if their heads were rewritten\n \n-- \n1.5.3.rc4.43.gaf14\n"},{"id":"50422","messageId":"864pj74ay9.fsf@lola.quinscape.zz","threadId":"9472","inReplyTo":"868x8j7aj2.fsf@lola.quinscape.zz","subject":"[fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-08-10T16:21:34Z","receivedAt":"2007-08-10T16:21:34Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"If filter-branch is entered with an unset GIT_DIR, things are rather\nfragile. The GIT_DIR variable setting then points to something like\n$(pwd)/../.. which is neither guaranteed to be a git directory\n(depends on where filter-branch is started), nor will it continue to\nwork once the temporary directory (for which the pwd is output) ceases\nto exist.\n\nSo we just call git-rev-parse in order to get the correct setting here\nfor exporting.\n\nSigned-off-by: David Kastrup \n---\n git-filter-branch.sh | 13 +++++--------\n 1 files changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex b5fa449..9e9e8bf 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -170,14 +170,11 @@ do\n \tesac\n done < \"$tempdir\"/backup-refs\n \n-case \"$GIT_DIR\" in\n-/*)\n-\t;;\n-*)\n-\tGIT_DIR=\"$(pwd)/../../$GIT_DIR\"\n-\t;;\n-esac\n-export GIT_DIR GIT_WORK_TREE=.\n+GIT_DIR=$(cd ../..;cd \"./$(git-rev-parse --git-dir)\";pwd)\n+\n+GIT_WORK_TREE=.\n+\n+export GIT_DIR GIT_WORK_TREE\n \n # These refs should be updated if their heads were rewritten\n \n-- \n1.5.3.rc4.74.g3739cb17\n"},{"id":"50470","messageId":"7vk5s2mllf.fsf@assigned-by-dhcp.cox.net","threadId":"9472","inReplyTo":"864pj74ay9.fsf@lola.quinscape.zz","subject":"Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-11T10:06:36Z","receivedAt":"2007-08-11T10:06:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Kastrup writes:\n\n> If filter-branch is entered with an unset GIT_DIR, things are rather\n> fragile. The GIT_DIR variable setting then points to something like\n> $(pwd)/../.. which is neither guaranteed to be a git directory\n\nI think this comment refers to this part, ...\n\n> -case \"$GIT_DIR\" in\n> -/*)\n> -\t;;\n> -*)\n> -\tGIT_DIR=\"$(pwd)/../../$GIT_DIR\"\n> -\t;;\n> -esac\n\n... however, at the beginning of the script, it dot-includes\ngit-sh-setup, which sets (but not export) GIT_DIR for the rest\nof the script to use (see the last if..then..else).\n\nIf you got an unset GIT_DIR when you reached that case statement\nyou are removing here, I suspect that there is something else\ngoing on, but I do not see what it is...\n\nPuzzled...\n"},{"id":"50476","messageId":"85r6mawedp.fsf@lola.goethe.zz","threadId":"9472","inReplyTo":"7vk5s2mllf.fsf@assigned-by-dhcp.cox.net","subject":"Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-08-11T10:32:18Z","receivedAt":"2007-08-11T10:32:18Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Junio C Hamano writes:\n\n> David Kastrup writes:\n>\n>> If filter-branch is entered with an unset GIT_DIR, things are rather\n>> fragile. The GIT_DIR variable setting then points to something like\n>> $(pwd)/../.. which is neither guaranteed to be a git directory\n>\n> I think this comment refers to this part, ...\n>\n>> -case \"$GIT_DIR\" in\n>> -/*)\n>> -\t;;\n>> -*)\n>> -\tGIT_DIR=\"$(pwd)/../../$GIT_DIR\"\n>> -\t;;\n>> -esac\n>\n> ... however, at the beginning of the script, it dot-includes\n> git-sh-setup, which sets (but not export) GIT_DIR for the rest\n> of the script to use (see the last if..then..else).\n\nAh, ok. In that case my patch is overengineered. However, one could\nreplace the whole case with\n\nGITDIR=$(cd ../..;cd $GIT_DIR;pwd)\n\nThis would work with Windows absolute paths, too.\n\n> If you got an unset GIT_DIR when you reached that case statement\n> you are removing here, I suspect that there is something else\n> going on, but I do not see what it is...\n>\n> Puzzled...\n\nThe problem I saw was that \"$(pwd)/../../$GIT_DIR\" does no longer exist\nby the time the end of the script it reached, because what was\n\"$(pwd)\" has already been removed. So it is less onerous than I\nthought, but still a nuisance when the script ends.\n\nAnyway, if $GIT_DIR is a relative path, things would go wrong in some\nlines earlier, where the path is already being changed.\n\nIf git-sh-setup sets GIT_DIR, perhaps it would be sanest if it also\nmade it absolute?\n\nOtherwise any script that does \"cd\" will lose track of GIT_DIR, right?\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"},{"id":"50477","messageId":"858x8iw9q1.fsf@lola.goethe.zz","threadId":"9472","inReplyTo":"85r6mawedp.fsf@lola.goethe.zz","subject":"Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-08-11T12:12:54Z","receivedAt":"2007-08-11T12:12:54Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"David Kastrup writes:\n\n> Anyway, if $GIT_DIR is a relative path, things would go wrong in some\n> lines earlier,\n\nand if GIT_DIR were used or exported which it isn't, so we are lucky...\n\n> where the path is already being changed.\n>\n> If git-sh-setup sets GIT_DIR, perhaps it would be sanest if it also\n> made it absolute?\n>\n> Otherwise any script that does \"cd\" will lose track of GIT_DIR, right?\n\nActually, wouldn't by far the most straightforward thing be if\ngit-rev-parse --git-dir always returned an absolute path (even when\nbeing passed a relative path in GIT_DIR)?\n\nNo need for postprocessing, no need to keep track of changed\ndirectories.\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"},{"id":"50479","messageId":"851weaw8z6.fsf@lola.goethe.zz","threadId":"9472","inReplyTo":"858x8iw9q1.fsf@lola.goethe.zz","subject":"Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-08-11T12:29:01Z","receivedAt":"2007-08-11T12:29:01Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"David Kastrup writes:\n\n> David Kastrup writes:\n>\n>> Anyway, if $GIT_DIR is a relative path, things would go wrong in some\n>> lines earlier,\n>\n> and if GIT_DIR were used or exported which it isn't, so we are lucky...\n>\n>> where the path is already being changed.\n>>\n>> If git-sh-setup sets GIT_DIR, perhaps it would be sanest if it also\n>> made it absolute?\n>>\n>> Otherwise any script that does \"cd\" will lose track of GIT_DIR, right?\n>\n> Actually, wouldn't by far the most straightforward thing be if\n> git-rev-parse --git-dir always returned an absolute path (even when\n> being passed a relative path in GIT_DIR)?\n>\n> No need for postprocessing, no need to keep track of changed\n> directories.\n\nTo illustrate the potential for trouble:\n\ngit-commit.sh\n\nhas the lines\n\n647:\n cd_to_toplevel\n\n git rerere\n\n if test \"$ret\" = 0\n then\n if test -x \"$GIT_DIR\"/hooks/post-commit\n then\n \"$GIT_DIR\"/hooks/post-commit\n fi\n\nObviously, this won't work if GIT_DIR has a relative path and the\ncommand has not been started from the top level already.\n\nIn a similar vein, git-checkout.sh has\n\n263:\n GIT_DIR=\"$GIT_DIR\" git symbolic-ref -m \"checkout: moving from $old_branch_name to $branch\" HEAD \"refs/heads/$branch\"\n\nwhile we have previously cd_to_toplevel in line 154.\n\n\nAnd so on. Rather than fixing all those scripts, it would seem safer\nto let git-rev-parse --git-dir (I don't understand the code in\nbuiltin-rev-parse, by the way: why does it look at \"prefix\"?), or at\nthe very least git-sh-setup.sh return an absolute path.\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"},{"id":"50480","messageId":"46BDB0E7.3020506@midwinter.com","threadId":"9472","inReplyTo":"858x8iw9q1.fsf@lola.goethe.zz","subject":"Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR","fromName":"Steven Grimm","fromEmail":"koreth@midwinter.com","sentAt":"2007-08-11T12:51:51Z","receivedAt":"2007-08-11T12:51:51Z","isPatch":true,"sender":{"key":"koreth@midwinter.com","avatar":"https://gravatar.com/avatar/71b4d2e8b62f168bdc9e9205341159e3567003b4f9e2127c617c5fa0a1f5bad2?d=mp&s=160"},"body":"David Kastrup wrote:\n> Actually, wouldn't by far the most straightforward thing be if\n> git-rev-parse --git-dir always returned an absolute path (even when\n> being passed a relative path in GIT_DIR)?\n> \n\nThat might also help get rid of an annoying failure mode where some git \ncommands (the shell script ones) fail if you cd into a git repository \nvia a symlink, while others (the builtins) work just fine.\n\n-Steve\n"},{"id":"50495","messageId":"7vbqddn8zj.fsf@assigned-by-dhcp.cox.net","threadId":"9472","inReplyTo":"851weaw8z6.fsf@lola.goethe.zz","subject":"Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-11T19:53:36Z","receivedAt":"2007-08-11T19:53:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I think your other patch to always give full path to the shell\nscripts from git-sh-setup makes sense. Could you please make\ntest scripts to expose the problems you described in the message\nI am replying to and add it as say t/t2050-git-dir-relative.sh?\n"},{"id":"50499","messageId":"7vveblltmw.fsf@assigned-by-dhcp.cox.net","threadId":"9472","inReplyTo":"46BDB0E7.3020506@midwinter.com","subject":"Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-11T20:10:31Z","receivedAt":"2007-08-11T20:10:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steven Grimm writes:\n\n> David Kastrup wrote:\n>> Actually, wouldn't by far the most straightforward thing be if\n>> git-rev-parse --git-dir always returned an absolute path (even when\n>> being passed a relative path in GIT_DIR)?\n>>\n>\n> That might also help get rid of an annoying failure mode where some\n> git commands (the shell script ones) fail if you cd into a git\n> repository via a symlink, while others (the builtins) work just fine.\n\nIt might be the easiest workaround for this issue to unset PWD\nin git-sh-setup.sh (and git-clone.sh) where we unset CDPATH.\nAnybody wants to try to come up with additional test scripts to\nexpose the problem and a patch to these two files?\n"},{"id":"50504","messageId":"857io1u6ix.fsf@lola.goethe.zz","threadId":"9472","inReplyTo":"7vbqddn8zj.fsf@assigned-by-dhcp.cox.net","subject":"Re: [fixed PATCH] git-filter-branch.sh: Fix broken setting of GIT_DIR","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-08-11T21:04:54Z","receivedAt":"2007-08-11T21:04:54Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Junio C Hamano writes:\n\n> I think your other patch to always give full path to the shell\n> scripts from git-sh-setup makes sense. Could you please make\n> test scripts to expose the problems you described in the message\n> I am replying to and add it as say t/t2050-git-dir-relative.sh?\n\nIt is actually a bit harder to trigger than I expected: if GIT_DIR is\nnot a direct descendant of the current directory, git-rev-parse does\noutput an absolute path. So at least for the test failure in\ngit-commit, I have to revert to a non-run-of-the-mill setup.\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"},{"id":"50509","messageId":"85tzr5spaj.fsf@lola.goethe.zz","threadId":"9472","inReplyTo":"7vbqddn8zj.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] Add a test for git-commit being confused by relative GIT_DIR","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-08-11T22:02:28Z","receivedAt":"2007-08-11T22:02:28Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"\nSigned-off-by: David Kastrup \n---\nIt takes an inordinarily large amount of time to create these tests, I\nam afraid. There is little sense in making a more extensive audit,\ncranking out more cases which fail with relative paths, when the fix,\nnamely making GIT_DIR absolute, is so simple.\n\n t/t2050-git-dir-relative.sh | 54 +++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 54 insertions(+), 0 deletions(-)\n create mode 100755 t/t2050-git-dir-relative.sh\n\ndiff --git a/t/t2050-git-dir-relative.sh b/t/t2050-git-dir-relative.sh\nnew file mode 100755\nindex 0000000..54a3512\n--- /dev/null\n+++ b/t/t2050-git-dir-relative.sh\n@@ -0,0 +1,54 @@\n+#!/bin/sh\n+\n+test_description='check problems with relative GIT_DIR\n+\n+This test creates a working tree state with a file and subdir:\n+\n+ top (committed several times)\n+ subdir (a subdirectory)\n+\n+It creates a commit-hook and tests it, then moves .git\n+into the subdir while keeping the worktree location, \n+and tries commits from the top and the subdir, checking\n+that the commit-hook still gets called.'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'Setting up post-commit hook' '\n+mkdir -p .git/hooks &&\n+cat <.git/hooks/post-commit &&\n+#!/bin/sh\n+touch $(pwd)/output\n+echo \"Post commit hook was called.\"\n+EOF\n+chmod +x .git/hooks/post-commit'\n+\n+test_expect_success 'post-commit hook used ordinarily' '\n+echo initial >top &&\n+git-add top\n+git-commit -m initial &&\n+test -r output\n+'\n+\n+rm -rf output\n+mkdir subdir\n+mv .git subdir\n+\n+test_expect_success 'post-commit-hook created and used from top dir' '\n+echo changed >top &&\n+git --git-dir subdir/.git add top &&\n+git --git-dir subdir/.git commit -m topcommit &&\n+test -r output\n+'\n+\n+rm -rf output\n+\n+test_expect_success 'post-commit-hook from sub dir' '\n+echo changed again >top\n+cd subdir &&\n+git --git-dir .git --work-tree .. add ../top &&\n+git --git-dir .git --work-tree .. commit -m subcommit &&\n+test -r ../output\n+'\n+\n+test_done\n-- \n1.5.3.rc2.187.g9a1d2-dirty\n"},{"id":"50530","messageId":"7vvebljz31.fsf@assigned-by-dhcp.cox.net","threadId":"9472","inReplyTo":"85tzr5spaj.fsf@lola.goethe.zz","subject":"Re: [PATCH] Add a test for git-commit being confused by relative GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-12T01:55:46Z","receivedAt":"2007-08-12T01:55:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Kastrup writes:\n\n> +test_expect_success 'Setting up post-commit hook' '\n> +mkdir -p .git/hooks &&\n> +cat <.git/hooks/post-commit &&\n> +#!/bin/sh\n> +touch $(pwd)/output\n> +echo \"Post commit hook was called.\"\n> +EOF\n> +chmod +x .git/hooks/post-commit'\n\nWe have avoided to use here text inside test_expect_success, as\nthere have been reports that some otherwise reasonably usable\nshells do not grok it. Although I prefer to do everything,\nincluding the set-up part, inside test_expect_success, please\nmove this code outside.\n\nAlso I do not think you would want to say touch $(pwd)/output\nthere inside the here text that begins with < writes:\n\n> David Kastrup writes:\n>\n>> +test_expect_success 'Setting up post-commit hook' '\n>> +mkdir -p .git/hooks &&\n>> +cat <.git/hooks/post-commit &&\n>> +#!/bin/sh\n>> +touch $(pwd)/output\n>> +echo \"Post commit hook was called.\"\n>> +EOF\n>> +chmod +x .git/hooks/post-commit'\n>\n> We have avoided to use here text inside test_expect_success, as\n> there have been reports that some otherwise reasonably usable\n> shells do not grok it.\n\nOk.\n\n> Although I prefer to do everything, including the set-up part,\n> inside test_expect_success, please move this code outside.\n\nI can easily enough make it an echo.\n\n> Also I do not think you would want to say touch $(pwd)/output\n> there inside the here text that begins with < writes:\n\n>> Also I do not think you would want to say touch $(pwd)/output\n>> there inside the here text that begins with <\n> But I most certainly do!\n\nThen you would need to shell-quote $(pwd) so that you can have\nyour git.git checked out somewhere under \"/My Documents/source\"\ndirectory, wouldn't you?\n"},{"id":"50569","messageId":"85eji8oan6.fsf@lola.goethe.zz","threadId":"9472","inReplyTo":"7vir7kk7jg.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Add a test for git-commit being confused by relative GIT_DIR","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-08-12T18:44:45Z","receivedAt":"2007-08-12T18:44:45Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Junio C Hamano writes:\n\n> David Kastrup writes:\n>\n>>> Also I do not think you would want to say touch $(pwd)/output\n>>> there inside the here text that begins with <>\n>> But I most certainly do!\n>\n> Then you would need to shell-quote $(pwd) so that you can have\n> your git.git checked out somewhere under \"/My Documents/source\"\n> directory, wouldn't you?\n\nOk, that's a tough one.\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"}]}