{"thread":{"id":"41541","subject":"[PATCH/RFC 0/2] fix some rev-parse options in non-repos","startedAt":"2016-02-26T23:25:08Z","lastAt":"2016-02-29T21:29:43Z","messageCount":13,"participants":["Jeff King","Junio C Hamano","John Keeping","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"279578","messageId":"20160226232507.GA9404@sigill.intra.peff.net","threadId":"41541","inReplyTo":null,"subject":"[PATCH/RFC 0/2] fix some rev-parse options in non-repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-26T23:25:08Z","receivedAt":"2016-02-26T23:25:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Michael Haggerty noticed recently (off-list) that \"git rev-parse\n--local-env-vars\" doesn't work outside of a git repository. This turns\nout to be a regression in v1.8.5, due to a patch by John Keeping that\nlifted some other restrictions on how the option could be used.\n\nThis fixes it by reverting John's patch, which puts the original\nrestrictions back in place. I won't repeat the lengthy discussion from\npatch 2's commit message here, but the gist of it is that probably\nnobody cares about those restrictions, it's more important to fix the\noriginal regression, and it's probably too hard to make both work.\n\nThe only thing that gives me pause (and hence the RFC) is that it has\nbeen over 2 years since the original regression. So it's entirely\npossible somebody will consider _this_ fix a regression.\n\n  [1/2]: t1515: add tests for rev-parse out-of-repo helpers\n  [2/2]: Revert \"rev-parse: remove restrictions on some options\"\n\n-Peff\n"},{"id":"279580","messageId":"20160226232632.GA9552@sigill.intra.peff.net","threadId":"41541","inReplyTo":"20160226232507.GA9404@sigill.intra.peff.net","subject":"[PATCH 1/2] t1515: add tests for rev-parse out-of-repo helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-26T23:26:32Z","receivedAt":"2016-02-26T23:26:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The git-rev-parse command is a dumping ground for helpers\nthat let scripts make various queries of git. Many of these\nare conceptually independent of being inside a git\nrepository.\n\nWith the exception of --parseopt, we do not directly test\nmost of these features in our test suite. Let's give them\nsome basic sanity checks, which reveals that some of them\nhave been broken for some time when run from outside a\nrepository.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t1515-rev-parse-outside-repo.sh | 45 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 45 insertions(+)\n create mode 100755 t/t1515-rev-parse-outside-repo.sh\n\ndiff --git a/t/t1515-rev-parse-outside-repo.sh b/t/t1515-rev-parse-outside-repo.sh\nnew file mode 100755\nindex 0000000..ae33093\n--- /dev/null\n+++ b/t/t1515-rev-parse-outside-repo.sh\n@@ -0,0 +1,45 @@\n+#!/bin/sh\n+\n+test_description='check that certain rev-parse options work outside repo'\n+. ./test-lib.sh\n+\n+test_expect_success 'set up non-repo directory' '\n+\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\texport GIT_CEILING_DIRECTORIES &&\n+\tmkdir non-repo &&\n+\tcd non-repo &&\n+\t# confirm that git does not find a repo\n+\ttest_must_fail git rev-parse --git-dir\n+'\n+\n+# Rather than directly test the output of sq-quote directly,\n+# make sure the shell can read back a tricky case, since\n+# that's what we really care about anyway.\n+tricky=\"really tricky with \\\\ and \\\" and '\"\n+dump_args () {\n+\tfor i in \"$@\"; do\n+\t\techo \"arg: $i\"\n+\tdone\n+}\n+test_expect_success 'rev-parse --sq-quote' '\n+\tdump_args \"$tricky\" easy >expect &&\n+\teval \"dump_args $(git rev-parse --sq-quote \"$tricky\" easy)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'rev-parse --local-env-vars' '\n+\tgit rev-parse --local-env-vars >actual &&\n+\t# we do not want to depend on the complete list here,\n+\t# so just look for something plausible\n+\tgrep ^GIT_DIR actual\n+'\n+\n+test_expect_failure 'rev-parse --resolve-git-dir' '\n+\tgit init --separate-git-dir repo dir &&\n+\ttest_must_fail git rev-parse --resolve-git-dir . &&\n+\techo \"$(pwd)/repo\" >expect &&\n+\tgit rev-parse --resolve-git-dir dir/.git >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.7.2.767.g705917e\n"},{"id":"279581","messageId":"20160226232957.GB9552@sigill.intra.peff.net","threadId":"41541","inReplyTo":"20160226232507.GA9404@sigill.intra.peff.net","subject":"[PATCH 2/2] Revert \"rev-parse: remove restrictions on some options\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-26T23:29:57Z","receivedAt":"2016-02-26T23:29:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This reverts commit 68889b416d5b6a5cf7d280a428281d635fe9b292.\n\nThat commit bumped some rev-parse options into the main\noption-parsing loop, which meant that they no longer had to\ncome at the very beginning of the option list. However, that\nalso means that they now came after our call to\nsetup_git_directory(), and will no longer run outside of a\ngit repository.\n\nFor --local-env-vars, this is semi-questionable. The main\nuse of that option is to get a list of environment variables\nto clear, and if you are not in a repository, there\n_probably_ isn't anything to clear. But it places an\nunnecessary restriction on callers who may be using it\npreemptively.\n\nFor --resolve-git-dir, it is almost certainly a regression.\nThat option is about finding a git dir in the first place,\nso it is reasonably likely to be called from outside an\nexisting repository.\n\nThe best solution here would be to have a full parsing loop\nthat handles all options, but only calls setup_git_directory\nas appropriate. Unfortunately, that's a bit complicated to\nimplement. We _have_ to handle each option in the order it\nappears on the command line. If the caller asked for:\n\n  git rev-parse --resolve-git-dir foo/.git --show-toplevel\n\nthen it must receive the two lines of output in the correct\nto know which is which. But asking for:\n\n  git rev-parse --show-toplevel --resolve-git-dir foo/.git\n\nis weird; we have to setup_git_directory() for the first\noption.\n\nSo any implementation would probably have to either:\n\n  - make two passes over the options, first figuring out\n    whether we need a git-dir, and then actually handling\n    the options. That's possible, but it's probably not\n    worth the trouble.\n\n  - call setup_git_directory() on the fly when an option\n    needs it; that requires annotating all of the options,\n    and there are a considerable number of them.\n\nThe original patch was not spurred by an actual bug report,\nbut by an observation[1] that was essentially \"eh, this\nlooks unnecessarily restrictive\". It _is_ restrictive, but\nit turns out to be necessarily so. :)\n\nAnd in practice, it is unlikely anybody was bothered by the\nrestriction. It's not really sane to use --local-env-vars\nwith other options, anyway, as it produces unbounded output\n(so the caller only know it ends at EOF). It's more\nplausible for --resolve-git-dir to be used with other\noptions, but still unlikely. It's main use is accessing\n_other_ repositories (e.g., submodules), so making a query\non the main repository at the same time isn't very useful.\n\nThis patch therefore just reverts 68889b416, with a few\ncaveats:\n\n  1. Since the original change, --resolve-git-dir learned to\n     avoid segfaulting on a bogus. We don't know need to\n     backport that, because the \"restricted\" form checks\n     argc.\n\n  2. The original patch mentioned that we didn't need to\n     clean up any documentation, because the restriction\n     wasn't documented. We can at least fix that by\n     mentioning the restriction in the manpage.\n\n  3. We can now mark our newly-added tests as passing. :)\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/230849\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git-rev-parse.txt   | 27 ++++++++++++++-------------\n builtin/rev-parse.c               | 31 +++++++++++++++----------------\n t/t1515-rev-parse-outside-repo.sh |  4 ++--\n 3 files changed, 31 insertions(+), 31 deletions(-)\n\ndiff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\nindex b6c6326..0f2bb9b 100644\n--- a/Documentation/git-rev-parse.txt\n+++ b/Documentation/git-rev-parse.txt\n@@ -28,7 +28,8 @@ OPTIONS\n Operation Modes\n ~~~~~~~~~~~~~~~\n \n-Each of these options must appear first on the command line.\n+Each of these options must appear first on the command line; they do not\n+need to be run in a git repository.\n \n --parseopt::\n \tUse 'git rev-parse' in option parsing mode (see PARSEOPT section below).\n@@ -38,6 +39,18 @@ Each of these options must appear first on the command line.\n \tsection below). In contrast to the `--sq` option below, this\n \tmode does only quoting. Nothing else is done to command input.\n \n+--local-env-vars::\n+\tList the GIT_* environment variables that are local to the\n+\trepository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).\n+\tOnly the names of the variables are listed, not their value,\n+\teven if they are set.\n+\n+--resolve-git-dir <path>::\n+\tCheck if <path> is a valid repository or a gitfile that\n+\tpoints at a valid repository, and print the location of the\n+\trepository.  If <path> is a gitfile then the resolved path\n+\tto the real repository is printed.\n+\n Options for --parseopt\n ~~~~~~~~~~~~~~~~~~~~~~\n \n@@ -201,12 +214,6 @@ explicitly.\n Options for Files\n ~~~~~~~~~~~~~~~~~\n \n---local-env-vars::\n-\tList the GIT_* environment variables that are local to the\n-\trepository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).\n-\tOnly the names of the variables are listed, not their value,\n-\teven if they are set.\n-\n --git-dir::\n \tShow `$GIT_DIR` if defined. Otherwise show the path to\n \tthe .git directory. The path shown, when relative, is\n@@ -230,12 +237,6 @@ print a message to stderr and exit with nonzero status.\n --is-bare-repository::\n \tWhen the repository is bare print \"true\", otherwise \"false\".\n \n---resolve-git-dir <path>::\n-\tCheck if <path> is a valid repository or a gitfile that\n-\tpoints at a valid repository, and print the location of the\n-\trepository.  If <path> is a gitfile then the resolved path\n-\tto the real repository is printed.\n-\n --git-path <path>::\n \tResolve \"$GIT_DIR/<path>\" and takes other path relocation\n \tvariables such as $GIT_OBJECT_DIRECTORY,\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex cf8487b..ccc0328 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -518,6 +518,21 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \tif (argc > 1 && !strcmp(\"--sq-quote\", argv[1]))\n \t\treturn cmd_sq_quote(argc - 2, argv + 2);\n \n+\tif (argc == 2 && !strcmp(\"--local-env-vars\", argv[1])) {\n+\t\tint i;\n+\t\tfor (i = 0; local_repo_env[i]; i++)\n+\t\t\tprintf(\"%s\\n\", local_repo_env[i]);\n+\t\treturn 0;\n+\t}\n+\n+\tif (argc > 2 && !strcmp(argv[1], \"--resolve-git-dir\")) {\n+\t\tconst char *gitdir = resolve_gitdir(argv[2]);\n+\t\tif (!gitdir)\n+\t\t\tdie(\"not a gitdir '%s'\", argv[2]);\n+\t\tputs(gitdir);\n+\t\treturn 0;\n+\t}\n+\n \tif (argc > 1 && !strcmp(\"-h\", argv[1]))\n \t\tusage(builtin_rev_parse_usage);\n \n@@ -706,12 +721,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tadd_ref_exclusion(&ref_excludes, arg + 10);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (!strcmp(arg, \"--local-env-vars\")) {\n-\t\t\t\tint i;\n-\t\t\t\tfor (i = 0; local_repo_env[i]; i++)\n-\t\t\t\t\tprintf(\"%s\\n\", local_repo_env[i]);\n-\t\t\t\tcontinue;\n-\t\t\t}\n \t\t\tif (!strcmp(arg, \"--show-toplevel\")) {\n \t\t\t\tconst char *work_tree = get_git_work_tree();\n \t\t\t\tif (work_tree)\n@@ -767,16 +776,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tputs(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (!strcmp(arg, \"--resolve-git-dir\")) {\n-\t\t\t\tconst char *gitdir = argv[++i];\n-\t\t\t\tif (!gitdir)\n-\t\t\t\t\tdie(\"--resolve-git-dir requires an argument\");\n-\t\t\t\tgitdir = resolve_gitdir(gitdir);\n-\t\t\t\tif (!gitdir)\n-\t\t\t\t\tdie(\"not a gitdir '%s'\", argv[i]);\n-\t\t\t\tputs(gitdir);\n-\t\t\t\tcontinue;\n-\t\t\t}\n \t\t\tif (!strcmp(arg, \"--is-inside-git-dir\")) {\n \t\t\t\tprintf(\"%s\\n\", is_inside_git_dir() ? \"true\"\n \t\t\t\t\t\t: \"false\");\ndiff --git a/t/t1515-rev-parse-outside-repo.sh b/t/t1515-rev-parse-outside-repo.sh\nindex ae33093..3ec2971 100755\n--- a/t/t1515-rev-parse-outside-repo.sh\n+++ b/t/t1515-rev-parse-outside-repo.sh\n@@ -27,14 +27,14 @@ test_expect_success 'rev-parse --sq-quote' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'rev-parse --local-env-vars' '\n+test_expect_success 'rev-parse --local-env-vars' '\n \tgit rev-parse --local-env-vars >actual &&\n \t# we do not want to depend on the complete list here,\n \t# so just look for something plausible\n \tgrep ^GIT_DIR actual\n '\n \n-test_expect_failure 'rev-parse --resolve-git-dir' '\n+test_expect_success 'rev-parse --resolve-git-dir' '\n \tgit init --separate-git-dir repo dir &&\n \ttest_must_fail git rev-parse --resolve-git-dir . &&\n \techo \"$(pwd)/repo\" >expect &&\n-- \n2.7.2.767.g705917e\n"},{"id":"279583","messageId":"20160226233449.GA9622@sigill.intra.peff.net","threadId":"41541","inReplyTo":"20160226232957.GB9552@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] Revert \"rev-parse: remove restrictions on some options\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-26T23:34:49Z","receivedAt":"2016-02-26T23:34:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 26, 2016 at 06:29:57PM -0500, Jeff King wrote:\n\n> The best solution here would be to have a full parsing loop\n> that handles all options, but only calls setup_git_directory\n> as appropriate. Unfortunately, that's a bit complicated to\n> implement. We _have_ to handle each option in the order it\n> appears on the command line. If the caller asked for:\n> \n>   git rev-parse --resolve-git-dir foo/.git --show-toplevel\n> \n> then it must receive the two lines of output in the correct\n> to know which is which. But asking for:\n> \n>   git rev-parse --show-toplevel --resolve-git-dir foo/.git\n> \n> is weird; we have to setup_git_directory() for the first\n> option.\n> \n> So any implementation would probably have to either:\n> \n>   - make two passes over the options, first figuring out\n>     whether we need a git-dir, and then actually handling\n>     the options. That's possible, but it's probably not\n>     worth the trouble.\n> \n>   - call setup_git_directory() on the fly when an option\n>     needs it; that requires annotating all of the options,\n>     and there are a considerable number of them.\n\nHaving just sent this, of course, it occurs to me that a loop like:\n\n   setup_git_directory_gently(&nongit);\n   for (i = 0; i < argc; i++) {\n  \tif (!strcmp(argv[i], \"--local-env-vars\"))\n  \t... and other nongit-ok options ...\n  \n  \tif (nongit)\n  \t\tdie(\"need a repo\");\n  \n  \tif (!strcmp(argv[i], \"--git-dir\"))\n  \t... and other options ...\n   }\n\nwould probably work. It does still leave things like --parseopt and\n--sq-quote as one-offs, though, because they consume the rest of the\ncommand line. And the fact remains that --local-repo-env isn't really\nsuitable for use with other options.\n\nSo I'm still tempted by this \"lazy\" approach.\n\n-Peff\n"},{"id":"279587","messageId":"xmqqbn73hwam.fsf@gitster.mtv.corp.google.com","threadId":"41541","inReplyTo":"20160226233449.GA9622@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] Revert \"rev-parse: remove restrictions on some options\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-26T23:44:01Z","receivedAt":"2016-02-26T23:44:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Feb 26, 2016 at 06:29:57PM -0500, Jeff King wrote:\n>\n>> The best solution here would be to have a full parsing loop\n>> that handles all options, but only calls setup_git_directory\n>> as appropriate. Unfortunately, that's a bit complicated to\n>> implement. We _have_ to handle each option in the order it\n>> appears on the command line. If the caller asked for:\n>> \n>>   git rev-parse --resolve-git-dir foo/.git --show-toplevel\n>> \n>> then it must receive the two lines of output in the correct\n>> to know which is which. But asking for:\n>> \n>>   git rev-parse --show-toplevel --resolve-git-dir foo/.git\n>> \n>> is weird; we have to setup_git_directory() for the first\n>> option.\n>> \n>> So any implementation would probably have to either:\n>> \n>>   - make two passes over the options, first figuring out\n>>     whether we need a git-dir, and then actually handling\n>>     the options. That's possible, but it's probably not\n>>     worth the trouble.\n>> \n>>   - call setup_git_directory() on the fly when an option\n>>     needs it; that requires annotating all of the options,\n>>     and there are a considerable number of them.\n>\n> Having just sent this, of course, it occurs to me that a loop like:\n>\n>    setup_git_directory_gently(&nongit);\n>    for (i = 0; i < argc; i++) {\n>   \tif (!strcmp(argv[i], \"--local-env-vars\"))\n>   \t... and other nongit-ok options ...\n>   \n>   \tif (nongit)\n>   \t\tdie(\"need a repo\");\n>   \n>   \tif (!strcmp(argv[i], \"--git-dir\"))\n>   \t... and other options ...\n>    }\n>\n> would probably work. It does still leave things like --parseopt and\n> --sq-quote as one-offs, though, because they consume the rest of the\n> command line. And the fact remains that --local-repo-env isn't really\n> suitable for use with other options.\n>\n> So I'm still tempted by this \"lazy\" approach.\n>\n> -Peff\n\nYup.\n\nBut why do you even need to run local-env-vars from outside a\nrepository in the first place?\n"},{"id":"279598","messageId":"20160227032236.GA10313@sigill.intra.peff.net","threadId":"41541","inReplyTo":"xmqqbn73hwam.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] Revert \"rev-parse: remove restrictions on some options\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-27T03:22:37Z","receivedAt":"2016-02-27T03:22:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 26, 2016 at 03:44:01PM -0800, Junio C Hamano wrote:\n\n> But why do you even need to run local-env-vars from outside a\n> repository in the first place?\n\nThe short answer is: because it is about clearing the state to move into\na new repository, and we do not necessarily know what the old state was.\n\nHere's a longer answer.\n\nWe (GitHub) have some scripts that preemptively clear the git env when\nmoving into another repository directory, so that the environment\ndoesn't lead to us operating in the wrong repository.\n\nFor example, we use alternates to share object storage between a series\nof forks. So frequently in such scripts we may need to switch between\nrepositories (e.g., to sync a fork to the shared storage, and then go\nback to the shared storage and run repack). To do so safely, we have to\nclear the git env for each \"cd\" (otherwise things work fine when\n$GIT_DIR is not set and we rely on auto-finding the repo, and break\nhorribly if the script is run with $GIT_DIR set).\n\nThere are a few corner cases where library code wants to \"cd\", but\ndoesn't know if it's coming from another repo or not.  So it clears the\ngit env to be on the safe side. We could fix it by always going to the\nnew repo and running \"unset $(git rev-parse --local-env-vars)\" there,\nbut I think that just has the opposite problem (you may be _leaving_ a\nrepository, and want to make sure you are no longer in one).\n\nFor us, it's mostly just an annoyance. rev-parse produces no output so\nwe don't clear any variables, and its stderr gets logged somewhere. We\nreally only care about $GIT_DIR, and if that is set to something valid,\nthen you are in a repo, rev-parse works, and we clear it. But you can\ncome up with cases where it does the wrong thing, like:\n\n  # Work in some repo; set some git vars in the environment, but\n  # rely on auto-discovery to find the actual repo.\n  cd some-git-repo\n  export GIT_WORK_TREE=/whatever\n  git ...\n\n  # Now go back to our root and do some work elsewhere.\n  # We're no longer in a git repo.\n  cd ..\n  ... run some non-git commands ...\n\n  # Now we want to go into a new repo. Clear the environment.\n  unset $(git rev-parse --local-env-vars)\n  cd ../another-git-repo\n  git ...\n\nIn the third directory, you'd still have GIT_WORK_TREE set, even though\nyou asked to clear existing git state.\n\nI don't think we have any scripts that do that, but it doesn't seem that\nimplausible to me.\n\n-Peff\n"},{"id":"279637","messageId":"20160227122511.GR1766@serenity.lan","threadId":"41541","inReplyTo":"20160226232957.GB9552@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] Revert \"rev-parse: remove restrictions on some options\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-02-27T12:25:11Z","receivedAt":"2016-02-27T12:25:11Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Feb 26, 2016 at 06:29:57PM -0500, Jeff King wrote:\n> This reverts commit 68889b416d5b6a5cf7d280a428281d635fe9b292.\n[snip]\n> The original patch was not spurred by an actual bug report,\n> but by an observation[1] that was essentially \"eh, this\n> looks unnecessarily restrictive\". It _is_ restrictive, but\n> it turns out to be necessarily so. :)\n\nThe aim of the original series was to improve the documentation, so I\ndon't think it's unreasonable to consider this a regression and revert\nthe functional change.  Although I think we can improve the behaviour\nslightly (see below).\n\n> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\n> index b6c6326..0f2bb9b 100644\n> --- a/Documentation/git-rev-parse.txt\n> +++ b/Documentation/git-rev-parse.txt\n> @@ -28,7 +28,8 @@ OPTIONS\n>  Operation Modes\n>  ~~~~~~~~~~~~~~~\n>  \n> -Each of these options must appear first on the command line.\n> +Each of these options must appear first on the command line; they do not\n> +need to be run in a git repository.\n>  \n>  --parseopt::\n>  \tUse 'git rev-parse' in option parsing mode (see PARSEOPT section below).\n> @@ -38,6 +39,18 @@ Each of these options must appear first on the command line.\n>  \tsection below). In contrast to the `--sq` option below, this\n>  \tmode does only quoting. Nothing else is done to command input.\n>  \n> +--local-env-vars::\n> +\tList the GIT_* environment variables that are local to the\n> +\trepository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).\n> +\tOnly the names of the variables are listed, not their value,\n> +\teven if they are set.\n\nI think we should add:\n\n\tNo other arguments may be supplied.\n\n> +--resolve-git-dir <path>::\n> +\tCheck if <path> is a valid repository or a gitfile that\n> +\tpoints at a valid repository, and print the location of the\n> +\trepository.  If <path> is a gitfile then the resolved path\n> +\tto the real repository is printed.\n\nAgain I think this should say that only the `path` argument may be\nsupplied.\n\n>  --git-path <path>::\n>  \tResolve \"$GIT_DIR/<path>\" and takes other path relocation\n>  \tvariables such as $GIT_OBJECT_DIRECTORY,\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index cf8487b..ccc0328 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -518,6 +518,21 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>  \tif (argc > 1 && !strcmp(\"--sq-quote\", argv[1]))\n>  \t\treturn cmd_sq_quote(argc - 2, argv + 2);\n>  \n> +\tif (argc == 2 && !strcmp(\"--local-env-vars\", argv[1])) {\n\nMaybe:\n\n\tif (argc > 1 && !strcmp(\"--local-env-vars\", argv[1])) {\n\t\tif (argc != 2)\n\t\t\tdie(\"--local-env-vars must be the only argument\");\n\nsince the behaviour of:\n\n\t$ git rev-parse --local-env-vars --\n\t--local-env-vars\n\t--\n\nis quite surprising.\n\n> +\t\tint i;\n> +\t\tfor (i = 0; local_repo_env[i]; i++)\n> +\t\t\tprintf(\"%s\\n\", local_repo_env[i]);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tif (argc > 2 && !strcmp(argv[1], \"--resolve-git-dir\")) {\n\nThis is less bad, but again it might be nice to provide a better error\nif the path argument isn't supplied.\n\n> +\t\tconst char *gitdir = resolve_gitdir(argv[2]);\n> +\t\tif (!gitdir)\n> +\t\t\tdie(\"not a gitdir '%s'\", argv[2]);\n> +\t\tputs(gitdir);\n> +\t\treturn 0;\n> +\t}\n> +\n>  \tif (argc > 1 && !strcmp(\"-h\", argv[1]))\n>  \t\tusage(builtin_rev_parse_usage);\n>  \n> @@ -706,12 +721,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>  \t\t\t\tadd_ref_exclusion(&ref_excludes, arg + 10);\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n> -\t\t\tif (!strcmp(arg, \"--local-env-vars\")) {\n\nWhat about leaving this in and replacing the body of the if statement\nwith:\n\n\tdie(\"--local-env-vars must be the first argument\");\n\n?  I expect this will significantly reduce debugging time if anyone is\nrelying on the current behaviour.\n\n> -\t\t\t\tint i;\n> -\t\t\t\tfor (i = 0; local_repo_env[i]; i++)\n> -\t\t\t\t\tprintf(\"%s\\n\", local_repo_env[i]);\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n>  \t\t\tif (!strcmp(arg, \"--show-toplevel\")) {\n>  \t\t\t\tconst char *work_tree = get_git_work_tree();\n>  \t\t\t\tif (work_tree)\n> @@ -767,16 +776,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>  \t\t\t\tputs(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n> -\t\t\tif (!strcmp(arg, \"--resolve-git-dir\")) {\n> -\t\t\t\tconst char *gitdir = argv[++i];\n> -\t\t\t\tif (!gitdir)\n> -\t\t\t\t\tdie(\"--resolve-git-dir requires an argument\");\n> -\t\t\t\tgitdir = resolve_gitdir(gitdir);\n> -\t\t\t\tif (!gitdir)\n> -\t\t\t\t\tdie(\"not a gitdir '%s'\", argv[i]);\n> -\t\t\t\tputs(gitdir);\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n>  \t\t\tif (!strcmp(arg, \"--is-inside-git-dir\")) {\n>  \t\t\t\tprintf(\"%s\\n\", is_inside_git_dir() ? \"true\"\n>  \t\t\t\t\t\t: \"false\");\n> diff --git a/t/t1515-rev-parse-outside-repo.sh b/t/t1515-rev-parse-outside-repo.sh\n> index ae33093..3ec2971 100755\n> --- a/t/t1515-rev-parse-outside-repo.sh\n> +++ b/t/t1515-rev-parse-outside-repo.sh\n> @@ -27,14 +27,14 @@ test_expect_success 'rev-parse --sq-quote' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> -test_expect_failure 'rev-parse --local-env-vars' '\n> +test_expect_success 'rev-parse --local-env-vars' '\n>  \tgit rev-parse --local-env-vars >actual &&\n>  \t# we do not want to depend on the complete list here,\n>  \t# so just look for something plausible\n>  \tgrep ^GIT_DIR actual\n>  '\n>  \n> -test_expect_failure 'rev-parse --resolve-git-dir' '\n> +test_expect_success 'rev-parse --resolve-git-dir' '\n>  \tgit init --separate-git-dir repo dir &&\n>  \ttest_must_fail git rev-parse --resolve-git-dir . &&\n>  \techo \"$(pwd)/repo\" >expect &&\n> -- \n> 2.7.2.767.g705917e\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"279678","messageId":"CAPig+cSXY0XE7C71EAO8MOpzrtSFOA0fcxu5wxAZ-J9RzjusPg@mail.gmail.com","threadId":"41541","inReplyTo":"20160226232957.GB9552@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] Revert \"rev-parse: remove restrictions on some options\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-28T00:53:02Z","receivedAt":"2016-02-28T00:53:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 26, 2016 at 6:29 PM, Jeff King <peff@peff.net> wrote:\n> This reverts commit 68889b416d5b6a5cf7d280a428281d635fe9b292.\n>\n> That commit bumped some rev-parse options into the main\n> option-parsing loop, which meant that they no longer had to\n> come at the very beginning of the option list. However, that\n> also means that they now came after our call to\n> setup_git_directory(), and will no longer run outside of a\n> git repository.\n>\n> For --local-env-vars, this is semi-questionable. The main\n> use of that option is to get a list of environment variables\n> to clear, and if you are not in a repository, there\n> _probably_ isn't anything to clear. But it places an\n> unnecessary restriction on callers who may be using it\n> preemptively.\n>\n> For --resolve-git-dir, it is almost certainly a regression.\n> That option is about finding a git dir in the first place,\n> so it is reasonably likely to be called from outside an\n> existing repository.\n>\n> The best solution here would be to have a full parsing loop\n> that handles all options, but only calls setup_git_directory\n> as appropriate. Unfortunately, that's a bit complicated to\n> implement. We _have_ to handle each option in the order it\n> appears on the command line. If the caller asked for:\n>\n>   git rev-parse --resolve-git-dir foo/.git --show-toplevel\n>\n> then it must receive the two lines of output in the correct\n\ns/correct/& order/\n\n> to know which is which. But asking for:\n>\n>   git rev-parse --show-toplevel --resolve-git-dir foo/.git\n>\n> is weird; we have to setup_git_directory() for the first\n> option.\n>\n> So any implementation would probably have to either:\n>\n>   - make two passes over the options, first figuring out\n>     whether we need a git-dir, and then actually handling\n>     the options. That's possible, but it's probably not\n>     worth the trouble.\n>\n>   - call setup_git_directory() on the fly when an option\n>     needs it; that requires annotating all of the options,\n>     and there are a considerable number of them.\n>\n> The original patch was not spurred by an actual bug report,\n> but by an observation[1] that was essentially \"eh, this\n> looks unnecessarily restrictive\". It _is_ restrictive, but\n> it turns out to be necessarily so. :)\n>\n> And in practice, it is unlikely anybody was bothered by the\n> restriction. It's not really sane to use --local-env-vars\n> with other options, anyway, as it produces unbounded output\n> (so the caller only know it ends at EOF). It's more\n> plausible for --resolve-git-dir to be used with other\n> options, but still unlikely. It's main use is accessing\n\ns/It's/Its/\n\n> _other_ repositories (e.g., submodules), so making a query\n> on the main repository at the same time isn't very useful.\n>\n> This patch therefore just reverts 68889b416, with a few\n> caveats:\n>\n>   1. Since the original change, --resolve-git-dir learned to\n>      avoid segfaulting on a bogus. We don't know need to\n\ns/bogus/& argument/\n\n>      backport that, because the \"restricted\" form checks\n>      argc.\n>\n>   2. The original patch mentioned that we didn't need to\n>      clean up any documentation, because the restriction\n>      wasn't documented. We can at least fix that by\n>      mentioning the restriction in the manpage.\n>\n>   3. We can now mark our newly-added tests as passing. :)\n>\n> [1] http://article.gmane.org/gmane.comp.version-control.git/230849\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n"},{"id":"279764","messageId":"20160229110156.GA29697@sigill.intra.peff.net","threadId":"41541","inReplyTo":"20160226233449.GA9622@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] Revert \"rev-parse: remove restrictions on some options\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-29T11:01:56Z","receivedAt":"2016-02-29T11:01:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 26, 2016 at 06:34:49PM -0500, Jeff King wrote:\n\n> > So any implementation would probably have to either:\n> > \n> >   - make two passes over the options, first figuring out\n> >     whether we need a git-dir, and then actually handling\n> >     the options. That's possible, but it's probably not\n> >     worth the trouble.\n> > \n> >   - call setup_git_directory() on the fly when an option\n> >     needs it; that requires annotating all of the options,\n> >     and there are a considerable number of them.\n> \n> Having just sent this, of course, it occurs to me that a loop like:\n> \n>    setup_git_directory_gently(&nongit);\n>    for (i = 0; i < argc; i++) {\n>   \tif (!strcmp(argv[i], \"--local-env-vars\"))\n>   \t... and other nongit-ok options ...\n>   \n>   \tif (nongit)\n>   \t\tdie(\"need a repo\");\n>   \n>   \tif (!strcmp(argv[i], \"--git-dir\"))\n>   \t... and other options ...\n>    }\n\nActually, it is even easier than that. Those options don't care whether\nthey are in a repo or not, so we can just wait to call\nsetup_git_directory() in the loop.\n\nIOW, I think my 2/2 should be replaced with this:\n\n-- >8 --\nSubject: [PATCH] rev-parse: let some options run outside repository\n\nOnce upon a time, you could use \"--local-env-vars\" and\n\"--resolve-git-dir\" outside of any git repository, but they\nhad to come first on the command line. Commit 68889b4\n(rev-parse: remove restrictions on some options, 2013-07-21)\nput them into the normal option-parsing loop, fixing the\nlatter. But it inadvertently broke the former, as we call\nsetup_git_directory() before starting that loop.\n\nWe can note that those options don't care even conditionally\nabout whether we are in a git repo. So it's fine if we\nsimply wait to setup the repo until we see an option that\nneeds it.\n\nHowever, there is one special exception we should make:\nhistorically, rev-parse will set up the repository and read\nconfig even if there are _no_ options. Some of the\ntests in t1300 rely on this to check \"git -c $config\"\nparsing. That's not mirroring real-world use, and we could\ntweak the test.  But t0002 uses a bare \"git rev-parse\" to\ncheck \"are we in a git repository?\". It's plausible that\nreal-world scripts are relying on this.\n\nSo let's cover this case specially, and treat an option-less\n\"rev-parse\" as \"see if we're in a repo\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/rev-parse.c               | 50 +++++++++++++++++++++++++--------------\n t/t1515-rev-parse-outside-repo.sh |  4 ++--\n 2 files changed, 34 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex cf8487b..c961b74 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -505,6 +505,7 @@ N_(\"git rev-parse --parseopt [<options>] -- [<args>...]\\n\"\n int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n {\n \tint i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;\n+\tint did_repo_setup = 0;\n \tint has_dashdash = 0;\n \tint output_prefix = 0;\n \tunsigned char sha1[20];\n@@ -528,11 +529,40 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tprefix = setup_git_directory();\n-\tgit_config(git_default_config, NULL);\n+\t/* No options; just report on whether we're in a git repo or not. */\n+\tif (argc == 1) {\n+\t\tsetup_git_directory();\n+\t\tgit_config(git_default_config, NULL);\n+\t\treturn 0;\n+\t}\n+\n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \n+\t\tif (!strcmp(arg, \"--local-env-vars\")) {\n+\t\t\tint i;\n+\t\t\tfor (i = 0; local_repo_env[i]; i++)\n+\t\t\t\tprintf(\"%s\\n\", local_repo_env[i]);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!strcmp(arg, \"--resolve-git-dir\")) {\n+\t\t\tconst char *gitdir = argv[++i];\n+\t\t\tif (!gitdir)\n+\t\t\t\tdie(\"--resolve-git-dir requires an argument\");\n+\t\t\tgitdir = resolve_gitdir(gitdir);\n+\t\t\tif (!gitdir)\n+\t\t\t\tdie(\"not a gitdir '%s'\", argv[i]);\n+\t\t\tputs(gitdir);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/* The rest of the options require a git repository. */\n+\t\tif (!did_repo_setup) {\n+\t\t\tprefix = setup_git_directory();\n+\t\t\tgit_config(git_default_config, NULL);\n+\t\t\tdid_repo_setup = 1;\n+\t\t}\n+\n \t\tif (!strcmp(arg, \"--git-path\")) {\n \t\t\tif (!argv[i + 1])\n \t\t\t\tdie(\"--git-path requires an argument\");\n@@ -706,12 +736,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tadd_ref_exclusion(&ref_excludes, arg + 10);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (!strcmp(arg, \"--local-env-vars\")) {\n-\t\t\t\tint i;\n-\t\t\t\tfor (i = 0; local_repo_env[i]; i++)\n-\t\t\t\t\tprintf(\"%s\\n\", local_repo_env[i]);\n-\t\t\t\tcontinue;\n-\t\t\t}\n \t\t\tif (!strcmp(arg, \"--show-toplevel\")) {\n \t\t\t\tconst char *work_tree = get_git_work_tree();\n \t\t\t\tif (work_tree)\n@@ -767,16 +791,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tputs(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (!strcmp(arg, \"--resolve-git-dir\")) {\n-\t\t\t\tconst char *gitdir = argv[++i];\n-\t\t\t\tif (!gitdir)\n-\t\t\t\t\tdie(\"--resolve-git-dir requires an argument\");\n-\t\t\t\tgitdir = resolve_gitdir(gitdir);\n-\t\t\t\tif (!gitdir)\n-\t\t\t\t\tdie(\"not a gitdir '%s'\", argv[i]);\n-\t\t\t\tputs(gitdir);\n-\t\t\t\tcontinue;\n-\t\t\t}\n \t\t\tif (!strcmp(arg, \"--is-inside-git-dir\")) {\n \t\t\t\tprintf(\"%s\\n\", is_inside_git_dir() ? \"true\"\n \t\t\t\t\t\t: \"false\");\ndiff --git a/t/t1515-rev-parse-outside-repo.sh b/t/t1515-rev-parse-outside-repo.sh\nindex ae33093..3ec2971 100755\n--- a/t/t1515-rev-parse-outside-repo.sh\n+++ b/t/t1515-rev-parse-outside-repo.sh\n@@ -27,14 +27,14 @@ test_expect_success 'rev-parse --sq-quote' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'rev-parse --local-env-vars' '\n+test_expect_success 'rev-parse --local-env-vars' '\n \tgit rev-parse --local-env-vars >actual &&\n \t# we do not want to depend on the complete list here,\n \t# so just look for something plausible\n \tgrep ^GIT_DIR actual\n '\n \n-test_expect_failure 'rev-parse --resolve-git-dir' '\n+test_expect_success 'rev-parse --resolve-git-dir' '\n \tgit init --separate-git-dir repo dir &&\n \ttest_must_fail git rev-parse --resolve-git-dir . &&\n \techo \"$(pwd)/repo\" >expect &&\n-- \n2.8.0.rc0.276.gddf4100\n"},{"id":"279765","messageId":"20160229111123.GA29769@sigill.intra.peff.net","threadId":"41541","inReplyTo":"20160227122511.GR1766@serenity.lan","subject":"Re: [PATCH 2/2] Revert \"rev-parse: remove restrictions on some options\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-29T11:11:23Z","receivedAt":"2016-02-29T11:11:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 27, 2016 at 12:25:11PM +0000, John Keeping wrote:\n\n> On Fri, Feb 26, 2016 at 06:29:57PM -0500, Jeff King wrote:\n> > This reverts commit 68889b416d5b6a5cf7d280a428281d635fe9b292.\n> [snip]\n> > The original patch was not spurred by an actual bug report,\n> > but by an observation[1] that was essentially \"eh, this\n> > looks unnecessarily restrictive\". It _is_ restrictive, but\n> > it turns out to be necessarily so. :)\n> \n> The aim of the original series was to improve the documentation, so I\n> don't think it's unreasonable to consider this a regression and revert\n> the functional change.  Although I think we can improve the behaviour\n> slightly (see below).\n\nThanks for looking this over. I agree that the changes you suggested\nwould be an improvement over what I posted. But I tried out the\nalternate plan to handle the repo-setup more gracefully inside the loop.\nI think that looks much simpler (at the very least, I had to spend a lot\nfewer words trying to justify it in the commit message!).\n\nAnd that makes most of your suggestions obsolete. I'll go through\nthem...\n\n> > +--local-env-vars::\n> > +\tList the GIT_* environment variables that are local to the\n> > +\trepository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).\n> > +\tOnly the names of the variables are listed, not their value,\n> > +\teven if they are set.\n> \n> I think we should add:\n> \n> \tNo other arguments may be supplied.\n\nSo now you can give other arguments. I doubt it's a _good idea_ to do\nso, but you could certainly do:\n\n  git rev-parse --git-dir --local-env-vars\n\nif you wanted to. You can even do the opposite, and I guess it would be\ncorrect as long as you popped the final line off the end as --git-dir.\n\nSo I guess we could restrict it, but I don't think it's an issue in\npractice, and it does technically work.\n\n> > +--resolve-git-dir <path>::\n> > +\tCheck if <path> is a valid repository or a gitfile that\n> > +\tpoints at a valid repository, and print the location of the\n> > +\trepository.  If <path> is a gitfile then the resolved path\n> > +\tto the real repository is printed.\n> \n> Again I think this should say that only the `path` argument may be\n> supplied.\n\nThis one I think is more reasonable to use along with other options. Or\nyou could even pass a sequence of:\n\n  git rev-parse --resolve-git-dir foo --resolve-git-dir bar\n\nAgain, I doubt it's very useful in practice, but it does the obvious\nthing (whereas with my original patch, it silently ignored subsequent\noptions).\n\n> > +\tif (argc == 2 && !strcmp(\"--local-env-vars\", argv[1])) {\n> \n> Maybe:\n> \n> \tif (argc > 1 && !strcmp(\"--local-env-vars\", argv[1])) {\n> \t\tif (argc != 2)\n> \t\t\tdie(\"--local-env-vars must be the only argument\");\n> \n> since the behaviour of:\n> \n> \t$ git rev-parse --local-env-vars --\n> \t--local-env-vars\n> \t--\n> \n> is quite surprising.\n\nThis now does what you'd expect (it's probably not _useful_, but at\nleast it isn't horrifying and surprising, like the v1 behavior).\n\n> > +\tif (argc > 2 && !strcmp(argv[1], \"--resolve-git-dir\")) {\n> \n> This is less bad, but again it might be nice to provide a better error\n> if the path argument isn't supplied.\n\nThis one is OK now, too.\n\n> > @@ -706,12 +721,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> >  \t\t\t\tadd_ref_exclusion(&ref_excludes, arg + 10);\n> >  \t\t\t\tcontinue;\n> >  \t\t\t}\n> > -\t\t\tif (!strcmp(arg, \"--local-env-vars\")) {\n> \n> What about leaving this in and replacing the body of the if statement\n> with:\n> \n> \tdie(\"--local-env-vars must be the first argument\");\n> \n> ?  I expect this will significantly reduce debugging time if anyone is\n> relying on the current behaviour.\n\nThe new version I sent covers this, too (and it does the right thing).\n\n\nThanks for a careful review of the corner cases. Even though my response\nto all of them is \"yeah, but your suggestion is now obsolete\", it makes\nme feel better about the v2 patch to see that it magically clears up all\nof these issues.\n\n-Peff\n"},{"id":"279766","messageId":"20160229111218.GB29769@sigill.intra.peff.net","threadId":"41541","inReplyTo":"CAPig+cSXY0XE7C71EAO8MOpzrtSFOA0fcxu5wxAZ-J9RzjusPg@mail.gmail.com","subject":"Re: [PATCH 2/2] Revert \"rev-parse: remove restrictions on some options\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-29T11:12:18Z","receivedAt":"2016-02-29T11:12:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 27, 2016 at 07:53:02PM -0500, Eric Sunshine wrote:\n\n> > then it must receive the two lines of output in the correct\n> \n> s/correct/& order/\n\nI fixed this and all of the other typos by switching to a patch that\nneeds about one tenth as much explanation. :)\n\nI'm sure it's not possible that I introduced any new ones...\n\n-Peff\n"},{"id":"279792","messageId":"xmqqy4a3cti1.fsf@gitster.mtv.corp.google.com","threadId":"41541","inReplyTo":"20160229110156.GA29697@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] Revert \"rev-parse: remove restrictions on some options\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-29T17:32:22Z","receivedAt":"2016-02-29T17:32:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> IOW, I think my 2/2 should be replaced with this:\n\nThis looks sensible.\n\nDon't we still want the documentation updates from the previous 2/2?\n"},{"id":"279847","messageId":"20160229212943.GA25342@sigill.intra.peff.net","threadId":"41541","inReplyTo":"xmqqy4a3cti1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] Revert \"rev-parse: remove restrictions on some options\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-29T21:29:43Z","receivedAt":"2016-02-29T21:29:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 29, 2016 at 09:32:22AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > IOW, I think my 2/2 should be replaced with this:\n> \n> This looks sensible.\n> \n> Don't we still want the documentation updates from the previous 2/2?\n\nI don't think so. They were primarily about moving those option blocks\nto the \"these options must come first...\" section, which is no longer\ntrue.\n\nIt also added \"you don't have to be in a git repository for these\" to\nthat section, but I think that is less important. We could add that\nindividually to each option, I guess.\n\n-Peff\n"}]}