{"thread":{"id":"52287","subject":"git rev-parse --show-toplevel inside `.git` returns 0 and prints nothing","startedAt":"2019-11-18T22:27:07Z","lastAt":"2019-11-19T08:05:45Z","messageCount":6,"participants":["Anthony Sottile","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"386471","messageId":"CA+dzEBmrMavFJeyPSQr2wA9kFZwz_Kfr6PFBLRfLJ-EuCVXJnA@mail.gmail.com","threadId":"52287","inReplyTo":null,"subject":"git rev-parse --show-toplevel inside `.git` returns 0 and prints nothing","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2019-11-18T22:26:53Z","receivedAt":"2019-11-18T22:27:07Z","isPatch":false,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"I would expect it to either:\n- exit nonzero\n- produce the full path to the root of the repository\n\nwould a patch be accepted which changes it to do one of those two\nthings? I'd be happy to contribute such a patch\n\nAnthony\n"},{"id":"386499","messageId":"xmqqk17wziex.fsf@gitster-ct.c.googlers.com","threadId":"52287","inReplyTo":"CA+dzEBmrMavFJeyPSQr2wA9kFZwz_Kfr6PFBLRfLJ-EuCVXJnA@mail.gmail.com","subject":"Re: git rev-parse --show-toplevel inside `.git` returns 0 and prints nothing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-19T02:52:54Z","receivedAt":"2019-11-19T02:53:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anthony Sottile <asottile@umich.edu> writes:\n\n> I would expect it to either:\n> - exit nonzero\n> - produce the full path to the root of the repository\n>\n> would a patch be accepted which changes it to do one of those two\n> things? I'd be happy to contribute such a patch\n\nThe most important invariant for the \"--show-toplevel\" and the\n\"--show-prefix\" options was that when they are concatenated the\nresult matches the current directory in a working tree.  So I think\nthe result is undefined when you are not anywhere in the working\ntree.\n\nHaving said that, as scripts that want to know if they are inside\n.git directory (either directly in it, or in its subdirectories)\nshould not be relying on the behaviour, and instead be using the\n\"--is-inside-git-dir\" option, I suspect that it won't be too\ndisruptive to change the behaviour after all these years.  \n\nBut it is no longer year 2007 and with widespread use of Git, it is\nalmost guaranteed to break somebody's script ;-)\n\nIf I were designing the feature today, with today's rest-of-git in\nmind, I would say\n\n - In a bare repository, exit with non-zero status after giving an\n   error message \"no working tree\".\n\n - In a repository that has a single associated working tree, show\n   the path to the top-level of that working tree and exit with zero\n   status.\n\nIn a repository that has more than one working trees (which is one\nof the things \"todasy's rest-of-git\" has that did not exist back\nwhen --show-prefix/--show-toplevel etc. were invented), then what?\nWould it make sense to show the primary working tree?  What if the\nworktree(s) were made off of a bare repository, in which case nobody\nis the primary?\n\nIf we were changing \"--show-toplevel\", we may need to make changes\nto \"--show-prefix\" to things consistent, but I am not sure what it\nshould say when run outside a working tree.  It should continue to\ngive nothing; it may make sense to exit with non-zero status, but\nI'd rather let sleeping dogs lie.  Nobody is hurting by the command\nexiting with zero status (the same question applies to the\n\"--show-toplevel\" option, for that matter, though), when the result\nis undefined.\n\nSo...  I dunno.\n\n\n\n"},{"id":"386503","messageId":"20191119033311.GA18613@sigill.intra.peff.net","threadId":"52287","inReplyTo":"xmqqk17wziex.fsf@gitster-ct.c.googlers.com","subject":"Re: git rev-parse --show-toplevel inside `.git` returns 0 and prints nothing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-19T03:33:11Z","receivedAt":"2019-11-19T03:33:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 19, 2019 at 11:52:54AM +0900, Junio C Hamano wrote:\n\n> If I were designing the feature today, with today's rest-of-git in\n> mind, I would say\n> \n>  - In a bare repository, exit with non-zero status after giving an\n>    error message \"no working tree\".\n> \n>  - In a repository that has a single associated working tree, show\n>    the path to the top-level of that working tree and exit with zero\n>    status.\n\nDo you mean to do this even in when the cwd is inside .git?\n\nI think that's confusing, because you don't actually have a working tree\nat all. E.g.:\n\n  $ git rev-parse --show-toplevel\n  /home/peff/tmp\n  $ git status -b --short\n  ## No commits yet on master\n\n  $ cd .git\n  $ git rev-parse --show-toplevel\n  $ git status -b --short\n  fatal: this operation must be run in a work tree\n\nSo internal commands like status accept that we have no working tree in\nthis situation. But \"--show-toplevel\" just prints nothing. I'd amend\nyour second point to be \"If we are in the working tree of a repository,\nshow the path to the top-level of that working tree and exit with zero\nstatus\".\n\nAnd then that leaves another case: we are not in the working tree of the\nrepository. In which case I think it should be the same as the bare\nrepository.\n\nAnd from that, your multi-working-tree case falls out naturally:\n\n> In a repository that has more than one working trees (which is one\n> of the things \"todasy's rest-of-git\" has that did not exist back\n> when --show-prefix/--show-toplevel etc. were invented), then what?\n> Would it make sense to show the primary working tree?  What if the\n> worktree(s) were made off of a bare repository, in which case nobody\n> is the primary?\n\nThere may be multiple working trees, but we can only be in one of them\nat a time. So that's the one that we show.\n\nAnd the only real change here is that \"--show-toplevel\" prints an error\nand exits non-zero when we won't have a working tree. Something like:\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 3857fd1b8a..81161f2dfb 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -805,6 +805,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tconst char *work_tree = get_git_work_tree();\n \t\t\t\tif (work_tree)\n \t\t\t\t\tputs(work_tree);\n+\t\t\t\telse\n+\t\t\t\t\tdie(\"this operation must be run in a work tree\");\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--show-superproject-working-tree\")) {\n\n\nI think the reason this hasn't come up until now is callers are expected\nto use require_work_tree() or \"rev-parse --is-inside-work-tree\" first.\n\nIt would probably make sense for the rev-parse documentation to also\nclarify what \"the top-level directory\" is.\n\n-Peff\n"},{"id":"386505","messageId":"CA+dzEBmekzDVdqy=4GDF+Wm8e-YTPEdbh0oVowZNQYO67vEhEg@mail.gmail.com","threadId":"52287","inReplyTo":"20191119033311.GA18613@sigill.intra.peff.net","subject":"Re: git rev-parse --show-toplevel inside `.git` returns 0 and prints nothing","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2019-11-19T04:13:02Z","receivedAt":"2019-11-19T04:13:14Z","isPatch":false,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"On Mon, Nov 18, 2019 at 7:33 PM Jeff King <peff@peff.net> wrote:\n>\n> On Tue, Nov 19, 2019 at 11:52:54AM +0900, Junio C Hamano wrote:\n>\n> > If I were designing the feature today, with today's rest-of-git in\n> > mind, I would say\n> >\n> >  - In a bare repository, exit with non-zero status after giving an\n> >    error message \"no working tree\".\n> >\n> >  - In a repository that has a single associated working tree, show\n> >    the path to the top-level of that working tree and exit with zero\n> >    status.\n>\n> Do you mean to do this even in when the cwd is inside .git?\n>\n> I think that's confusing, because you don't actually have a working tree\n> at all. E.g.:\n>\n>   $ git rev-parse --show-toplevel\n>   /home/peff/tmp\n>   $ git status -b --short\n>   ## No commits yet on master\n>\n>   $ cd .git\n>   $ git rev-parse --show-toplevel\n>   $ git status -b --short\n>   fatal: this operation must be run in a work tree\n>\n> So internal commands like status accept that we have no working tree in\n> this situation. But \"--show-toplevel\" just prints nothing. I'd amend\n> your second point to be \"If we are in the working tree of a repository,\n> show the path to the top-level of that working tree and exit with zero\n> status\".\n>\n> And then that leaves another case: we are not in the working tree of the\n> repository. In which case I think it should be the same as the bare\n> repository.\n>\n> And from that, your multi-working-tree case falls out naturally:\n>\n> > In a repository that has more than one working trees (which is one\n> > of the things \"todasy's rest-of-git\" has that did not exist back\n> > when --show-prefix/--show-toplevel etc. were invented), then what?\n> > Would it make sense to show the primary working tree?  What if the\n> > worktree(s) were made off of a bare repository, in which case nobody\n> > is the primary?\n>\n> There may be multiple working trees, but we can only be in one of them\n> at a time. So that's the one that we show.\n>\n> And the only real change here is that \"--show-toplevel\" prints an error\n> and exits non-zero when we won't have a working tree. Something like:\n>\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index 3857fd1b8a..81161f2dfb 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -805,6 +805,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>                                 const char *work_tree = get_git_work_tree();\n>                                 if (work_tree)\n>                                         puts(work_tree);\n> +                               else\n> +                                       die(\"this operation must be run in a work tree\");\n>                                 continue;\n>                         }\n>                         if (!strcmp(arg, \"--show-superproject-working-tree\")) {\n>\n>\n> I think the reason this hasn't come up until now is callers are expected\n> to use require_work_tree() or \"rev-parse --is-inside-work-tree\" first.\n>\n> It would probably make sense for the rev-parse documentation to also\n> clarify what \"the top-level directory\" is.\n>\n> -Peff\n\nI realize I forgot to include the X to my Y :) -- this was a totally\nsilly case that I got as a bug report:\nhttps://github.com/pre-commit/pre-commit/issues/1219\n\nI *expected* an error case but didn't get one\n\nAnthony\n"},{"id":"386511","messageId":"20191119073754.GA30634@sigill.intra.peff.net","threadId":"52287","inReplyTo":"CA+dzEBmekzDVdqy=4GDF+Wm8e-YTPEdbh0oVowZNQYO67vEhEg@mail.gmail.com","subject":"Re: git rev-parse --show-toplevel inside `.git` returns 0 and prints nothing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-19T07:37:54Z","receivedAt":"2019-11-19T07:37:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 18, 2019 at 08:13:02PM -0800, Anthony Sottile wrote:\n\n> > I think the reason this hasn't come up until now is callers are expected\n> > to use require_work_tree() or \"rev-parse --is-inside-work-tree\" first.\n> >\n> > It would probably make sense for the rev-parse documentation to also\n> > clarify what \"the top-level directory\" is.\n> >\n> > -Peff\n> \n> I realize I forgot to include the X to my Y :) -- this was a totally\n> silly case that I got as a bug report:\n> https://github.com/pre-commit/pre-commit/issues/1219\n> \n> I *expected* an error case but didn't get one\n\nYes, and I do agree that an error is the right thing.\n\nWould that have helped your pre-commit script? I guess it would have\nbarfed at that point. :) It sounds like it should be checking first that\nit has a working tree.\n\n-Peff\n"},{"id":"386512","messageId":"20191119080543.GA14313@sigill.intra.peff.net","threadId":"52287","inReplyTo":"20191119033311.GA18613@sigill.intra.peff.net","subject":"Re: git rev-parse --show-toplevel inside `.git` returns 0 and prints nothing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-19T08:05:43Z","receivedAt":"2019-11-19T08:05:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 18, 2019 at 10:33:11PM -0500, Jeff King wrote:\n\n> And the only real change here is that \"--show-toplevel\" prints an error\n> and exits non-zero when we won't have a working tree. Something like:\n> \n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index 3857fd1b8a..81161f2dfb 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -805,6 +805,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>  \t\t\t\tconst char *work_tree = get_git_work_tree();\n>  \t\t\t\tif (work_tree)\n>  \t\t\t\t\tputs(work_tree);\n> +\t\t\t\telse\n> +\t\t\t\t\tdie(\"this operation must be run in a work tree\");\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n>  \t\t\tif (!strcmp(arg, \"--show-superproject-working-tree\")) {\n> \n> \n> I think the reason this hasn't come up until now is callers are expected\n> to use require_work_tree() or \"rev-parse --is-inside-work-tree\" first.\n> \n> It would probably make sense for the rev-parse documentation to also\n> clarify what \"the top-level directory\" is.\n\nHere it is wrapped up with a commit message, a test, and a documentation\nfix. I have to admit I'm having second thoughts, though. It _is_\nslightly confusing, and this is what I would do if we were designing\nfrom scratch. But the current behavior, while weird, does let the caller\ndistinguish all cases. So another option would just be to document the\noutcome more clearly. I'm on the fence.\n\n-- >8 --\nSubject: [PATCH] rev-parse: make --show-toplevel without a worktree an error\n\nEver since it was introduced in 7cceca5ccc (Add 'git rev-parse\n--show-toplevel' option., 2010-01-12), the --show-toplevel option has\ntreated a missing working tree as a quiet success: it neither prints a\ntoplevel path, but nor does it report any kind of error.\n\nWhile a caller could distinguish this case by looking for an empty\nresponse, the behavior is rather confusing. We're better off complaining\nthat there is no working tree, as other internal commands would do in\nsimilar cases (e.g., \"git status\" or any builtin with NEED_WORK_TREE set\nwould just die()). So let's do the same here.\n\nWhile we're at it, let's clarify the documentation and add some tests,\nboth for the new behavior and for the more mundane case (which was not\ncovered).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git-rev-parse.txt |  3 ++-\n builtin/rev-parse.c             |  2 ++\n t/t1500-rev-parse.sh            | 10 ++++++++++\n 3 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\nindex 9985477efe..19b12b6d43 100644\n--- a/Documentation/git-rev-parse.txt\n+++ b/Documentation/git-rev-parse.txt\n@@ -262,7 +262,8 @@ print a message to stderr and exit with nonzero status.\n \tdirectory.\n \n --show-toplevel::\n-\tShow the absolute path of the top-level directory.\n+\tShow the absolute path of the top-level directory of the working\n+\ttree. If there is no working tree, report an error.\n \n --show-superproject-working-tree::\n \tShow the absolute path of the root of the superproject's\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 85ce2095bf..7a00da8203 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -803,6 +803,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tconst char *work_tree = get_git_work_tree();\n \t\t\t\tif (work_tree)\n \t\t\t\t\tputs(work_tree);\n+\t\t\t\telse\n+\t\t\t\t\tdie(\"this operation must be run in a work tree\");\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--show-superproject-working-tree\")) {\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 0177fd815c..603019b541 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -146,6 +146,16 @@ test_expect_success 'rev-parse --show-object-format in repo' '\n \tgrep \"unknown mode for --show-object-format: squeamish-ossifrage\" err\n '\n \n+test_expect_success '--show-toplevel from subdir of working tree' '\n+\tpwd >expect &&\n+\tgit -C sub/dir rev-parse --show-toplevel >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--show-toplevel from inside .git' '\n+\ttest_must_fail git -C .git rev-parse --show-toplevel\n+'\n+\n test_expect_success 'showing the superproject correctly' '\n \tgit rev-parse --show-superproject-working-tree >out &&\n \ttest_must_be_empty out &&\n-- \n2.24.0.512.g217e13b85d\n\n"}]}