{"thread":{"id":"24561","subject":"[PATCH] require_work_tree: Look for top-level instead of is-inside-work-tree","startedAt":"2010-07-28T16:47:04Z","lastAt":"2010-08-03T07:57:35Z","messageCount":6,"participants":["Tor Arne Vestbø","Junio C Hamano","Michael J Gruber"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"146637","messageId":"1280335624-90132-1-git-send-email-tor.arne.vestbo@nokia.com","threadId":"24561","inReplyTo":null,"subject":"[PATCH] require_work_tree: Look for top-level instead of is-inside-work-tree","fromName":"Tor Arne Vestbø","fromEmail":"tor.arne.vestbo@nokia.com","sentAt":"2010-07-28T16:47:04Z","receivedAt":"2010-07-28T16:47:04Z","isPatch":true,"sender":{"key":"tor.arne.vestbo@nokia.com","avatar":"https://gravatar.com/avatar/483668eb97b2c2076a95f0ceafe499a5167f313582bc174f1223c6b0ae7aa200?d=mp&s=160"},"body":"The documentation describes require_work_tree as guarding against\nbare repositories, and that's also the way it's used from porcelain\nsuch as git-rebase. When implemented using --is-inside-work-tree\nthe samantics change, causing git-rebase to fail if run from outside\nGIT_WORK_TREE, even if GIT_WORK_TREE is valid.\n\nSigned-off-by: Tor Arne Vestbø <tor.arne.vestbo@nokia.com>\n---\n git-sh-setup.sh     |    2 +-\n t/t1501-worktree.sh |    9 +++++++++\n 2 files changed, 10 insertions(+), 1 deletions(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 6131670..f8e4428 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -141,7 +141,7 @@ cd_to_toplevel () {\n }\n \n require_work_tree () {\n-\ttest \"$(git rev-parse --is-inside-work-tree 2>/dev/null)\" = true ||\n+\ttest -n \"$(git rev-parse --show-toplevel 2>/dev/null)\" ||\n \tdie \"fatal: $0 cannot be used without a working tree.\"\n }\n \ndiff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh\nindex bd8b607..45b09e7 100755\n--- a/t/t1501-worktree.sh\n+++ b/t/t1501-worktree.sh\n@@ -114,6 +114,15 @@ test_expect_success 'repo finds its work tree from work tree, too' '\n \t test sub/dir/tracked = \"$(git ls-files)\")\n '\n \n+test_expect_success 'require_work_tree finds work tree' '\n+\t(cd repo.git/work &&\n+\t. \"$(git --exec-path)\"/git-sh-setup &&\n+\tcd .. &&\n+\trequire_work_tree &&\n+\tcd .. &&\n+\trequire_work_tree)\n+'\n+\n test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' '\n \t(cd repo.git/work/sub/dir &&\n \tGIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \\\n-- \n1.7.2.19.g48995\n"},{"id":"146662","messageId":"7v4ofjw6t6.fsf@alter.siamese.dyndns.org","threadId":"24561","inReplyTo":"1280335624-90132-1-git-send-email-tor.arne.vestbo@nokia.com","subject":"Re: [PATCH] require_work_tree: Look for top-level instead of is-inside-work-tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-28T23:00:21Z","receivedAt":"2010-07-28T23:00:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tor Arne Vestbø <tor.arne.vestbo@nokia.com> writes:\n\n> The documentation describes require_work_tree as guarding against\n> bare repositories, and that's also the way it's used from porcelain\n> such as git-rebase. When implemented using --is-inside-work-tree\n> the samantics change, causing git-rebase to fail if run from outside\n> GIT_WORK_TREE, even if GIT_WORK_TREE is valid.\n>\n> Signed-off-by: Tor Arne Vestbø <tor.arne.vestbo@nokia.com>\n> ---\n\nThe \"requirement\" is that we _have_ work tree somewhere that we can\ncd-to-toplevel to if we wanted to, not that we _are_ already in the work\ntree.  I can buy that rationale.\n\nHowever, I notice that \"git bisect\", \"git mergetool\" and \"git submodule\"\ndo not seem to do cd_to_topleve immediately after require_work_tree.  The\nlast one has cd_to_toplevel in later parts of the codepath, presumably so\nthat it can collect paths relative to the subdirectory in the work tree.\nI wonder if all of them actually need to be run from inside a work tree?\nDon't they need a separate \"git rev-parse --is-inside-work-tree || die\"\ncheck after require_work_tree (or perhaps cd_to_toplevel) if we apply this\npatch?\n\n\"git rebase--interactive\" also lacks cd_to_toplevel but that is done by\nthe calling \"git rebase\" and I think that one is Ok.\n\n>  git-sh-setup.sh     |    2 +-\n>  t/t1501-worktree.sh |    9 +++++++++\n>  2 files changed, 10 insertions(+), 1 deletions(-)\n>\n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index 6131670..f8e4428 100644\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -141,7 +141,7 @@ cd_to_toplevel () {\n>  }\n>  \n>  require_work_tree () {\n> -\ttest \"$(git rev-parse --is-inside-work-tree 2>/dev/null)\" = true ||\n> +\ttest -n \"$(git rev-parse --show-toplevel 2>/dev/null)\" ||\n>  \tdie \"fatal: $0 cannot be used without a working tree.\"\n>  }\n>  \n> diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh\n> index bd8b607..45b09e7 100755\n> --- a/t/t1501-worktree.sh\n> +++ b/t/t1501-worktree.sh\n> @@ -114,6 +114,15 @@ test_expect_success 'repo finds its work tree from work tree, too' '\n>  \t test sub/dir/tracked = \"$(git ls-files)\")\n>  '\n>  \n> +test_expect_success 'require_work_tree finds work tree' '\n> +\t(cd repo.git/work &&\n> +\t. \"$(git --exec-path)\"/git-sh-setup &&\n> +\tcd .. &&\n> +\trequire_work_tree &&\n> +\tcd .. &&\n> +\trequire_work_tree)\n> +'\n> +\n>  test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' '\n>  \t(cd repo.git/work/sub/dir &&\n>  \tGIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \\\n> -- \n> 1.7.2.19.g48995\n"},{"id":"146775","messageId":"4C52B1BE.8020804@nokia.com","threadId":"24561","inReplyTo":"7v4ofjw6t6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] require_work_tree: Look for top-level instead of is-inside-work-tree","fromName":"Tor Arne Vestbø","fromEmail":"tor.arne.vestbo@nokia.com","sentAt":"2010-07-30T11:04:30Z","receivedAt":"2010-07-30T11:04:30Z","isPatch":true,"sender":{"key":"tor.arne.vestbo@nokia.com","avatar":"https://gravatar.com/avatar/483668eb97b2c2076a95f0ceafe499a5167f313582bc174f1223c6b0ae7aa200?d=mp&s=160"},"body":"Hey, and thanks for your feedback Junio!\n\nOn 29.07.10 01.00, ext Junio C Hamano wrote:\n> Tor Arne Vestbø <tor.arne.vestbo@nokia.com> writes:\n>\n>  > The documentation describes require_work_tree as guarding against\n>  > bare repositories, and that's also the way it's used from porcelain\n>  > such as git-rebase. When implemented using --is-inside-work-tree\n>  > the samantics change, causing git-rebase to fail if run from outside\n>  > GIT_WORK_TREE, even if GIT_WORK_TREE is valid.\n>  >\n>  > Signed-off-by: Tor Arne Vestbø <tor.arne.vestbo@nokia.com>\n>  > ---\n>\n> The \"requirement\" is that we _have_ work tree somewhere that we can\n> cd-to-toplevel to if we wanted to, not that we _are_ already in the work\n> tree. I can buy that rationale.\n\nRight. You put it much nicer than me :)\n\n> However, I notice that \"git bisect\", \"git mergetool\" and \"git submodule\"\n> do not seem to do cd_to_topleve immediately after require_work_tree. The\n> last one has cd_to_toplevel in later parts of the codepath, presumably so\n> that it can collect paths relative to the subdirectory in the work tree.\n> I wonder if all of them actually need to be run from inside a work tree?\n> Don't they need a separate \"git rev-parse --is-inside-work-tree || die\"\n> check after require_work_tree (or perhaps cd_to_toplevel) if we apply this\n> patch?\n\nI think if we have a work tree somewhere, we can at least do \"git \nrev-parse --is-inside-work-tree || cd_to_toplevel\" instead of dying, \nunless there's some danger to that (the user running a command from \noutside GIT_WORK_TREE but expecting GIT_WORK_TREE to not be touched).\n\nFor \"git bisect\" and \"git submodule\" running them in a sub-directory of \nthe work tree complains about needing to be run from the top-level, so I \nassume we can do an unconditional cd_to_toplevel after the \nrequire_work_tree?\n\nFor \"git mergetool\" we should probably do it conditionally only if the \nuser is not inside a work tree already, so that the behavior of running \nthe tool in a sub-directory is not changed.\n\nTor Arne\n"},{"id":"146944","messageId":"4C56D83F.3050507@drmicha.warpmail.net","threadId":"24561","inReplyTo":"1280335624-90132-1-git-send-email-tor.arne.vestbo@nokia.com","subject":"Re: [PATCH] require_work_tree: Look for top-level instead of is-inside-work-tree","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-08-02T14:37:51Z","receivedAt":"2010-08-02T14:37:51Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Tor Arne Vestbø venit, vidit, dixit 28.07.2010 18:47:\n> The documentation describes require_work_tree as guarding against\n> bare repositories, and that's also the way it's used from porcelain\n> such as git-rebase. When implemented using --is-inside-work-tree\n> the samantics change, causing git-rebase to fail if run from outside\n> GIT_WORK_TREE, even if GIT_WORK_TREE is valid.\n> \n> Signed-off-by: Tor Arne Vestbø <tor.arne.vestbo@nokia.com>\n> ---\n>  git-sh-setup.sh     |    2 +-\n>  t/t1501-worktree.sh |    9 +++++++++\n>  2 files changed, 10 insertions(+), 1 deletions(-)\n> \n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index 6131670..f8e4428 100644\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -141,7 +141,7 @@ cd_to_toplevel () {\n>  }\n>  \n>  require_work_tree () {\n> -\ttest \"$(git rev-parse --is-inside-work-tree 2>/dev/null)\" = true ||\n> +\ttest -n \"$(git rev-parse --show-toplevel 2>/dev/null)\" ||\n>  \tdie \"fatal: $0 cannot be used without a working tree.\"\n>  }\n>  \n\nAn alternative which does not change the established behavior of\nrequire_work_tree would be changing the order of require_work_tree and\ncd_to_top_level in the callers where possible along the lines of\n\nhttp://mid.gmane.org/96abf622ca2cf92998ce4ed393ccaa75d95dd9a8.1279112025.git.git@drmicha.warpmail.net\n\nwhich got lost somehow. (The other callers, as mentioned by Junio, would\nneed to be changed differently, e.g. by moving cd_to... earlier.)\n\nAnother problem I noticed back then (I was away since) was that a\nrelative GIT_WORK_TREE is left in place after a cd_to_top_level and\nmesses things up completely - it does not seem to be relative to\nGIT_DIR. So, there seems to be more to fix in this area.\n\nCheers,\nMichael\n"},{"id":"146963","messageId":"7v8w4onc0l.fsf@alter.siamese.dyndns.org","threadId":"24561","inReplyTo":"4C56D83F.3050507@drmicha.warpmail.net","subject":"Re: [PATCH] require_work_tree: Look for top-level instead of is-inside-work-tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-02T17:46:18Z","receivedAt":"2010-08-02T17:46:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n> An alternative which does not change the established behavior of\n> require_work_tree would be changing the order of require_work_tree and\n> cd_to_top_level in the callers where possible along the lines of\n>\n> http://mid.gmane.org/96abf622ca2cf92998ce4ed393ccaa75d95dd9a8.1279112025.git.git@drmicha.warpmail.net\n>\n> which got lost somehow. (The other callers, as mentioned by Junio, would\n> need to be changed differently, e.g. by moving cd_to... earlier.)\n\nDoesn't it sound stupid to \"cd-to-toplevel\" and then \"require-work-tree\"?\n\nIf you can go to the top-level, and once you successfully got there, you\nalready _know_ that you have a work tree (and also you already know at\nthat point you are in the work tree).  The reason why \"require-work-tree\"\nhas been placed before \"cd-to-toplevel\" is exactly for that purpose, I\nthink.  It is possible that some callers wanted to \"require-work-tree\" to\nmean \"I want you to not just _have_ a work tree, but actually be _in_ it\",\nbut I somehow doubt it.  It is more like \"I am going to ask you to go to\nthe top, but let's make sure that you do have a top before doing so\", I\nthink.\n\nI on the other hand do not think it is wrong to lose the existing calls to\nrequire-work-tree if you know that you are going to call cd-to-toplevel\nbefore doing any git operation that needs to have a work-tree, though.\n\n> Another problem I noticed back then (I was away since) was that a\n> relative GIT_WORK_TREE is left in place after a cd_to_top_level and\n> messes things up completely - it does not seem to be relative to\n> GIT_DIR. So, there seems to be more to fix in this area.\n\nI agree; I don't think GIT_WORK_TREE was designed to be anything but an\nabsolute path to begin with.  If a command chdir's around and exports the\nenvironment to its hooks and subcommands, it should be prepared to adjust\nit before doing so.\n"},{"id":"147023","messageId":"4C57CBEF.2070102@drmicha.warpmail.net","threadId":"24561","inReplyTo":"7v8w4onc0l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] require_work_tree: Look for top-level instead of is-inside-work-tree","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-08-03T07:57:35Z","receivedAt":"2010-08-03T07:57:35Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 02.08.2010 19:46:\n> Michael J Gruber <git@drmicha.warpmail.net> writes:\n> \n>> An alternative which does not change the established behavior of\n>> require_work_tree would be changing the order of require_work_tree and\n>> cd_to_top_level in the callers where possible along the lines of\n>>\n>> http://mid.gmane.org/96abf622ca2cf92998ce4ed393ccaa75d95dd9a8.1279112025.git.git@drmicha.warpmail.net\n>>\n>> which got lost somehow. (The other callers, as mentioned by Junio, would\n>> need to be changed differently, e.g. by moving cd_to... earlier.)\n> \n> Doesn't it sound stupid to \"cd-to-toplevel\" and then \"require-work-tree\"?\n\nIt sounds outright silly, agreed.\nThough, unless you know the implementation, \"cd_to_toplevel\" may succeed\ncd'ing to what \"rev-parse --show-toplevel\" returns without\nrequire_work_tree being happy.\n\nBut don't we try to preserve existing behavior unless it's a bug? We\ncertainly have a mismatch of behavior and documentation here. The\nquestion is whether we want to break anyone who relied on\n\"require_work_tree\" dieing when cwd is not within the work-tree.\n\n> \n> If you can go to the top-level, and once you successfully got there, you\n> already _know_ that you have a work tree (and also you already know at\n> that point you are in the work tree).  The reason why \"require-work-tree\"\n> has been placed before \"cd-to-toplevel\" is exactly for that purpose, I\n> think.  It is possible that some callers wanted to \"require-work-tree\" to\n> mean \"I want you to not just _have_ a work tree, but actually be _in_ it\",\n> but I somehow doubt it.  It is more like \"I am going to ask you to go to\n> the top, but let's make sure that you do have a top before doing so\", I\n> think.\n\nWell, if people relied on current behavior...\nI didn't, I don't mind changing this, in fact I'm usually in \"changing\nmood\" and running into the \"preserve behavior\" wall ;)\n\nIn any case, I think \"require_work_tree\" should really test whether we\ncan cd into the worktree, i.e. whether a cd_to_toplevel would succeed,\nand not just whether \"rev-parse --show-toplevel\" returns a non-empty string.\n\n> \n> I on the other hand do not think it is wrong to lose the existing calls to\n> require-work-tree if you know that you are going to call cd-to-toplevel\n> before doing any git operation that needs to have a work-tree, though.\n> \n>> Another problem I noticed back then (I was away since) was that a\n>> relative GIT_WORK_TREE is left in place after a cd_to_top_level and\n>> messes things up completely - it does not seem to be relative to\n>> GIT_DIR. So, there seems to be more to fix in this area.\n> \n> I agree; I don't think GIT_WORK_TREE was designed to be anything but an\n> absolute path to begin with.  If a command chdir's around and exports the\n> environment to its hooks and subcommands, it should be prepared to adjust\n> it before doing so.\n\nWe do have some magic to re-export a relative GIT_DIR as absolute, and\nthe doc says GIT_WORK_TREE is relative to GIT_DIR. We even have a test\nwhich succeeds by pure chance, as playing around with different layouts\nshows. I'll try to come up at least with tests for this when I get to it.\n\nCheers,\nMichael\n"}]}