{"thread":{"id":"42842","subject":"[PATCH] mingw: fix regression in t1308-config-set","startedAt":"2016-07-14T13:59:12Z","lastAt":"2016-07-14T18:40:28Z","messageCount":3,"participants":["Johannes Schindelin","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"291414","messageId":"6f439a56703ca6fb5c269c75904796ae67e96960.1468504461.git.johannes.schindelin@gmx.de","threadId":"42842","inReplyTo":null,"subject":"[PATCH] mingw: fix regression in t1308-config-set","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-14T13:58:59Z","receivedAt":"2016-07-14T13:59:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When we tried to fix in 58461bd (t1308: do not get fooled by symbolic\nlinks to the source tree, 2016-06-02) an obscure case where the user\ncd's into Git's source code via a symbolic link, a regression was\nintroduced that affects all test runs on Windows.\n\nThe original patch introducing the test case in question was careful to\nuse `$(pwd)` instead of `$PWD`.\n\nThis was done to account for the fact that Git's test suite uses shell\nscripting even on Windows, where the shell's Unix-y paths are\nincompatible with the main Git executable's idea of paths: it only\naccepts Windows paths.\n\nIt is an awkward but necessary thing, then, to use `$(pwd)` (which gives\nus a Windows path) when interacting with the Git executable and `$PWD`\n(which gives the shell's idea of the current working directory in Unix-y\nform) for shell scripts, including the test suite itself.\n\nObviously this broke the use case of the Git maintainer when changing\nthe working directory into Git's source code directory via a symlink,\ni.e. when `$(pwd)` does not agree with `$PWD`.\n\nHowever, we must not fix that use case at the expense of regressing\nanother use case.\n\nLet's special-case Windows here, even if it is ugly, for lack of a more\nelegant solution.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\nPublished-As: https://github.com/dscho/git/releases/tag/t1308-on-windows-v1\n\n\tSide note: it was not at all clear to me how 58461bd fixed the\n\tproblem by replacing $(pwd) with $HOME, given that HOME is set to\n\t$TRASH_DIRECTORY which is set to $TEST_OUTPUT_DIRECTORY/... after\n\tTEST_OUTPUT_DIRECTORY was set to TEST_DIRECTORY which in turn was\n\tset to $(pwd).\n\n\tI guess the reason is that -P in `cd -P \"$TRASH DIRECTORY\"`, but\n\tthen I *really* do not understand how $(pwd) and $PWD could\n\tdisagree.\n\n\tOh well. I have to move on to the next fire.\n\n t/t1308-config-set.sh | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex a06e71c..7655c94 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -233,11 +233,19 @@ cmdline_config=\"'foo.bar=from-cmdline'\"\n test_expect_success 'iteration shows correct origins' '\n \techo \"[foo]bar = from-repo\" >.git/config &&\n \techo \"[foo]bar = from-home\" >.gitconfig &&\n+\tif test_have_prereq MINGW\n+\tthen\n+\t\t# Use Windows path (i.e. *not* $HOME)\n+\t\tHOME_GITCONFIG=$(pwd)/.gitconfig\n+\telse\n+\t\t# Do not get fooled by symbolic links, i.e. $HOME != $(pwd)\n+\t\tHOME_GITCONFIG=$HOME/.gitconfig\n+\tfi &&\n \tcat >expect <<-EOF &&\n \tkey=foo.bar\n \tvalue=from-home\n \torigin=file\n-\tname=$HOME/.gitconfig\n+\tname=$HOME_GITCONFIG\n \tscope=global\n \n \tkey=foo.bar\n-- \n2.9.0.278.g1caae67\n\nbase-commit: 79ed43c28f626a4e805f350a77c54968b59be6e9\n"},{"id":"291432","messageId":"xmqqr3awry5x.fsf@gitster.mtv.corp.google.com","threadId":"42842","inReplyTo":"6f439a56703ca6fb5c269c75904796ae67e96960.1468504461.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] mingw: fix regression in t1308-config-set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-14T16:18:18Z","receivedAt":"2016-07-14T16:18:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> When we tried to fix in 58461bd (t1308: do not get fooled by symbolic\n> links to the source tree, 2016-06-02) an obscure case where the user\n> cd's into Git's source code via a symbolic link, a regression was\n> introduced that affects all test runs on Windows.\n\nThanks for producing a fix quickly after the topic hit 'master'.\n\nThe original came from\n\n  http://thread.gmane.org/gmane.comp.version-control.git/296021/focus=296199\n\nwhich was merged at e5e5bb67 (Merge branch 'jk/upload-pack-hook'\ninto next, 2016-06-28) to 'next'.  \n\nI see J6t raised C:/foo vs /c/foo issue in the thread later, but\nunfortunately I didn't notice it.\n\nI added a few missing Cc: and quoted the whole patch here to those\nwho were involved; I think this update is correct, but just trying\nto make sure people know.\n\nNot limited to this particular topic, there probably are some things\nwe can and should add to the procedure to prevent further episodes\nlike this, but I am not seeing anything immediately obvious offhand.\nThere already is a way to prominently mark a topic to be not-ready\nwith an outstanding issue called \"What's cooking\" report, but it is\nmaintained manually and it can be leaky without extra set of eyes\nconstantly monitoring.\n\n> The original patch introducing the test case in question was careful to\n> use `$(pwd)` instead of `$PWD`.\n>\n> This was done to account for the fact that Git's test suite uses shell\n> scripting even on Windows, where the shell's Unix-y paths are\n> incompatible with the main Git executable's idea of paths: it only\n> accepts Windows paths.\n>\n> It is an awkward but necessary thing, then, to use `$(pwd)` (which gives\n> us a Windows path) when interacting with the Git executable and `$PWD`\n> (which gives the shell's idea of the current working directory in Unix-y\n> form) for shell scripts, including the test suite itself.\n>\n> Obviously this broke the use case of the Git maintainer when changing\n> the working directory into Git's source code directory via a symlink,\n> i.e. when `$(pwd)` does not agree with `$PWD`.\n>\n> However, we must not fix that use case at the expense of regressing\n> another use case.\n>\n> Let's special-case Windows here, even if it is ugly, for lack of a more\n> elegant solution.\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n> Published-As: https://github.com/dscho/git/releases/tag/t1308-on-windows-v1\n>\n> \tSide note: it was not at all clear to me how 58461bd fixed the\n> \tproblem by replacing $(pwd) with $HOME, given that HOME is set to\n> \t$TRASH_DIRECTORY which is set to $TEST_OUTPUT_DIRECTORY/... after\n> \tTEST_OUTPUT_DIRECTORY was set to TEST_DIRECTORY which in turn was\n> \tset to $(pwd).\n>\n> \tI guess the reason is that -P in `cd -P \"$TRASH DIRECTORY\"`, but\n> \tthen I *really* do not understand how $(pwd) and $PWD could\n> \tdisagree.\n> \tOh well. I have to move on to the next fire.\n>\n>  t/t1308-config-set.sh | 10 +++++++++-\n>  1 file changed, 9 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\n> index a06e71c..7655c94 100755\n> --- a/t/t1308-config-set.sh\n> +++ b/t/t1308-config-set.sh\n> @@ -233,11 +233,19 @@ cmdline_config=\"'foo.bar=from-cmdline'\"\n>  test_expect_success 'iteration shows correct origins' '\n>  \techo \"[foo]bar = from-repo\" >.git/config &&\n>  \techo \"[foo]bar = from-home\" >.gitconfig &&\n> +\tif test_have_prereq MINGW\n> +\tthen\n> +\t\t# Use Windows path (i.e. *not* $HOME)\n> +\t\tHOME_GITCONFIG=$(pwd)/.gitconfig\n> +\telse\n> +\t\t# Do not get fooled by symbolic links, i.e. $HOME != $(pwd)\n> +\t\tHOME_GITCONFIG=$HOME/.gitconfig\n> +\tfi &&\n>  \tcat >expect <<-EOF &&\n>  \tkey=foo.bar\n>  \tvalue=from-home\n>  \torigin=file\n> -\tname=$HOME/.gitconfig\n> +\tname=$HOME_GITCONFIG\n>  \tscope=global\n>  \n>  \tkey=foo.bar\n"},{"id":"291453","messageId":"20160714184021.GD16497@sigill.intra.peff.net","threadId":"42842","inReplyTo":"xmqqr3awry5x.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] mingw: fix regression in t1308-config-set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-14T18:40:21Z","receivedAt":"2016-07-14T18:40:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 14, 2016 at 09:18:18AM -0700, Junio C Hamano wrote:\n\n> I added a few missing Cc: and quoted the whole patch here to those\n> who were involved; I think this update is correct, but just trying\n> to make sure people know.\n> \n> Not limited to this particular topic, there probably are some things\n> we can and should add to the procedure to prevent further episodes\n> like this, but I am not seeing anything immediately obvious offhand.\n> There already is a way to prominently mark a topic to be not-ready\n> with an outstanding issue called \"What's cooking\" report, but it is\n> maintained manually and it can be leaky without extra set of eyes\n> constantly monitoring.\n\nThanks, this fix looks good.\n\nI'm open to to suggestions to make life easier for Windows folks.\nUsually when dealing with paths, the suggestion is to use $(pwd), which\nI did in the original, but as 58461bd noted, that broke other cases.\n\nSo code-wise, maybe this technique could be more general. I.e., could\n$TRASH_DIRECTORY or $HOME already be in whatever format is likely to be\nproduced by git on the platform? Or does that just screw things up more\nbecause Windows sometimes needs one form and sometimes the other?\n\nProcess-wise, I'm not sure. I seem to recall a few times when Windows\nissues have come up in the past that somebody (JSixt?) seemed content to\nlet topics graduate with potential portability problems, and then have\nthe Git for Windows project fix them up separately. These days GfW seems\nto track upstream more closely (which is wonderful!), but it means\nportability problems cause a more immediate headache and slow down that\nprocess.\n\n> > \tSide note: it was not at all clear to me how 58461bd fixed the\n> > \tproblem by replacing $(pwd) with $HOME, given that HOME is set to\n> > \t$TRASH_DIRECTORY which is set to $TEST_OUTPUT_DIRECTORY/... after\n> > \tTEST_OUTPUT_DIRECTORY was set to TEST_DIRECTORY which in turn was\n> > \tset to $(pwd).\n> >\n> > \tI guess the reason is that -P in `cd -P \"$TRASH DIRECTORY\"`, but\n> > \tthen I *really* do not understand how $(pwd) and $PWD could\n> > \tdisagree.\n\nI don't think they did disagree. The issue is that \"cd -P\" caused _both_\nof them to print the physical directory. But git is not using either of\nthem. It is blindly using \"$HOME\". So basically:\n\n  HOME=/path/with/symlinks\n  cd -P \"$HOME\"\n\nwill cause \"$PWD\" and \"$HOME\" to disagree. Likewise with \"$(pwd)\" and\n\"$HOME\".\n\n-Peff\n"}]}