{"thread":{"id":"45086","subject":"[PATCH] rev-parse --git-path: fix output when running in a subdirectory","startedAt":"2017-02-08T12:18:27Z","lastAt":"2017-02-17T18:25:51Z","messageCount":27,"participants":["Johannes Schindelin","Junio C Hamano","Duy Nguyen","Mike Rappazzo","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"311054","messageId":"50fe3ea3302c40f4c96eaa5a568837e3334f9dc4.1486555851.git.johannes.schindelin@gmx.de","threadId":"45086","inReplyTo":null,"subject":"[PATCH] rev-parse --git-path: fix output when running in a subdirectory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-08T12:17:02Z","receivedAt":"2017-02-08T12:18:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In addition to making git_path() aware of certain file names that need\nto be handled differently e.g. when running in worktrees, the commit\n557bd833bb (git_path(): be aware of file relocation in $GIT_DIR,\n2014-11-30) also snuck in a new option for `git rev-parse`:\n`--git-path`.\n\nOn the face of it, there is no obvious bug in that commit's diff: it\nfaithfully calls git_path() on the argument and prints it out, i.e. `git\nrev-parse --git-path ` has the same precise behavior as\ncalling `git_path(\"\")` in C.\n\nThe problem lies deeper, much deeper. In hindsight (which is always\nunfair), implementing the .git/ directory discovery in\n`setup_git_directory()` by changing the working directory may have\nallowed us to avoid passing around a struct that contains information\nabout the current repository, but it bought us many, many problems.\n\nIn this case, when being called in a subdirectory, `git rev-parse`\nchanges the working directory to the top-level directory before calling\n`git_path()`. In the new working directory, the result is correct. But\nin the working directory of the calling script, it is incorrect.\n\nExample: when calling `git rev-parse --git-path HEAD` in, say, the\nDocumentation/ subdirectory of Git's own source code, the string\n`.git/HEAD` is printed.\n\nSide note: that bug is hidden when running in a subdirectory of a\nworktree that was added by the `git worktree` command: in that case, the\n(correct) absolute path of the `HEAD` file is printed.\n\nIn the interest of time, this patch does not go the \"correct\" route to\nintroduce a struct with repository information (and removing global\nstate in the process), instead this patch chooses to detect when the\ncommand was called in a subdirectory and forces the result to be an\nabsolute path.\n\nSigned-off-by: Johannes Schindelin \n---\nPublished-As: https://github.com/dscho/git/releases/tag/git-path-in-subdir-v1\nFetch-It-Via: git fetch https://github.com/dscho/git git-path-in-subdir-v1\n\n builtin/rev-parse.c | 6 +++++-\n t/t0060-path-utils.sh | 2 ++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex ff13e59e1d..f9d5762bf2 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -597,9 +597,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t}\n \n \t\tif (!strcmp(arg, \"--git-path\")) {\n+\t\t\tconst char *path;\n \t\t\tif (!argv[i + 1])\n \t\t\t\tdie(\"--git-path requires an argument\");\n-\t\t\tputs(git_path(\"%s\", argv[i + 1]));\n+\t\t\tpath = git_path(\"%s\", argv[i + 1]);\n+\t\t\tif (prefix && !is_absolute_path(path))\n+\t\t\t\tpath = real_path(path);\n+\t\t\tputs(path);\n \t\t\ti++;\n \t\t\tcontinue;\n \t\t}\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex 444b5a4df8..790584fcc5 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -271,6 +271,8 @@ relative_path \"\"\t\t\"\"\t./\n relative_path \"\"\t\t\"\"\t./\n relative_path \"\"\t\t/foo/a/b\t./\n \n+test_git_path \"mkdir sub && cd sub && test_when_finished cd .. &&\" \\\n+\tfoo \"$(pwd)/.git/foo\"\n test_git_path A=B info/grafts .git/info/grafts\n test_git_path GIT_GRAFT_FILE=foo info/grafts foo\n test_git_path GIT_GRAFT_FILE=foo info/////grafts foo\n\nbase-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8\n-- \n2.11.1.windows.1\n"},{"id":"311077","messageId":"xmqqh944wmcs.fsf@gitster.mtv.corp.google.com","threadId":"45086","inReplyTo":"50fe3ea3302c40f4c96eaa5a568837e3334f9dc4.1486555851.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-08T18:47:47Z","receivedAt":"2017-02-08T18:48:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The problem lies deeper, much deeper...\n> ... but it bought us many, many problems.\n\nI think you are making a mountain out of molehill here. \nThis looks like as an opposite problem of a bug that forgets to\nprepend the prefix to relative pathnames given by the end user.\n\nI do agree that some calling scripts may find it more convenient if\nthe output were relative to their current directory, and in that\nsense, this is worth addressing.\n\nHowever.\n\nHow long has \"rev-parse --git-path\" been around? Had scripts in the\nwild chance to learn to live with the \"output is relative to the top\nof the working tree\" reality? I think the answers are \"since 2.5\" and\n\"yes\".\n\nI do not think we can make this unconditionally without breaking\nusers. We instead need something like a new \"--git-path-relative\"\noption, similar in the spirit that output from \"git diff\" can be\nmade relative to the current directory with the \"--relative\" option.\n\nAssuming that we are discussing the new behaviour that is\nconditionally triggered, let's see the implementation.\n\n> -\t\t\tputs(git_path(\"%s\", argv[i + 1]));\n> +\t\t\tpath = git_path(\"%s\", argv[i + 1]);\n> +\t\t\tif (prefix && !is_absolute_path(path))\n> +\t\t\t\tpath = real_path(path);\n> +\t\t\tputs(path);\n\nDuy, want to help me out here? I am wondering if using a logic\nsimilar to what is used by \"cd t && git grep -e foo :/\" to emit\npaths as \"../Documentation/CodingGuidelines\" as relative to the\ncurrent working directory is more appropriate than forcing the\nabsolute path output here (and if so, it may be preferrable to use\nthe relative_path() helper to do so), or the paths to files in\n$GIT_DIR are conceptually different enough from paths to files in\nthe working tree and it will be more robust to have the output as an\nabsolute path.\n\nI am leaning toward the latter (i.e. the above use of real_path() is\nsimple and good), but I haven't thought things through and since we\nhave an area expert here in the thread...\n"},{"id":"311155","messageId":"CACsJy8CigsWjAq5cmJ=cbBmj=DdJtHdMKxmoifftuz9+9kqJiQ@mail.gmail.com","threadId":"45086","inReplyTo":"50fe3ea3302c40f4c96eaa5a568837e3334f9dc4.1486555851.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-02-09T09:48:20Z","receivedAt":"2017-02-09T09:58:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 8, 2017 at 7:17 PM, Johannes Schindelin\n wrote:\n> In addition to making git_path() aware of certain file names that need\n> to be handled differently e.g. when running in worktrees, the commit\n> 557bd833bb (git_path(): be aware of file relocation in $GIT_DIR,\n> 2014-11-30) also snuck in a new option for `git rev-parse`:\n> `--git-path`.\n>\n> On the face of it, there is no obvious bug in that commit's diff: it\n> faithfully calls git_path() on the argument and prints it out, i.e. `git\n> rev-parse --git-path ` has the same precise behavior as\n> calling `git_path(\"\")` in C.\n>\n> The problem lies deeper, much deeper. In hindsight (which is always\n> unfair), implementing the .git/ directory discovery in\n> `setup_git_directory()` by changing the working directory may have\n> allowed us to avoid passing around a struct that contains information\n> about the current repository, but it bought us many, many problems.\n\nRelevant thread in the past [1] which fixes both --git-path and\n--git-common-dir. I think the author dropped it somehow (or forgot\nabout it, I know I did). Sorry can't comment on that thread, or this\npatch, yet.\n\n[1] http://public-inbox.org/git/1464261556-89722-1-git-send-email-rappazzo@gmail.com/\n-- \nDuy\n"},{"id":"311178","messageId":"CANoM8SWv+KD92T263gS0Uxxi2ekNQdo0aNGx3AmweVasXk3GbA@mail.gmail.com","threadId":"45086","inReplyTo":"CACsJy8CigsWjAq5cmJ=cbBmj=DdJtHdMKxmoifftuz9+9kqJiQ@mail.gmail.com","subject":"Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2017-02-09T13:46:22Z","receivedAt":"2017-02-09T14:18:50Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Thu, Feb 9, 2017 at 4:48 AM, Duy Nguyen wrote:\n> On Wed, Feb 8, 2017 at 7:17 PM, Johannes Schindelin\n> wrote:\n>> In addition to making git_path() aware of certain file names that need\n>> to be handled differently e.g. when running in worktrees, the commit\n>> 557bd833bb (git_path(): be aware of file relocation in $GIT_DIR,\n>> 2014-11-30) also snuck in a new option for `git rev-parse`:\n>> `--git-path`.\n>>\n>> On the face of it, there is no obvious bug in that commit's diff: it\n>> faithfully calls git_path() on the argument and prints it out, i.e. `git\n>> rev-parse --git-path ` has the same precise behavior as\n>> calling `git_path(\"\")` in C.\n>>\n>> The problem lies deeper, much deeper. In hindsight (which is always\n>> unfair), implementing the .git/ directory discovery in\n>> `setup_git_directory()` by changing the working directory may have\n>> allowed us to avoid passing around a struct that contains information\n>> about the current repository, but it bought us many, many problems.\n>\n> Relevant thread in the past [1] which fixes both --git-path and\n> --git-common-dir. I think the author dropped it somehow (or forgot\n> about it, I know I did). Sorry can't comment on that thread, or this\n> patch, yet.\n\nI didn't exactly forget it (I have it sitting in a branch), I wasn't\nsure what else was needed (from a v5 I guess), so it has stagnated.\n\nThere was also another patch [1] at the time done by SZEDER Gábor\ntrying to speed up the completion scripts by adding `git rev-parse\n--absolute-git-dir` option to deal with this case as well.\n\n>\n> [1] http://public-inbox.org/git/1464261556-89722-1-git-send-email-rappazzo@gmail.com/\n> --\n> Duy\n\n[1] http://public-inbox.org/git/20170203024829.8071-16-szeder.dev@gmail.com/\n"},{"id":"311212","messageId":"alpine.DEB.2.20.1702092208220.3496@virtualbox","threadId":"45086","inReplyTo":"CANoM8SWv+KD92T263gS0Uxxi2ekNQdo0aNGx3AmweVasXk3GbA@mail.gmail.com","subject":"Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-09T21:11:49Z","receivedAt":"2017-02-09T21:18:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Mike,\n\nOn Thu, 9 Feb 2017, Mike Rappazzo wrote:\n\n> On Thu, Feb 9, 2017 at 4:48 AM, Duy Nguyen wrote:\n>\n> > Relevant thread in the past [1] which fixes both --git-path and\n> > --git-common-dir. I think the author dropped it somehow (or forgot\n> > about it, I know I did). Sorry can't comment on that thread, or this\n> > patch, yet.\n> \n> I didn't exactly forget it (I have it sitting in a branch), I wasn't\n> sure what else was needed (from a v5 I guess), so it has stagnated.\n> \n> There was also another patch [1] at the time done by SZEDER Gábor trying\n> to speed up the completion scripts by adding `git rev-parse\n> --absolute-git-dir` option to deal with this case as well.\n> \n> > [1] http://public-inbox.org/git/1464261556-89722-1-git-send-email-rappazzo@gmail.com/\n> \n> [1] http://public-inbox.org/git/20170203024829.8071-16-szeder.dev@gmail.com/\n\nAh, so I was not the only person reporting this bug, but I am seemingly\nhaving as much luck getting a fix in.\n\nI had a quick look at your v4:\nhttp://public-inbox.org/git/1464261556-89722-3-git-send-email-rappazzo@gmail.com/\n\nIt seems you replaced the git_path() by a combination of git_dir() and\nrelative_path(), but that would break the use case where git_path()\nhandles certain arguments specially, e.g. \"objects\" which knows that the\n.git/objects/ path can be overridden via the environment.\n\nI tried very hard to keep that working in my patch, essentially by\nemulating what git_path() does already when being called in a worktree's\nsubdirectory: make the path absolute.\n\nCiao,\nJohannes"},{"id":"311215","messageId":"xmqqshnnnj6q.fsf@gitster.mtv.corp.google.com","threadId":"45086","inReplyTo":"CACsJy8CigsWjAq5cmJ=cbBmj=DdJtHdMKxmoifftuz9+9kqJiQ@mail.gmail.com","subject":"Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-09T21:33:17Z","receivedAt":"2017-02-09T21:35:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> Relevant thread in the past [1] which fixes both --git-path and\n> --git-common-dir. I think the author dropped it somehow (or forgot\n> about it, I know I did). Sorry can't comment on that thread, or this\n> patch, yet.\n>\n> [1] http://public-inbox.org/git/1464261556-89722-1-git-send-email-rappazzo@gmail.com/\n\nThanks for a pointer. I see Mike responded to this message (I\nhaven't had a chance to read and think about it yet), so I trust\nthat you three can figure out if these are the same issues and what\nthe final solution in the longer term should be. \n\nI have no strong opinion for or against a \"longer term\" solution\nthat makes \"rev-parse --git-path\" behave differently from how it\nbehaves today, but I am not yet convinced that we can reach that\nlonger term goal without a transition period, as I suspect there are\nexisting users that know and came to expect how it behaves, based on\nits today's behaviour. Other than that I do not have suggestion on\nthis topic at the moment.\n\nThanks.\n"},{"id":"311217","messageId":"alpine.DEB.2.20.1702092201080.3496@virtualbox","threadId":"45086","inReplyTo":"xmqqh944wmcs.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-09T21:05:24Z","receivedAt":"2017-02-09T21:37:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 8 Feb 2017, Junio C Hamano wrote:\n\n> How long has \"rev-parse --git-path\" been around? Had scripts in the\n> wild chance to learn to live with the \"output is relative to the top of\n> the working tree\" reality? I think the answers are \"since 2.5\" and\n> \"yes\".\n\nCorrect. And the third question is: How did the scripts work around this\nfeature?\n\nThe answer is obvious: by switching back to `$(git rev-parse\n--git-dir)/filename`.\n\nThis is literally on what I spent the better part of Wednesday.\n\nThere is just no way you can \"fix\" this otherwise. As an occasional shell\nscripter, you may be tempted to use `$(git rev-parse --show-cdup)$(git\nrev-parse --git-path filename)`, but of course that breaks in worktrees\nand if you do not use worktrees you would not even know that your\nworkaround introduced another bug.\n\nThe current handling of --git-path is just plain wrong. The fact that I am\nthe first person to report this here merely shows how much it is used in\nthe wild.\n\nCiao,\nJohannes\n"},{"id":"311219","messageId":"xmqqo9ybnie0.fsf@gitster.mtv.corp.google.com","threadId":"45086","inReplyTo":"alpine.DEB.2.20.1702092201080.3496@virtualbox","subject":"Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-09T21:50:31Z","receivedAt":"2017-02-09T21:51:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Wed, 8 Feb 2017, Junio C Hamano wrote:\n>\n>> How long has \"rev-parse --git-path\" been around? Had scripts in the\n>> wild chance to learn to live with the \"output is relative to the top of\n>> the working tree\" reality? I think the answers are \"since 2.5\" and\n>> \"yes\".\n>\n> Correct. And the third question is: How did the scripts work around this\n> feature?\n>\n> The answer is obvious: by switching back to `$(git rev-parse\n> --git-dir)/filename`.\n>\n> This is literally on what I spent the better part of Wednesday.\n>\n> There is just no way you can \"fix\" this otherwise. As an occasional shell\n> scripter, you may be tempted to use `$(git rev-parse --show-cdup)$(git\n> rev-parse --git-path filename)`, but of course that breaks in worktrees\n> and if you do not use worktrees you would not even know that your\n> workaround introduced another bug.\n\nIn case it is not clear, I understand all of the above. \n\nI was just worried about the people who do *NOT* use worktrees and\ndid the obvious \"concatenate --cdup with --git-path\" and thought\ntheir script were working happily and well. By prepending the path\nto the (real) location of the .git in the updated --git-path output\nourselves, they will complain, our update broke their script.\n\nIf we introduced the fix gently, by (1) warn when \"--git-path\" is\nused but give the current output anyway, while adding the \"fixed\"\none as another new option, and then (2) remove \"--git-path\" after\nseveral releases, they will not have to complain, even though they\nwill see warning until they stop using \"--git-path\".\n\nThere may be gentler alternative ways to transition, and I do not\nworry about the specifics of them too much. I just think we cannot\ndo this in a single step without harming existing users.\n\n\n"},{"id":"311226","messageId":"alpine.DEB.2.20.1702092304250.3496@virtualbox","threadId":"45086","inReplyTo":"xmqqshnnnj6q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-09T22:11:14Z","receivedAt":"2017-02-09T22:18:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 9 Feb 2017, Junio C Hamano wrote:\n\n> Duy Nguyen writes:\n> \n> > Relevant thread in the past [1] which fixes both --git-path and\n> > --git-common-dir. I think the author dropped it somehow (or forgot\n> > about it, I know I did). Sorry can't comment on that thread, or this\n> > patch, yet.\n> >\n> > [1] http://public-inbox.org/git/1464261556-89722-1-git-send-email-rappazzo@gmail.com/\n> \n> Thanks for a pointer. I see Mike responded to this message (I\n> haven't had a chance to read and think about it yet), so I trust\n> that you three can figure out if these are the same issues and what\n> the final solution in the longer term should be. \n> \n> I have no strong opinion for or against a \"longer term\" solution\n> that makes \"rev-parse --git-path\" behave differently from how it\n> behaves today, but I am not yet convinced that we can reach that\n> longer term goal without a transition period, as I suspect there are\n> existing users that know and came to expect how it behaves, based on\n> its today's behaviour. Other than that I do not have suggestion on\n> this topic at the moment.\n\nGiven that\n\n- the output is incorrect, not some output that could maybe be improved,\n\n- warnings in a script execution are most likely to be missed,\n\n- --git-path gives incorrect output in subdirectories, except inside\n worktrees, therefore scripts relying on the current behavior are highly\n likely to misbehave in worktrees anyway,\n\n- leaving this bug unfixed even when we know about it for 3 major releases\n reflects really badly on Git as a project, and\n\n- the longer we wait to fix this bug, the more developers will simply stay\n away from --git-path (of course, only *after* they were bitten by the\n bug, like I was),\n\nit should be safe to assume that a transitional period is more likely to\ndo more harm to our users than bring benefit.\n\nCiao,\nJohannes\n"},{"id":"311235","messageId":"xmqqwpczm0vj.fsf@gitster.mtv.corp.google.com","threadId":"45086","inReplyTo":"alpine.DEB.2.20.1702092304250.3496@virtualbox","subject":"Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-09T22:54:08Z","receivedAt":"2017-02-09T23:25:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> I have no strong opinion for or against a \"longer term\" solution\n>> that makes \"rev-parse --git-path\" behave differently from how it\n>> behaves today, but I am not yet convinced that we can reach that\n>> longer term goal without a transition period, as I suspect there are\n>> existing users that know and came to expect how it behaves, based on\n>> its today's behaviour. Other than that I do not have suggestion on\n>> this topic at the moment.\n\nI think I was simply being silly (not merely \"overcautious\", but\njust \"silly\") here.\n\nThere is no reason for people to use \"--git-path\" if they are not\npreparing to work with secondary worktrees, because the whole point\nof the feature is so that cases where \"$(rev-parse --git-dir)/path\"\ndoes a wrong thing (e.g. end up referring to the main worktree thing\nwhen you need to refer to your own, or vice versa).\n\n> Given that\n> ...\n> it should be safe to assume that a transitional period is more likely to\n> do more harm to our users than bring benefit.\n\nIn short, \"--git-path as currently exposed to the end-users is\nutterly broken and cannot have been used for anything sensible\". If\nthat is the case, let's just change that with an entry in the\nrelease notes that states so (iow, there is no need for even a\nbackward compatibility notice, we just have an entry that says \"this\nwas totally broken in such and such way, and now it is fixed to\nbehave this way\").\n\nThat leaves what the right single-step behaviour change should be.\nAs I recall Duy said something about --common-dir and other things\nMike's earlier change also covered, I'd prefer to leave it to three\nof you to figure out what the final patch should be.\n\nThanks.\n"},{"id":"311245","messageId":"CANoM8SV7oJ6YmKM-n63620EkODxD562BZnLZB6OvX8O6BmDT1A@mail.gmail.com","threadId":"45086","inReplyTo":"xmqqwpczm0vj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2017-02-10T03:52:36Z","receivedAt":"2017-02-10T04:00:24Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Thu, Feb 9, 2017 at 5:54 PM, Junio C Hamano wrote:\n>\n> That leaves what the right single-step behaviour change should be.\n> As I recall Duy said something about --common-dir and other things\n> Mike's earlier change also covered, I'd prefer to leave it to three\n> of you to figure out what the final patch should be.\n>\n\nThe tests which I covered in my previous patch [1] addressed the\nplaces where we identified similar problems. We should try to include\nsome form of those tests. As far as implementation goes in rev-parse,\nthe version in this thread is probably better that what I had, but it\nwould need to also be applied to --git-common-dir and\n--shared-index-path.\n\n\n[1] http://public-inbox.org/git/1464261556-89722-2-git-send-email-rappazzo@gmail.com/\n"},{"id":"311246","messageId":"20170210042115.d4kmdf4obxqwobv4@sigill.intra.peff.net","threadId":"45086","inReplyTo":"xmqqo9ybnie0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-10T04:21:15Z","receivedAt":"2017-02-10T04:42:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 09, 2017 at 01:50:31PM -0800, Junio C Hamano wrote:\n\n> > There is just no way you can \"fix\" this otherwise. As an occasional shell\n> > scripter, you may be tempted to use `$(git rev-parse --show-cdup)$(git\n> > rev-parse --git-path filename)`, but of course that breaks in worktrees\n> > and if you do not use worktrees you would not even know that your\n> > workaround introduced another bug.\n> \n> In case it is not clear, I understand all of the above. \n> \n> I was just worried about the people who do *NOT* use worktrees and\n> did the obvious \"concatenate --cdup with --git-path\" and thought\n> their script were working happily and well. By prepending the path\n> to the (real) location of the .git in the updated --git-path output\n> ourselves, they will complain, our update broke their script.\n\nThat concatenating approach is broken in other ways, too. For example,\nif you have $GIT_DIR set to an absolute path, then --git-path will use\nthat. I don't think we have ever promised that the output of --git-path\n(or --git-dir) would ever be absolute or relative (in fact, the\n--git-dir documentation implies that you may get either).\n\nSo yes, there could be somebody who is doing this concatenation\nworkaround, but only ever calls their script in a certain way that never\ntriggers the absolute-path variant. They're happy now, and may not be\nafter Dscho's patch. But I really think they are relying on a bogus\nassumption, and their scripts are already buggy.\n\n-Peff\n"},{"id":"311268","messageId":"cover.1486740772.git.johannes.schindelin@gmx.de","threadId":"45086","inReplyTo":"50fe3ea3302c40f4c96eaa5a568837e3334f9dc4.1486555851.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 0/2] Fix bugs in rev-parse's output when run in a subdirectory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-10T15:33:31Z","receivedAt":"2017-02-10T15:42:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The bug that bit me (hard!) and that triggered not only a long series of\ncurses but also my writing a patch and sending it to the list was that\n`git rev-parse --git-path HEAD` would give *incorrect* output when run\nin a subdirectory of a regular checkout, but *correct* output when run\nin a subdirectory of an associated *worktree*.\n\nI had tested the script in question quite a bit, but in a worktree. And\nin production, it quietly did exactly the wrong thing.\n\nRelative to v1 of the then-single patch, this changed:\n\n- the patch now covers also --git-common-dir and --shared-index-path\n\n- the output is now a relative path when git_dir is relative\n\n- I plucked the tests from Mike's patch series and dropped my ad-hoc\n test from t0060.\n\n- While at it, I fixed Mike's test that assumed that the objects/\n directory lives in .git/worktrees//objects/.\n\n\nJohannes Schindelin (1):\n rev-parse: fix several options when running in a subdirectory\n\nMichael Rappazzo (1):\n rev-parse tests: add tests executed from a subdirectory\n\n builtin/rev-parse.c | 15 +++++++++++----\n t/t1500-rev-parse.sh | 28 ++++++++++++++++++++++++++++\n t/t1700-split-index.sh | 17 +++++++++++++++++\n t/t2027-worktree-list.sh | 10 +++++++++-\n 4 files changed, 65 insertions(+), 5 deletions(-)\n\n\nbase-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8\nPublished-As: https://github.com/dscho/git/releases/tag/git-path-in-subdir-v2\nFetch-It-Via: git fetch https://github.com/dscho/git git-path-in-subdir-v2\n\nInterdiff vs v1:\n\n diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n index f9d5762bf2b..84af2802f6f 100644\n --- a/builtin/rev-parse.c\n +++ b/builtin/rev-parse.c\n @@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \tunsigned int flags = 0;\n \tconst char *name = NULL;\n \tstruct object_context unused;\n +\tstruct strbuf buf = STRBUF_INIT;\n \n \tif (argc > 1 && !strcmp(\"--parseopt\", argv[1]))\n \t\treturn cmd_parseopt(argc - 1, argv + 1, prefix);\n @@ -597,13 +598,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t}\n \n \t\tif (!strcmp(arg, \"--git-path\")) {\n -\t\t\tconst char *path;\n \t\t\tif (!argv[i + 1])\n \t\t\t\tdie(\"--git-path requires an argument\");\n -\t\t\tpath = git_path(\"%s\", argv[i + 1]);\n -\t\t\tif (prefix && !is_absolute_path(path))\n -\t\t\t\tpath = real_path(path);\n -\t\t\tputs(path);\n +\t\t\tstrbuf_reset(&buf);\n +\t\t\tputs(relative_path(git_path(\"%s\", argv[i + 1]),\n +\t\t\t\t\t prefix, &buf));\n \t\t\ti++;\n \t\t\tcontinue;\n \t\t}\n @@ -825,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--git-common-dir\")) {\n -\t\t\t\tconst char *pfx = prefix ? prefix : \"\";\n -\t\t\t\tputs(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));\n +\t\t\t\tstrbuf_reset(&buf);\n +\t\t\t\tputs(relative_path(get_git_common_dir(),\n +\t\t\t\t\t\t prefix, &buf));\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--is-inside-git-dir\")) {\n @@ -849,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tdie(_(\"Could not read the index\"));\n \t\t\t\tif (the_index.split_index) {\n \t\t\t\t\tconst unsigned char *sha1 = the_index.split_index->base_sha1;\n -\t\t\t\t\tputs(git_path(\"sharedindex.%s\", sha1_to_hex(sha1)));\n +\t\t\t\t\tconst char *path = git_path(\"sharedindex.%s\", sha1_to_hex(sha1));\n +\t\t\t\t\tstrbuf_reset(&buf);\n +\t\t\t\t\tputs(relative_path(path, prefix, &buf));\n \t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t}\n @@ -910,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\tdie_no_single_rev(quiet);\n \t} else\n \t\tshow_default();\n +\tstrbuf_release(&buf);\n \treturn 0;\n }\n diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\n index 790584fcc54..444b5a4df80 100755\n --- a/t/t0060-path-utils.sh\n +++ b/t/t0060-path-utils.sh\n @@ -271,8 +271,6 @@ relative_path \"\"\t\t\"\"\t./\n relative_path \"\"\t\t\"\"\t./\n relative_path \"\"\t\t/foo/a/b\t./\n \n -test_git_path \"mkdir sub && cd sub && test_when_finished cd .. &&\" \\\n -\tfoo \"$(pwd)/.git/foo\"\n test_git_path A=B info/grafts .git/info/grafts\n test_git_path GIT_GRAFT_FILE=foo info/grafts foo\n test_git_path GIT_GRAFT_FILE=foo info/////grafts foo\n diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\n index 038e24c4014..d74f09ad93e 100755\n --- a/t/t1500-rev-parse.sh\n +++ b/t/t1500-rev-parse.sh\n @@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = tru\n \n test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''\n \n +test_expect_success 'git-common-dir from worktree root' '\n +\techo .git >expect &&\n +\tgit rev-parse --git-common-dir >actual &&\n +\ttest_cmp expect actual\n +'\n +\n +test_expect_success 'git-common-dir inside sub-dir' '\n +\tmkdir -p path/to/child &&\n +\ttest_when_finished \"rm -rf path\" &&\n +\techo \"$(git -C path/to/child rev-parse --show-cdup).git\" >expect &&\n +\tgit -C path/to/child rev-parse --git-common-dir >actual &&\n +\ttest_cmp expect actual\n +'\n +\n +test_expect_success 'git-path from worktree root' '\n +\techo .git/objects >expect &&\n +\tgit rev-parse --git-path objects >actual &&\n +\ttest_cmp expect actual\n +'\n +\n +test_expect_success 'git-path inside sub-dir' '\n +\tmkdir -p path/to/child &&\n +\ttest_when_finished \"rm -rf path\" &&\n +\techo \"$(git -C path/to/child rev-parse --show-cdup).git/objects\" >expect &&\n +\tgit -C path/to/child rev-parse --git-path objects >actual &&\n +\ttest_cmp expect actual\n +'\n +\n test_done\n diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n index 292a0720fcc..446ff34f966 100755\n --- a/t/t1700-split-index.sh\n +++ b/t/t1700-split-index.sh\n @@ -200,4 +200,21 @@ EOF\n \ttest_cmp expect actual\n '\n \n +test_expect_success 'rev-parse --shared-index-path' '\n +\trm -rf .git &&\n +\ttest_create_repo . &&\n +\tgit update-index --split-index &&\n +\tls -t .git/sharedindex* | tail -n 1 >expect &&\n +\tgit rev-parse --shared-index-path >actual &&\n +\ttest_cmp expect actual &&\n +\tmkdir work &&\n +\ttest_when_finished \"rm -rf work\" &&\n +\t(\n +\t\tcd work &&\n +\t\tls -t ../.git/sharedindex* | tail -n 1 >expect &&\n +\t\tgit rev-parse --shared-index-path >actual &&\n +\t\ttest_cmp expect actual\n +\t)\n +'\n +\n test_done\n diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh\n index 465eeeacd3d..848da5f3684 100755\n --- a/t/t2027-worktree-list.sh\n +++ b/t/t2027-worktree-list.sh\n @@ -14,10 +14,18 @@ test_expect_success 'rev-parse --git-common-dir on main worktree' '\n \ttest_cmp expected actual &&\n \tmkdir sub &&\n \tgit -C sub rev-parse --git-common-dir >actual2 &&\n -\techo sub/.git >expected2 &&\n +\techo ../.git >expected2 &&\n \ttest_cmp expected2 actual2\n '\n \n +test_expect_success 'rev-parse --git-path objects linked worktree' '\n +\techo \"$(git rev-parse --show-toplevel)/.git/objects\" >expect &&\n +\ttest_when_finished \"rm -rf linked-tree && git worktree prune\" &&\n +\tgit worktree add --detach linked-tree master &&\n +\tgit -C linked-tree rev-parse --git-path objects >actual &&\n +\ttest_cmp expect actual\n +'\n +\n test_expect_success '\"list\" all worktrees from main' '\n \techo \"$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]\" >expect &&\n \ttest_when_finished \"rm -rf here && git worktree prune\" &&\n\n-- \n2.11.1.windows.1\n\n"},{"id":"311269","messageId":"9242ee9717dcec95351b356070102f198eeb2537.1486740772.git.johannes.schindelin@gmx.de","threadId":"45086","inReplyTo":"cover.1486740772.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 2/2] rev-parse: fix several options when running in a subdirectory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-10T15:33:46Z","receivedAt":"2017-02-10T15:42:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In addition to making git_path() aware of certain file names that need\nto be handled differently e.g. when running in worktrees, the commit\n557bd833bb (git_path(): be aware of file relocation in $GIT_DIR,\n2014-11-30) also snuck in a new option for `git rev-parse`:\n`--git-path`.\n\nOn the face of it, there is no obvious bug in that commit's diff: it\nfaithfully calls git_path() on the argument and prints it out, i.e. `git\nrev-parse --git-path ` has the same precise behavior as\ncalling `git_path(\"\")` in C.\n\nThe problem lies deeper, much deeper. In hindsight (which is always\nunfair), implementing the .git/ directory discovery in\n`setup_git_directory()` by changing the working directory may have\nallowed us to avoid passing around a struct that contains information\nabout the current repository, but it bought us many, many problems.\n\nIn this case, when being called in a subdirectory, `git rev-parse`\nchanges the working directory to the top-level directory before calling\n`git_path()`. In the new working directory, the result is correct. But\nin the working directory of the calling script, it is incorrect.\n\nExample: when calling `git rev-parse --git-path HEAD` in, say, the\nDocumentation/ subdirectory of Git's own source code, the string\n`.git/HEAD` is printed.\n\nSide note: that bug is hidden when running in a subdirectory of a\nworktree that was added by the `git worktree` command: in that case, the\n(correct) absolute path of the `HEAD` file is printed.\n\nIn the interest of time, this patch does not go the \"correct\" route to\nintroduce a struct with repository information (and removing global\nstate in the process), instead this patch chooses to detect when the\ncommand was called in a subdirectory and forces the result to be an\nabsolute path.\n\nWhile at it, we are also fixing the output of --git-common-dir and\n--shared-index-path.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rev-parse.c | 15 +++++++++++----\n t/t1500-rev-parse.sh | 4 ++--\n t/t1700-split-index.sh | 2 +-\n t/t2027-worktree-list.sh | 4 ++--\n 4 files changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex ff13e59e1db..84af2802f6f 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \tunsigned int flags = 0;\n \tconst char *name = NULL;\n \tstruct object_context unused;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n \tif (argc > 1 && !strcmp(\"--parseopt\", argv[1]))\n \t\treturn cmd_parseopt(argc - 1, argv + 1, prefix);\n@@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\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-\t\t\tputs(git_path(\"%s\", argv[i + 1]));\n+\t\t\tstrbuf_reset(&buf);\n+\t\t\tputs(relative_path(git_path(\"%s\", argv[i + 1]),\n+\t\t\t\t\t prefix, &buf));\n \t\t\ti++;\n \t\t\tcontinue;\n \t\t}\n@@ -821,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--git-common-dir\")) {\n-\t\t\t\tconst char *pfx = prefix ? prefix : \"\";\n-\t\t\t\tputs(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));\n+\t\t\t\tstrbuf_reset(&buf);\n+\t\t\t\tputs(relative_path(get_git_common_dir(),\n+\t\t\t\t\t\t prefix, &buf));\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--is-inside-git-dir\")) {\n@@ -845,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tdie(_(\"Could not read the index\"));\n \t\t\t\tif (the_index.split_index) {\n \t\t\t\t\tconst unsigned char *sha1 = the_index.split_index->base_sha1;\n-\t\t\t\t\tputs(git_path(\"sharedindex.%s\", sha1_to_hex(sha1)));\n+\t\t\t\t\tconst char *path = git_path(\"sharedindex.%s\", sha1_to_hex(sha1));\n+\t\t\t\t\tstrbuf_reset(&buf);\n+\t\t\t\t\tputs(relative_path(path, prefix, &buf));\n \t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t}\n@@ -906,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\tdie_no_single_rev(quiet);\n \t} else\n \t\tshow_default();\n+\tstrbuf_release(&buf);\n \treturn 0;\n }\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex f39f783f2db..d74f09ad93e 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -93,7 +93,7 @@ test_expect_success 'git-common-dir from worktree root' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'git-common-dir inside sub-dir' '\n+test_expect_success 'git-common-dir inside sub-dir' '\n \tmkdir -p path/to/child &&\n \ttest_when_finished \"rm -rf path\" &&\n \techo \"$(git -C path/to/child rev-parse --show-cdup).git\" >expect &&\n@@ -107,7 +107,7 @@ test_expect_success 'git-path from worktree root' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'git-path inside sub-dir' '\n+test_expect_success 'git-path inside sub-dir' '\n \tmkdir -p path/to/child &&\n \ttest_when_finished \"rm -rf path\" &&\n \techo \"$(git -C path/to/child rev-parse --show-cdup).git/objects\" >expect &&\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 1d6e27a09d8..446ff34f966 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -200,7 +200,7 @@ EOF\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'rev-parse --shared-index-path' '\n+test_expect_success 'rev-parse --shared-index-path' '\n \trm -rf .git &&\n \ttest_create_repo . &&\n \tgit update-index --split-index &&\ndiff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh\nindex c1a072348e7..848da5f3684 100755\n--- a/t/t2027-worktree-list.sh\n+++ b/t/t2027-worktree-list.sh\n@@ -8,7 +8,7 @@ test_expect_success 'setup' '\n \ttest_commit init\n '\n \n-test_expect_failure 'rev-parse --git-common-dir on main worktree' '\n+test_expect_success 'rev-parse --git-common-dir on main worktree' '\n \tgit rev-parse --git-common-dir >actual &&\n \techo .git >expected &&\n \ttest_cmp expected actual &&\n@@ -18,7 +18,7 @@ test_expect_failure 'rev-parse --git-common-dir on main worktree' '\n \ttest_cmp expected2 actual2\n '\n \n-test_expect_failure 'rev-parse --git-path objects linked worktree' '\n+test_expect_success 'rev-parse --git-path objects linked worktree' '\n \techo \"$(git rev-parse --show-toplevel)/.git/objects\" >expect &&\n \ttest_when_finished \"rm -rf linked-tree && git worktree prune\" &&\n \tgit worktree add --detach linked-tree master &&\n-- \n2.11.1.windows.1\n"},{"id":"311270","messageId":"alpine.DEB.2.20.1702101634010.3496@virtualbox","threadId":"45086","inReplyTo":"CANoM8SV7oJ6YmKM-n63620EkODxD562BZnLZB6OvX8O6BmDT1A@mail.gmail.com","subject":"Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-10T15:44:21Z","receivedAt":"2017-02-10T15:46:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Mike,\n\nOn Thu, 9 Feb 2017, Mike Rappazzo wrote:\n\n> On Thu, Feb 9, 2017 at 5:54 PM, Junio C Hamano wrote:\n> >\n> > That leaves what the right single-step behaviour change should be. As\n> > I recall Duy said something about --common-dir and other things Mike's\n> > earlier change also covered, I'd prefer to leave it to three of you to\n> > figure out what the final patch should be.\n> >\n> \n> The tests which I covered in my previous patch [1] addressed the places\n> where we identified similar problems. We should try to include some\n> form of those tests. As far as implementation goes in rev-parse, the\n> version in this thread is probably better that what I had, but it would\n> need to also be applied to --git-common-dir and --shared-index-path.\n\nThank you so much for pointing out that git-common-dir and\nshared-index-path have the same problem.\n\nI looked a little further, and it seems that the show_file() function may\nhave the exact same problem... but then, it only prefixes filenames if the\n--prefix= option has been passed, and it could be argued that\nthose prefixed filenames are *not* meant to be relative to the cwd but to\nthe top-level directory.\n\nAnways, v2 was just sent out, and with Peff's acknowledgement that this\nfixes a real bug and that hypothetical scripts relying on the buggy\nbehavior were broken beyond repair even without worktrees anyway, I am\nhopeful that we'll get somewhere.\n\nCiao,\nJohannes\n"},{"id":"311272","messageId":"cc23463af8096c5f8429f939ce881cf0eb5c2dcd.1486740772.git.johannes.schindelin@gmx.de","threadId":"45086","inReplyTo":"cover.1486740772.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-10T15:33:41Z","receivedAt":"2017-02-10T15:52:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Michael Rappazzo \n\nt2027-worktree-list has an incorrect expectation for --git-common-dir\nwhich has been adjusted and marked to expect failure.\n\nSome of the tests added have been marked to expect failure. These\ndemonstrate a problem with the way that some options to git rev-parse\nbehave when executed from a subdirectory of the main worktree.\n\n[jes: fixed incorrect assumption that objects/ lives in the\nworktree-specific git-dir (it lives in the common dir instead).]\n\nSigned-off-by: Michael Rappazzo \nSigned-off-by: Johannes Schindelin \n---\n t/t1500-rev-parse.sh | 28 ++++++++++++++++++++++++++++\n t/t1700-split-index.sh | 17 +++++++++++++++++\n t/t2027-worktree-list.sh | 12 ++++++++++--\n 3 files changed, 55 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 038e24c4014..f39f783f2db 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = tru\n \n test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''\n \n+test_expect_success 'git-common-dir from worktree root' '\n+\techo .git >expect &&\n+\tgit rev-parse --git-common-dir >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'git-common-dir inside sub-dir' '\n+\tmkdir -p path/to/child &&\n+\ttest_when_finished \"rm -rf path\" &&\n+\techo \"$(git -C path/to/child rev-parse --show-cdup).git\" >expect &&\n+\tgit -C path/to/child rev-parse --git-common-dir >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git-path from worktree root' '\n+\techo .git/objects >expect &&\n+\tgit rev-parse --git-path objects >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'git-path inside sub-dir' '\n+\tmkdir -p path/to/child &&\n+\ttest_when_finished \"rm -rf path\" &&\n+\techo \"$(git -C path/to/child rev-parse --show-cdup).git/objects\" >expect &&\n+\tgit -C path/to/child rev-parse --git-path objects >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 292a0720fcc..1d6e27a09d8 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -200,4 +200,21 @@ EOF\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'rev-parse --shared-index-path' '\n+\trm -rf .git &&\n+\ttest_create_repo . &&\n+\tgit update-index --split-index &&\n+\tls -t .git/sharedindex* | tail -n 1 >expect &&\n+\tgit rev-parse --shared-index-path >actual &&\n+\ttest_cmp expect actual &&\n+\tmkdir work &&\n+\ttest_when_finished \"rm -rf work\" &&\n+\t(\n+\t\tcd work &&\n+\t\tls -t ../.git/sharedindex* | tail -n 1 >expect &&\n+\t\tgit rev-parse --shared-index-path >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh\nindex 465eeeacd3d..c1a072348e7 100755\n--- a/t/t2027-worktree-list.sh\n+++ b/t/t2027-worktree-list.sh\n@@ -8,16 +8,24 @@ test_expect_success 'setup' '\n \ttest_commit init\n '\n \n-test_expect_success 'rev-parse --git-common-dir on main worktree' '\n+test_expect_failure 'rev-parse --git-common-dir on main worktree' '\n \tgit rev-parse --git-common-dir >actual &&\n \techo .git >expected &&\n \ttest_cmp expected actual &&\n \tmkdir sub &&\n \tgit -C sub rev-parse --git-common-dir >actual2 &&\n-\techo sub/.git >expected2 &&\n+\techo ../.git >expected2 &&\n \ttest_cmp expected2 actual2\n '\n \n+test_expect_failure 'rev-parse --git-path objects linked worktree' '\n+\techo \"$(git rev-parse --show-toplevel)/.git/objects\" >expect &&\n+\ttest_when_finished \"rm -rf linked-tree && git worktree prune\" &&\n+\tgit worktree add --detach linked-tree master &&\n+\tgit -C linked-tree rev-parse --git-path objects >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '\"list\" all worktrees from main' '\n \techo \"$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]\" >expect &&\n \ttest_when_finished \"rm -rf here && git worktree prune\" &&\n-- \n2.11.1.windows.1\n\n\n"},{"id":"311284","messageId":"xmqqshnllw2f.fsf@gitster.mtv.corp.google.com","threadId":"45086","inReplyTo":"cc23463af8096c5f8429f939ce881cf0eb5c2dcd.1486740772.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-10T18:50:16Z","receivedAt":"2017-02-10T18:50:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> From: Michael Rappazzo \n>\n> t2027-worktree-list has an incorrect expectation for --git-common-dir\n> which has been adjusted and marked to expect failure.\n>\n> Some of the tests added have been marked to expect failure. These\n> demonstrate a problem with the way that some options to git rev-parse\n> behave when executed from a subdirectory of the main worktree.\n>\n> [jes: fixed incorrect assumption that objects/ lives in the\n> worktree-specific git-dir (it lives in the common dir instead).]\n>\n> Signed-off-by: Michael Rappazzo \n> Signed-off-by: Johannes Schindelin \n> ---\n\nJust one iffy part; otherwise looks good.\n\n> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\n> index 038e24c4014..f39f783f2db 100755\n> --- a/t/t1500-rev-parse.sh\n> +++ b/t/t1500-rev-parse.sh\n> @@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = tru\n> \n> test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''\n> \n> +test_expect_success 'git-common-dir from worktree root' '\n> +\techo .git >expect &&\n> +\tgit rev-parse --git-common-dir >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_failure 'git-common-dir inside sub-dir' '\n> +\tmkdir -p path/to/child &&\n> +\ttest_when_finished \"rm -rf path\" &&\n> +\techo \"$(git -C path/to/child rev-parse --show-cdup).git\" >expect &&\n> +\tgit -C path/to/child rev-parse --git-common-dir >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'git-path from worktree root' '\n> +\techo .git/objects >expect &&\n> +\tgit rev-parse --git-path objects >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_failure 'git-path inside sub-dir' '\n> +\tmkdir -p path/to/child &&\n> +\ttest_when_finished \"rm -rf path\" &&\n> +\techo \"$(git -C path/to/child rev-parse --show-cdup).git/objects\" >expect &&\n> +\tgit -C path/to/child rev-parse --git-path objects >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nAll of these look sensible.\n\n> test_done\n> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> index 292a0720fcc..1d6e27a09d8 100755\n> --- a/t/t1700-split-index.sh\n> +++ b/t/t1700-split-index.sh\n> @@ -200,4 +200,21 @@ EOF\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_failure 'rev-parse --shared-index-path' '\n> +\trm -rf .git &&\n> +\ttest_create_repo . &&\n> +\tgit update-index --split-index &&\n> +\tls -t .git/sharedindex* | tail -n 1 >expect &&\n> +\tgit rev-parse --shared-index-path >actual &&\n> +\ttest_cmp expect actual &&\n> +\tmkdir work &&\n> +\ttest_when_finished \"rm -rf work\" &&\n> +\t(\n> +\t\tcd work &&\n> +\t\tls -t ../.git/sharedindex* | tail -n 1 >expect &&\n> +\t\tgit rev-parse --shared-index-path >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n\nThis looks iffy. If we expect multiple sharedindex* files, the\nfirst output from \"ls -t\" may or may not match the real one in use\n(multiple things do happen within a single second or whatever your\nfilesystem's time granularity is). Two \"ls -t\" run in this test\nwould (hopefully) give stable results, but I suspect that the chance\nthe first line in the output matches what --shared-index-path reports\nis 50% if we expect to have two sharedindex* files.\n\nOn the other hand, if we do not expect multiple sharedindex* files,\nuse of \"ls\" piped to \"tail\" is simply misleading.\n\nIf this test can be written in such a way that there is only one\nsuch file that match the pattern, it would be the cleanest to\nunderstand and explain. As there is only a single invocation of\n\"update-index --split-index\" immediately after a new repository is\ncreated, I suspect that the expectation to see only one sharedindex*\nfile already holds (because its name is unpredictable, we still need\nto catch it with wildcard), and if that is the case, we can just\nlose \"-t\" and pipe to tail, i.e. \"ls .git/sharedindex* >expect\".\n\n> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh\n> index 465eeeacd3d..c1a072348e7 100755\n> --- a/t/t2027-worktree-list.sh\n> +++ b/t/t2027-worktree-list.sh\n> @@ -8,16 +8,24 @@ test_expect_success 'setup' '\n> \ttest_commit init\n> '\n> \n> -test_expect_success 'rev-parse --git-common-dir on main worktree' '\n> +test_expect_failure 'rev-parse --git-common-dir on main worktree' '\n> \tgit rev-parse --git-common-dir >actual &&\n> \techo .git >expected &&\n> \ttest_cmp expected actual &&\n> \tmkdir sub &&\n> \tgit -C sub rev-parse --git-common-dir >actual2 &&\n> -\techo sub/.git >expected2 &&\n> +\techo ../.git >expected2 &&\n> \ttest_cmp expected2 actual2\n> '\n\nOK, this swaps a wrong expectation with a more usable one.\n\n> +test_expect_failure 'rev-parse --git-path objects linked worktree' '\n> +\techo \"$(git rev-parse --show-toplevel)/.git/objects\" >expect &&\n> +\ttest_when_finished \"rm -rf linked-tree && git worktree prune\" &&\n> +\tgit worktree add --detach linked-tree master &&\n> +\tgit -C linked-tree rev-parse --git-path objects >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n\nLooking good.\n\nThanks.\n"},{"id":"311288","messageId":"xmqqo9y9lvqk.fsf@gitster.mtv.corp.google.com","threadId":"45086","inReplyTo":"9242ee9717dcec95351b356070102f198eeb2537.1486740772.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 2/2] rev-parse: fix several options when running in a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-10T18:57:23Z","receivedAt":"2017-02-10T18:57:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index ff13e59e1db..84af2802f6f 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> \tunsigned int flags = 0;\n> \tconst char *name = NULL;\n> \tstruct object_context unused;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> \n> \tif (argc > 1 && !strcmp(\"--parseopt\", argv[1]))\n> \t\treturn cmd_parseopt(argc - 1, argv + 1, prefix);\n> @@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\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> -\t\t\tputs(git_path(\"%s\", argv[i + 1]));\n> +\t\t\tstrbuf_reset(&buf);\n> +\t\t\tputs(relative_path(git_path(\"%s\", argv[i + 1]),\n> +\t\t\t\t\t prefix, &buf));\n> \t\t\ti++;\n> \t\t\tcontinue;\n> \t\t}\n> @@ -821,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (!strcmp(arg, \"--git-common-dir\")) {\n> -\t\t\t\tconst char *pfx = prefix ? prefix : \"\";\n> -\t\t\t\tputs(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));\n> +\t\t\t\tstrbuf_reset(&buf);\n> +\t\t\t\tputs(relative_path(get_git_common_dir(),\n> +\t\t\t\t\t\t prefix, &buf));\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t\tif (!strcmp(arg, \"--is-inside-git-dir\")) {\n> @@ -845,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> \t\t\t\t\tdie(_(\"Could not read the index\"));\n> \t\t\t\tif (the_index.split_index) {\n> \t\t\t\t\tconst unsigned char *sha1 = the_index.split_index->base_sha1;\n> -\t\t\t\t\tputs(git_path(\"sharedindex.%s\", sha1_to_hex(sha1)));\n> +\t\t\t\t\tconst char *path = git_path(\"sharedindex.%s\", sha1_to_hex(sha1));\n> +\t\t\t\t\tstrbuf_reset(&buf);\n> +\t\t\t\t\tputs(relative_path(path, prefix, &buf));\n> \t\t\t\t}\n> \t\t\t\tcontinue;\n> \t\t\t}\n> @@ -906,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> \t\tdie_no_single_rev(quiet);\n> \t} else\n> \t\tshow_default();\n> +\tstrbuf_release(&buf);\n\nThis uses \"reset then use\" pattern for repeated use of strbuf, and\ncauses the string last held in the strbuf to leak on early return,\nwhich can be mitigated by using \"use then reset\" pattern. I.e.\n\n\t\t\tif (!strcmp(arg, \"--git-common-dir\")) {\n\t\t\t\tputs(relative_path(get_git_common_dir(),\n\t\t\t\t\t\t prefix, &buf));\n\t\t\t\tstrbuf_reset(&buf);\n\t\t\t\tcontinue;\n\t\t\t}\n\nI'd think. You'd still want to release it at the end anyway for\ngood code hygiene, though.\n\nOther than that, looks good to me.\n\nThanks.\n"},{"id":"311289","messageId":"xmqqk28xlvmb.fsf@gitster.mtv.corp.google.com","threadId":"45086","inReplyTo":"cover.1486740772.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 0/2] Fix bugs in rev-parse's output when run in a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-10T18:59:56Z","receivedAt":"2017-02-10T19:00:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Johannes Schindelin (1):\n> rev-parse: fix several options when running in a subdirectory\n>\n> Michael Rappazzo (1):\n> rev-parse tests: add tests executed from a subdirectory\n>\n> builtin/rev-parse.c | 15 +++++++++++----\n> t/t1500-rev-parse.sh | 28 ++++++++++++++++++++++++++++\n> t/t1700-split-index.sh | 17 +++++++++++++++++\n> t/t2027-worktree-list.sh | 10 +++++++++-\n> 4 files changed, 65 insertions(+), 5 deletions(-)\n>\n>\n> base-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8\n> Published-As: https://github.com/dscho/git/releases/tag/git-path-in-subdir-v2\n> Fetch-It-Via: git fetch https://github.com/dscho/git git-path-in-subdir-v2\n\nWill queue as js/git-path-in-subdir topic forked at 2.12-rc0.\n\nI still think the log message in 2/2 is making a mountain out of\nmolehill and showing a skewed perception on pros-and-cons in a\ndesign decision, but I won't repeat my review. I saw a few\ncorrectness issues and pointed them out on the patches.\n"},{"id":"311304","messageId":"xmqqpoipkd3l.fsf@gitster.mtv.corp.google.com","threadId":"45086","inReplyTo":"cc23463af8096c5f8429f939ce881cf0eb5c2dcd.1486740772.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-10T20:25:18Z","receivedAt":"2017-02-10T20:25:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> index 292a0720fcc..1d6e27a09d8 100755\n> --- a/t/t1700-split-index.sh\n> +++ b/t/t1700-split-index.sh\n> @@ -200,4 +200,21 @@ EOF\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_failure 'rev-parse --shared-index-path' '\n> +\trm -rf .git &&\n> +\ttest_create_repo . &&\n\nAnother thing that I notice only after merging this and other topics\nto 'pu' was that this piece needs to always come at the end of the\nscript because of this removal. It would make the test more robust\nto create a test repository for this test and work inside it.\n\n> +\tgit update-index --split-index &&\n> +\tls -t .git/sharedindex* | tail -n 1 >expect &&\n> +\tgit rev-parse --shared-index-path >actual &&\n> +\ttest_cmp expect actual &&\n> +\tmkdir work &&\n> +\ttest_when_finished \"rm -rf work\" &&\n> +\t(\n> +\t\tcd work &&\n> +\t\tls -t ../.git/sharedindex* | tail -n 1 >expect &&\n> +\t\tgit rev-parse --shared-index-path >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> test_done\n"},{"id":"311901","messageId":"alpine.DEB.2.20.1702171749180.3496@virtualbox","threadId":"45086","inReplyTo":"xmqqo9y9lvqk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] rev-parse: fix several options when running in a subdirectory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-17T16:53:13Z","receivedAt":"2017-02-17T16:53:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 10 Feb 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> > index ff13e59e1db..84af2802f6f 100644\n> > --- a/builtin/rev-parse.c\n> > +++ b/builtin/rev-parse.c\n> > @@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> > \tunsigned int flags = 0;\n> > \tconst char *name = NULL;\n> > \tstruct object_context unused;\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > \n> > \tif (argc > 1 && !strcmp(\"--parseopt\", argv[1]))\n> > \t\treturn cmd_parseopt(argc - 1, argv + 1, prefix);\n> > @@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\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> > -\t\t\tputs(git_path(\"%s\", argv[i + 1]));\n> > +\t\t\tstrbuf_reset(&buf);\n> > +\t\t\tputs(relative_path(git_path(\"%s\", argv[i + 1]),\n> > +\t\t\t\t\t prefix, &buf));\n> > \t\t\ti++;\n> > \t\t\tcontinue;\n> > \t\t}\n> > @@ -821,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> > \t\t\t\tcontinue;\n> > \t\t\t}\n> > \t\t\tif (!strcmp(arg, \"--git-common-dir\")) {\n> > -\t\t\t\tconst char *pfx = prefix ? prefix : \"\";\n> > -\t\t\t\tputs(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));\n> > +\t\t\t\tstrbuf_reset(&buf);\n> > +\t\t\t\tputs(relative_path(get_git_common_dir(),\n> > +\t\t\t\t\t\t prefix, &buf));\n> > \t\t\t\tcontinue;\n> > \t\t\t}\n> > \t\t\tif (!strcmp(arg, \"--is-inside-git-dir\")) {\n> > @@ -845,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> > \t\t\t\t\tdie(_(\"Could not read the index\"));\n> > \t\t\t\tif (the_index.split_index) {\n> > \t\t\t\t\tconst unsigned char *sha1 = the_index.split_index->base_sha1;\n> > -\t\t\t\t\tputs(git_path(\"sharedindex.%s\", sha1_to_hex(sha1)));\n> > +\t\t\t\t\tconst char *path = git_path(\"sharedindex.%s\", sha1_to_hex(sha1));\n> > +\t\t\t\t\tstrbuf_reset(&buf);\n> > +\t\t\t\t\tputs(relative_path(path, prefix, &buf));\n> > \t\t\t\t}\n> > \t\t\t\tcontinue;\n> > \t\t\t}\n> > @@ -906,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> > \t\tdie_no_single_rev(quiet);\n> > \t} else\n> > \t\tshow_default();\n> > +\tstrbuf_release(&buf);\n> \n> This uses \"reset then use\" pattern for repeated use of strbuf, and\n> causes the string last held in the strbuf to leak on early return,\n\n... which cannot happen due to the lack of an early return...\n\n> which can be mitigated by using \"use then reset\" pattern. I.e.\n> \n> \t\t\tif (!strcmp(arg, \"--git-common-dir\")) {\n> \t\t\t\tputs(relative_path(get_git_common_dir(),\n> \t\t\t\t\t\t prefix, &buf));\n> \t\t\t\tstrbuf_reset(&buf);\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \n> I'd think.\n\nThis would not release the memory, though:\n\n\t#define strbuf_reset(sb) strbuf_setlen(sb, 0)\n\nand\n\n\tstatic inline void strbuf_setlen(struct strbuf *sb, size_t len)\n\t{\n\t\tif (len > (sb->alloc ? sb->alloc - 1 : 0))\n\t\t\tdie(\"BUG: strbuf_setlen() beyond buffer\");\n\t\tsb->len = len;\n\t\tsb->buf[len] = '\\0';\n\t}\n\nThere is not a single free() statement there.\n\nSo the \"use then reset\" scheme would leak *just the same*.\n\n> You'd still want to release it at the end anyway for good code hygiene,\n> though.\n\nWhich I do.\n\nTechnically, this is not even necessary because all of the cmd_*()\nfunctions are immediately followed by a call to exit(). Wasn't that the\ngenius idea in the early Git days, that we could simply get away with\nsloppy memory management because the program exit()s shortly afterwards,\nanyway? ;-)\n\nIn any case, I adjusted the commit message to clarify why the \"reset then\nuse\" scheme is correct here.\n\nCiao,\nJohannes\n"},{"id":"311902","messageId":"alpine.DEB.2.20.1702171753340.3496@virtualbox","threadId":"45086","inReplyTo":"xmqqshnllw2f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-17T16:55:41Z","receivedAt":"2017-02-17T16:55:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 10 Feb 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> > index 292a0720fcc..1d6e27a09d8 100755\n> > --- a/t/t1700-split-index.sh\n> > +++ b/t/t1700-split-index.sh\n> > @@ -200,4 +200,21 @@ EOF\n> > \ttest_cmp expect actual\n> > '\n> > \n> > +test_expect_failure 'rev-parse --shared-index-path' '\n> > +\trm -rf .git &&\n> > +\ttest_create_repo . &&\n> > +\tgit update-index --split-index &&\n> > +\tls -t .git/sharedindex* | tail -n 1 >expect &&\n> > +\tgit rev-parse --shared-index-path >actual &&\n> > +\ttest_cmp expect actual &&\n> > +\tmkdir work &&\n> > +\ttest_when_finished \"rm -rf work\" &&\n> > +\t(\n> > +\t\tcd work &&\n> > +\t\tls -t ../.git/sharedindex* | tail -n 1 >expect &&\n> > +\t\tgit rev-parse --shared-index-path >actual &&\n> > +\t\ttest_cmp expect actual\n> > +\t)\n> \n> This looks iffy.\n\nIndeed.\n\n> If we expect multiple sharedindex* files, the first output from \"ls -t\"\n> may or may not match the real one in use (multiple things do happen\n> within a single second or whatever your filesystem's time granularity\n> is). Two \"ls -t\" run in this test would (hopefully) give stable\n> results, but I suspect that the chance the first line in the output\n> matches what --shared-index-path reports is 50% if we expect to have two\n> sharedindex* files.\n> \n> On the other hand, if we do not expect multiple sharedindex* files,\n> use of \"ls\" piped to \"tail\" is simply misleading.\n> \n> If this test can be written in such a way that there is only one\n> such file that match the pattern, it would be the cleanest to\n> understand and explain. As there is only a single invocation of\n> \"update-index --split-index\" immediately after a new repository is\n> created, I suspect that the expectation to see only one sharedindex*\n> file already holds (because its name is unpredictable, we still need\n> to catch it with wildcard), and if that is the case, we can just\n> lose \"-t\" and pipe to tail, i.e. \"ls .git/sharedindex* >expect\".\n\nIndeed. We can expect only one sharedindex file to be present, and we do\nnot even have to call out to `ls` but can get away with calling `echo`\n(which is a builtin in Bash).\n\nCiao,\nJohannes\n"},{"id":"311904","messageId":"alpine.DEB.2.20.1702171755470.3496@virtualbox","threadId":"45086","inReplyTo":"xmqqpoipkd3l.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-17T16:57:56Z","receivedAt":"2017-02-17T16:58:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 10 Feb 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> > index 292a0720fcc..1d6e27a09d8 100755\n> > --- a/t/t1700-split-index.sh\n> > +++ b/t/t1700-split-index.sh\n> > @@ -200,4 +200,21 @@ EOF\n> > \ttest_cmp expect actual\n> > '\n> > \n> > +test_expect_failure 'rev-parse --shared-index-path' '\n> > +\trm -rf .git &&\n> > +\ttest_create_repo . &&\n> \n> Another thing that I notice only after merging this and other topics\n> to 'pu' was that this piece needs to always come at the end of the\n> script because of this removal. It would make the test more robust\n> to create a test repository for this test and work inside it.\n\nYes, this is indeed unnecessary and even undesirable.\n\nI waited a couple of days to give the original author a chance to fix\nthis. As I want a fix really badly (because I am really embarrassed of\nthis bug), I adjusted t1700 appropriately and will send out v3 in a\nsecond.\n\nCiao,\nJohannes\n"},{"id":"311905","messageId":"cover.1487350582.git.johannes.schindelin@gmx.de","threadId":"45086","inReplyTo":"cover.1486740772.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 0/2] Fix bugs in rev-parse's output when run in a subdirectory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-17T16:58:53Z","receivedAt":"2017-02-17T16:59:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The bug that bit me (hard!) and that triggered not only a long series of\ncurses but also my writing a patch and sending it to the list was that\n`git rev-parse --git-path HEAD` would give *incorrect* output when run\nin a subdirectory of a regular checkout, but *correct* output when run\nin a subdirectory of an associated *worktree*.\n\nI had tested the script in question quite a bit, but in a worktree. And\nin production, it quietly did exactly the wrong thing.\n\nChanges relative to v2:\n\n- the \"iffy\" test in t1700 was made \"uniffy\"\n\n- clarified in the commit message of 2/2 why we can get away with the\n \"reset then use\" pattern\n\n\nJohannes Schindelin (1):\n rev-parse: fix several options when running in a subdirectory\n\nMichael Rappazzo (1):\n rev-parse tests: add tests executed from a subdirectory\n\n builtin/rev-parse.c | 15 +++++++++++----\n t/t1500-rev-parse.sh | 28 ++++++++++++++++++++++++++++\n t/t1700-split-index.sh | 16 ++++++++++++++++\n t/t2027-worktree-list.sh | 10 +++++++++-\n 4 files changed, 64 insertions(+), 5 deletions(-)\n\n\nbase-commit: 076c05393a047247ea723896289b48d6549ed7d0\nPublished-As: https://github.com/dscho/git/releases/tag/git-path-in-subdir-v3\nFetch-It-Via: git fetch https://github.com/dscho/git git-path-in-subdir-v3\n\nInterdiff vs v2:\n\n diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n index 84af2802f6f..2cfd8d2aae4 100644\n --- a/builtin/rev-parse.c\n +++ b/builtin/rev-parse.c\n @@ -903,6 +903,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\tverify_filename(prefix, arg, 1);\n \t}\n +\tstrbuf_release(&buf);\n \tif (verify) {\n \t\tif (revs_count == 1) {\n \t\t\tshow_rev(type, sha1, name);\n @@ -912,6 +913,5 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\tdie_no_single_rev(quiet);\n \t} else\n \t\tshow_default();\n -\tstrbuf_release(&buf);\n \treturn 0;\n }\n diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n index 446ff34f966..6096f2c6309 100755\n --- a/t/t1700-split-index.sh\n +++ b/t/t1700-split-index.sh\n @@ -201,17 +201,16 @@ EOF\n '\n \n test_expect_success 'rev-parse --shared-index-path' '\n -\trm -rf .git &&\n -\ttest_create_repo . &&\n -\tgit update-index --split-index &&\n -\tls -t .git/sharedindex* | tail -n 1 >expect &&\n -\tgit rev-parse --shared-index-path >actual &&\n -\ttest_cmp expect actual &&\n -\tmkdir work &&\n -\ttest_when_finished \"rm -rf work\" &&\n +\ttest_create_repo split-index &&\n \t(\n -\t\tcd work &&\n -\t\tls -t ../.git/sharedindex* | tail -n 1 >expect &&\n +\t\tcd split-index &&\n +\t\tgit update-index --split-index &&\n +\t\techo .git/sharedindex* >expect &&\n +\t\tgit rev-parse --shared-index-path >actual &&\n +\t\ttest_cmp expect actual &&\n +\t\tmkdir subdirectory &&\n +\t\tcd subdirectory &&\n +\t\techo ../.git/sharedindex* >expect &&\n \t\tgit rev-parse --shared-index-path >actual &&\n \t\ttest_cmp expect actual\n \t)\n\n-- \n2.11.1.windows.1.2.g87ad093.dirty\n\n"},{"id":"311906","messageId":"471f711137c41073e34d8e9ee562fefe95715842.1487350582.git.johannes.schindelin@gmx.de","threadId":"45086","inReplyTo":"cover.1487350582.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-17T16:59:02Z","receivedAt":"2017-02-17T16:59:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Michael Rappazzo \n\nt2027-worktree-list has an incorrect expectation for --git-common-dir\nwhich has been adjusted and marked to expect failure.\n\nSome of the tests added have been marked to expect failure. These\ndemonstrate a problem with the way that some options to git rev-parse\nbehave when executed from a subdirectory of the main worktree.\n\n[jes: fixed incorrect assumption that objects/ lives in the\nworktree-specific git-dir (it lives in the common dir instead). Also\nadjusted t1700 so that the test case does not *need* to be the last\none in that script.]\n\nSigned-off-by: Michael Rappazzo \nSigned-off-by: Johannes Schindelin \n---\n t/t1500-rev-parse.sh | 28 ++++++++++++++++++++++++++++\n t/t1700-split-index.sh | 16 ++++++++++++++++\n t/t2027-worktree-list.sh | 12 ++++++++++--\n 3 files changed, 54 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 038e24c4014..f39f783f2db 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = tru\n \n test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''\n \n+test_expect_success 'git-common-dir from worktree root' '\n+\techo .git >expect &&\n+\tgit rev-parse --git-common-dir >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'git-common-dir inside sub-dir' '\n+\tmkdir -p path/to/child &&\n+\ttest_when_finished \"rm -rf path\" &&\n+\techo \"$(git -C path/to/child rev-parse --show-cdup).git\" >expect &&\n+\tgit -C path/to/child rev-parse --git-common-dir >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git-path from worktree root' '\n+\techo .git/objects >expect &&\n+\tgit rev-parse --git-path objects >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'git-path inside sub-dir' '\n+\tmkdir -p path/to/child &&\n+\ttest_when_finished \"rm -rf path\" &&\n+\techo \"$(git -C path/to/child rev-parse --show-cdup).git/objects\" >expect &&\n+\tgit -C path/to/child rev-parse --git-path objects >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 292a0720fcc..b754865a618 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -200,4 +200,20 @@ EOF\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'rev-parse --shared-index-path' '\n+\ttest_create_repo split-index &&\n+\t(\n+\t\tcd split-index &&\n+\t\tgit update-index --split-index &&\n+\t\techo .git/sharedindex* >expect &&\n+\t\tgit rev-parse --shared-index-path >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tmkdir subdirectory &&\n+\t\tcd subdirectory &&\n+\t\techo ../.git/sharedindex* >expect &&\n+\t\tgit rev-parse --shared-index-path >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh\nindex 465eeeacd3d..c1a072348e7 100755\n--- a/t/t2027-worktree-list.sh\n+++ b/t/t2027-worktree-list.sh\n@@ -8,16 +8,24 @@ test_expect_success 'setup' '\n \ttest_commit init\n '\n \n-test_expect_success 'rev-parse --git-common-dir on main worktree' '\n+test_expect_failure 'rev-parse --git-common-dir on main worktree' '\n \tgit rev-parse --git-common-dir >actual &&\n \techo .git >expected &&\n \ttest_cmp expected actual &&\n \tmkdir sub &&\n \tgit -C sub rev-parse --git-common-dir >actual2 &&\n-\techo sub/.git >expected2 &&\n+\techo ../.git >expected2 &&\n \ttest_cmp expected2 actual2\n '\n \n+test_expect_failure 'rev-parse --git-path objects linked worktree' '\n+\techo \"$(git rev-parse --show-toplevel)/.git/objects\" >expect &&\n+\ttest_when_finished \"rm -rf linked-tree && git worktree prune\" &&\n+\tgit worktree add --detach linked-tree master &&\n+\tgit -C linked-tree rev-parse --git-path objects >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '\"list\" all worktrees from main' '\n \techo \"$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]\" >expect &&\n \ttest_when_finished \"rm -rf here && git worktree prune\" &&\n-- \n2.11.1.windows.1.2.g87ad093.dirty\n\n\n"},{"id":"311907","messageId":"e5fddea216960a53519ba152696d76f509c54bee.1487350582.git.johannes.schindelin@gmx.de","threadId":"45086","inReplyTo":"cover.1487350582.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 2/2] rev-parse: fix several options when running in a subdirectory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-17T16:59:06Z","receivedAt":"2017-02-17T16:59:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In addition to making git_path() aware of certain file names that need\nto be handled differently e.g. when running in worktrees, the commit\n557bd833bb (git_path(): be aware of file relocation in $GIT_DIR,\n2014-11-30) also snuck in a new option for `git rev-parse`:\n`--git-path`.\n\nOn the face of it, there is no obvious bug in that commit's diff: it\nfaithfully calls git_path() on the argument and prints it out, i.e. `git\nrev-parse --git-path ` has the same precise behavior as\ncalling `git_path(\"\")` in C.\n\nThe problem lies deeper, much deeper. In hindsight (which is always\nunfair), implementing the .git/ directory discovery in\n`setup_git_directory()` by changing the working directory may have\nallowed us to avoid passing around a struct that contains information\nabout the current repository, but it bought us many, many problems.\n\nIn this case, when being called in a subdirectory, `git rev-parse`\nchanges the working directory to the top-level directory before calling\n`git_path()`. In the new working directory, the result is correct. But\nin the working directory of the calling script, it is incorrect.\n\nExample: when calling `git rev-parse --git-path HEAD` in, say, the\nDocumentation/ subdirectory of Git's own source code, the string\n`.git/HEAD` is printed.\n\nSide note: that bug is hidden when running in a subdirectory of a\nworktree that was added by the `git worktree` command: in that case, the\n(correct) absolute path of the `HEAD` file is printed.\n\nIn the interest of time, this patch does not go the \"correct\" route to\nintroduce a struct with repository information (and removing global\nstate in the process), instead this patch chooses to detect when the\ncommand was called in a subdirectory and forces the result to be an\nabsolute path.\n\nWhile at it, we are also fixing the output of --git-common-dir and\n--shared-index-path.\n\nLastly, please note that we reuse the same strbuf for all of the\nrelative_path() calls; this avoids frequent allocation (and duplicated\ncode), and it does not risk memory leaks, for two reasons: 1) the\ncmd_rev_parse() function does not return anywhere between the use of\nthe new strbuf instance and its final release, and 2) git-rev-parse is\none of these \"one-shot\" programs in Git, i.e. it exits after running\nfor a very short time, meaning that all allocated memory is released\nwith the exit() call anyway.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rev-parse.c | 15 +++++++++++----\n t/t1500-rev-parse.sh | 4 ++--\n t/t1700-split-index.sh | 2 +-\n t/t2027-worktree-list.sh | 4 ++--\n 4 files changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex ff13e59e1db..2cfd8d2aae4 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \tunsigned int flags = 0;\n \tconst char *name = NULL;\n \tstruct object_context unused;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n \tif (argc > 1 && !strcmp(\"--parseopt\", argv[1]))\n \t\treturn cmd_parseopt(argc - 1, argv + 1, prefix);\n@@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\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-\t\t\tputs(git_path(\"%s\", argv[i + 1]));\n+\t\t\tstrbuf_reset(&buf);\n+\t\t\tputs(relative_path(git_path(\"%s\", argv[i + 1]),\n+\t\t\t\t\t prefix, &buf));\n \t\t\ti++;\n \t\t\tcontinue;\n \t\t}\n@@ -821,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--git-common-dir\")) {\n-\t\t\t\tconst char *pfx = prefix ? prefix : \"\";\n-\t\t\t\tputs(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));\n+\t\t\t\tstrbuf_reset(&buf);\n+\t\t\t\tputs(relative_path(get_git_common_dir(),\n+\t\t\t\t\t\t prefix, &buf));\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--is-inside-git-dir\")) {\n@@ -845,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tdie(_(\"Could not read the index\"));\n \t\t\t\tif (the_index.split_index) {\n \t\t\t\t\tconst unsigned char *sha1 = the_index.split_index->base_sha1;\n-\t\t\t\t\tputs(git_path(\"sharedindex.%s\", sha1_to_hex(sha1)));\n+\t\t\t\t\tconst char *path = git_path(\"sharedindex.%s\", sha1_to_hex(sha1));\n+\t\t\t\t\tstrbuf_reset(&buf);\n+\t\t\t\t\tputs(relative_path(path, prefix, &buf));\n \t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t}\n@@ -897,6 +903,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\tverify_filename(prefix, arg, 1);\n \t}\n+\tstrbuf_release(&buf);\n \tif (verify) {\n \t\tif (revs_count == 1) {\n \t\t\tshow_rev(type, sha1, name);\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex f39f783f2db..d74f09ad93e 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -93,7 +93,7 @@ test_expect_success 'git-common-dir from worktree root' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'git-common-dir inside sub-dir' '\n+test_expect_success 'git-common-dir inside sub-dir' '\n \tmkdir -p path/to/child &&\n \ttest_when_finished \"rm -rf path\" &&\n \techo \"$(git -C path/to/child rev-parse --show-cdup).git\" >expect &&\n@@ -107,7 +107,7 @@ test_expect_success 'git-path from worktree root' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'git-path inside sub-dir' '\n+test_expect_success 'git-path inside sub-dir' '\n \tmkdir -p path/to/child &&\n \ttest_when_finished \"rm -rf path\" &&\n \techo \"$(git -C path/to/child rev-parse --show-cdup).git/objects\" >expect &&\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex b754865a618..6096f2c6309 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -200,7 +200,7 @@ EOF\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'rev-parse --shared-index-path' '\n+test_expect_success 'rev-parse --shared-index-path' '\n \ttest_create_repo split-index &&\n \t(\n \t\tcd split-index &&\ndiff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh\nindex c1a072348e7..848da5f3684 100755\n--- a/t/t2027-worktree-list.sh\n+++ b/t/t2027-worktree-list.sh\n@@ -8,7 +8,7 @@ test_expect_success 'setup' '\n \ttest_commit init\n '\n \n-test_expect_failure 'rev-parse --git-common-dir on main worktree' '\n+test_expect_success 'rev-parse --git-common-dir on main worktree' '\n \tgit rev-parse --git-common-dir >actual &&\n \techo .git >expected &&\n \ttest_cmp expected actual &&\n@@ -18,7 +18,7 @@ test_expect_failure 'rev-parse --git-common-dir on main worktree' '\n \ttest_cmp expected2 actual2\n '\n \n-test_expect_failure 'rev-parse --git-path objects linked worktree' '\n+test_expect_success 'rev-parse --git-path objects linked worktree' '\n \techo \"$(git rev-parse --show-toplevel)/.git/objects\" >expect &&\n \ttest_when_finished \"rm -rf linked-tree && git worktree prune\" &&\n \tgit worktree add --detach linked-tree master &&\n-- \n2.11.1.windows.1.2.g87ad093.dirty\n"},{"id":"311918","messageId":"xmqq8tp4u129.fsf@gitster.mtv.corp.google.com","threadId":"45086","inReplyTo":"cover.1487350582.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 0/2] Fix bugs in rev-parse's output when run in a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-17T18:25:34Z","receivedAt":"2017-02-17T18:25:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The bug that bit me (hard!) and that triggered not only a long series of\n> curses but also my writing a patch and sending it to the list was that\n> `git rev-parse --git-path HEAD` would give *incorrect* output when run\n> in a subdirectory of a regular checkout, but *correct* output when run\n> in a subdirectory of an associated *worktree*.\n>\n> I had tested the script in question quite a bit, but in a worktree. And\n> in production, it quietly did exactly the wrong thing.\n>\n> Changes relative to v2:\n>\n> - the \"iffy\" test in t1700 was made \"uniffy\"\n>\n> - clarified in the commit message of 2/2 why we can get away with the\n> \"reset then use\" pattern\n\nIt is no longer relevant between \"reset then use\" and \"use then\nreset\", I think, because you did something much better, which is to\nmove strbuf_release() up so that it comes before the possible early\nreturns.\n\nBoth patches look good. Let's queue this and move it to 'next'\nshortly. Personally, I think it is OK to fast-track this to\n'master' before the final, but just like any other bugs, we've lived\nwith the bug for some time, and it is not a big deal if we have to\nlive with it for a bit longer.\n\nThanks.\n"}]}