{"thread":{"id":"41366","subject":"[PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh","startedAt":"2016-02-07T19:11:37Z","lastAt":"2016-02-09T17:30:21Z","messageCount":13,"participants":["Michael J Gruber","Jeff King","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"277700","messageId":"982f6f499c988e1063275e2951c9856d622a83f3.1454872161.git.git@drmicha.warpmail.net","threadId":"41366","inReplyTo":null,"subject":"[PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2016-02-07T19:11:37Z","receivedAt":"2016-02-07T19:11:37Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"bcb11f1 (mingw: mark t9100's test cases with appropriate prereqs, 2016-01-27)\nreplaced \"/bin/sh\" in exec.sh by the shell specified in SHELL_PATH, but\nthat breaks the subtest which checks for a specific checksum of a tree\ncontaining.\n\nRevert that change that was not explained in the commit message anyways\n(exec.sh is never executed).\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\n\n t/t9100-git-svn-basic.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh\nindex 5464b5b..936913c 100755\n--- a/t/t9100-git-svn-basic.sh\n+++ b/t/t9100-git-svn-basic.sh\n@@ -30,7 +30,8 @@ test_expect_success \\\n \t\techo \"deep dir\" >dir/a/b/c/d/e/file &&\n \t\tmkdir bar &&\n \t\techo \"zzz\" >bar/zzz &&\n-\t\twrite_script exec.sh </dev/null &&\n+\t\techo \"#!/bin/sh\" >exec.sh &&\n+\t\tchmod +x exec.sh &&\n \t\tsvn_cmd import -m \"import for git svn\" . \"$svnrepo\" >/dev/null\n \t) &&\n \trm -rf import &&\n-- \n2.7.0.373.g083f1fe\n"},{"id":"277727","messageId":"20160208135013.GA27054@sigill.intra.peff.net","threadId":"41366","inReplyTo":"982f6f499c988e1063275e2951c9856d622a83f3.1454872161.git.git@drmicha.warpmail.net","subject":"Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-08T13:50:13Z","receivedAt":"2016-02-08T13:50:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 07, 2016 at 08:11:37PM +0100, Michael J Gruber wrote:\n\n> bcb11f1 (mingw: mark t9100's test cases with appropriate prereqs, 2016-01-27)\n> replaced \"/bin/sh\" in exec.sh by the shell specified in SHELL_PATH, but\n> that breaks the subtest which checks for a specific checksum of a tree\n> containing.\n> \n> Revert that change that was not explained in the commit message anyways\n> (exec.sh is never executed).\n\nI think this just re-breaks things on Windows. That first setup test\nused \"chmod +x\" (which is brought back by your patch), without having\nthe POSIXPERM prerequisite.\n\nWe probably do not want to mark the whole setup test as POSIXPERM, as\nthat would effectively break all of the other tests on Windows. The rest\nof the tests need to be able to work whether or not the \"chmod +x\" was\nrun. It may be simpler to just break the executable-bit tests, including\nsetup, out to their own section of the script.\n\nThat being said, t9100 seems to pass for me, even at bcb11f1. Can you\nshow us the breakage you are seeing?\n\n-Peff\n"},{"id":"277743","messageId":"CAA19uiRsdc0eyZDqGtjpAcLyn=hyO4DXbhDbd=hoj8OxK2GK3A@mail.gmail.com","threadId":"41366","inReplyTo":"20160208135013.GA27054@sigill.intra.peff.net","subject":"Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2016-02-08T16:34:14Z","receivedAt":"2016-02-08T16:34:14Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"[warning: experimenting with forwarding to and replying from gmail...]\n2016-02-08 14:50 GMT+01:00 Jeff King <peff@peff.net>:\n> On Sun, Feb 07, 2016 at 08:11:37PM +0100, Michael J Gruber wrote:\n>\n>> bcb11f1 (mingw: mark t9100's test cases with appropriate prereqs, 2016-01-27)\n>> replaced \"/bin/sh\" in exec.sh by the shell specified in SHELL_PATH, but\n>> that breaks the subtest which checks for a specific checksum of a tree\n>> containing.\n>>\n>> Revert that change that was not explained in the commit message anyways\n>> (exec.sh is never executed).\n>\n> I think this just re-breaks things on Windows. That first setup test\n> used \"chmod +x\" (which is brought back by your patch), without having\n> the POSIXPERM prerequisite.\n>\n> We probably do not want to mark the whole setup test as POSIXPERM, as\n> that would effectively break all of the other tests on Windows. The rest\n> of the tests need to be able to work whether or not the \"chmod +x\" was\n> run. It may be simpler to just break the executable-bit tests, including\n> setup, out to their own section of the script.\n>\n\nThe commit message does not explain that part of the patch at all - to\nme it looks as if the direct \"echo\" and \"chmod +x\" is simply replaced\nby calling a function which does just that, or more exactly, not quite:\n\n> That being said, t9100 seems to pass for me, even at bcb11f1. Can you\n> show us the breakage you are seeing?\n>\n> -Peff\n\nSHELL_PATH=/bin/dash (in config.mak)\n\nAs I explained in my commit message, the problem arises when\nSHELL_PATH is not \"/bin/sh\" and, consequently,\nthe generated \"exec.sh\" results in a blob with a different sha1.\n\nJust try \"/usr/bin/sh\" for good measure...\n\nMichael\n\n[This time plain text, hopefully, and thus vger-palatable. How do I\nmake this default...]\n"},{"id":"277744","messageId":"20160208163700.GA22929@sigill.intra.peff.net","threadId":"41366","inReplyTo":"CAA19uiRSu_6Os3b498obSNec7b2uiYv20SZ=y93CkjsWqhqHzA@mail.gmail.com","subject":"Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-08T16:37:00Z","receivedAt":"2016-02-08T16:37:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2016 at 05:27:30PM +0100, Michael J Gruber wrote:\n\n> > I think this just re-breaks things on Windows. That first setup test\n> > used \"chmod +x\" (which is brought back by your patch), without having\n> > the POSIXPERM prerequisite.\n> >\n> > We probably do not want to mark the whole setup test as POSIXPERM, as\n> > that would effectively break all of the other tests on Windows. The rest\n> > of the tests need to be able to work whether or not the \"chmod +x\" was\n> > run. It may be simpler to just break the executable-bit tests, including\n> > setup, out to their own section of the script.\n> >\n> The commit message does not explain that part of the patch at all - to me\n> it looks as if the direct \"echo\" and \"chmod +x\" is simply replaced\n> by calling a function which does just that, or more exactly, not quite:\n\nAh, right. I figured that systems that don't handle `chmod +x` would\nomit it from write_script(). But it looks like we don't. I guess the\nlogic is that on Windows \"chmod +x\" doesn't _complain_, it's simply a\nnoop for adding the file to the index (because we unset core.filemode).\n\nSo in that sense, Windows is fine with that setup either way.\n\nI wondered why it would not later fail the same sha1 check, since \"git\nadd\" would not respect the executable bit on such a system. But the\nanswer is that we do not \"git add\" the result; we import it using svn,\nand then convert that to a git tree.\n\n> > That being said, t9100 seems to pass for me, even at bcb11f1. Can you\n> > show us the breakage you are seeing?\n> \n> SHELL_PATH=/bin/dash (in config.mak)\n> \n> As I explained in my commit message, the problem arises when SHELL_PATH is\n> not \"/bin/sh\" and, consequently,\n> the generated \"exec.sh\" results in a blob with a different sha1.\n\nOh, of course. I forgot that my SHELL_PATH is in fact /bin/sh. Sorry for\nbeing thick.\n\nAssuming your patch works on Windows (and from the logic above, I think\nit should be the case?), then I think it's a good solution.\n\n-Peff\n"},{"id":"277754","messageId":"alpine.DEB.2.20.1602082027550.2964@virtualbox","threadId":"41366","inReplyTo":"20160208163700.GA22929@sigill.intra.peff.net","subject":"Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-08T19:31:54Z","receivedAt":"2016-02-08T19:31:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 8 Feb 2016, Jeff King wrote:\n\n> Assuming your patch works on Windows\n\nIf it re-introduces that chmod +x, it won't.\n\nPlease note that my *original* patch actually only guarded the chmod +x,\nbut Junio suggested switching to write_script and since it passed the test\nsuite here, I though it would be safe.\n\nI still think write_script is the better alternative.\n\nSo why not just prefix it with `SHELL_PATH=/bin/sh`?\n\n-- snipsnap --\nt a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh\nindex 56acc1e..6ad8537 100755\n--- a/t/t9100-git-svn-basic.sh\n+++ b/t/t9100-git-svn-basic.sh\n@@ -30,7 +30,7 @@ test_expect_success \\\n \t\techo \"deep dir\" >dir/a/b/c/d/e/file &&\n \t\tmkdir bar &&\n \t\techo \"zzz\" >bar/zzz &&\n-\t\twrite_script exec.sh </dev/null &&\n+\t\tSHELL_PATH=/bin/sh write_script exec.sh </dev/null &&\n \t\tsvn_cmd import -m \"import for git svn\" . \"$svnrepo\" >/dev/null\n \t) &&\n \trm -rf import &&\n"},{"id":"277755","messageId":"20160208193509.GA30554@sigill.intra.peff.net","threadId":"41366","inReplyTo":"alpine.DEB.2.20.1602082027550.2964@virtualbox","subject":"Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-08T19:35:09Z","receivedAt":"2016-02-08T19:35:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2016 at 08:31:54PM +0100, Johannes Schindelin wrote:\n\n> On Mon, 8 Feb 2016, Jeff King wrote:\n> \n> > Assuming your patch works on Windows\n> \n> If it re-introduces that chmod +x, it won't.\n> \n> Please note that my *original* patch actually only guarded the chmod +x,\n> but Junio suggested switching to write_script and since it passed the test\n> suite here, I though it would be safe.\n> \n> I still think write_script is the better alternative.\n\nI'm confused why it matters. write_script() unconditionally calls \"chmod\n+x\", doesn't it?\n\nI just double-checked its definition in test-lib-function.sh; am I\nmissing some Windows-specific magic that kicks in?\n\n> So why not just prefix it with `SHELL_PATH=/bin/sh`?\n\nBut then what is write_script buying us?\n\n-Peff\n"},{"id":"277757","messageId":"xmqqlh6v7zu0.fsf@gitster.mtv.corp.google.com","threadId":"41366","inReplyTo":"20160208193509.GA30554@sigill.intra.peff.net","subject":"Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-08T19:43:19Z","receivedAt":"2016-02-08T19:43:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'm confused why it matters. write_script() unconditionally calls \"chmod\n> +x\", doesn't it?\n\nYeah, that was exactly my thought, too.  Sorry for not noticing that\nthis depended the \"interpreter\" exactly be /bin/sh, though (it is\nnot even executed).\n\n> I just double-checked its definition in test-lib-function.sh; am I\n> missing some Windows-specific magic that kicks in?\n>\n>> So why not just prefix it with `SHELL_PATH=/bin/sh`?\n>\n> But then what is write_script buying us?\n\nThe correct way to write a script for a specific interpreter is to\ngive a second parameter to write_script, i.e.\n\n\t\twrite_script exec.sh /bin/sh </dev/null &&\n\nand the answer to the question is \"it will save us one line\".\n\nThe version in 'master' that does\n\n                echo \"#!/bin/sh\" >exec.sh &&\n                chmod +x exec.sh &&\n\nshould be equivalent, so dropping that hunk from the patch is the\nright resolution perhaps?\n"},{"id":"277760","messageId":"20160208195630.GB30693@sigill.intra.peff.net","threadId":"41366","inReplyTo":"xmqqlh6v7zu0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-08T19:56:30Z","receivedAt":"2016-02-08T19:56:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2016 at 11:43:19AM -0800, Junio C Hamano wrote:\n\n> > But then what is write_script buying us?\n> \n> The correct way to write a script for a specific interpreter is to\n> give a second parameter to write_script, i.e.\n> \n> \t\twrite_script exec.sh /bin/sh </dev/null &&\n> \n> and the answer to the question is \"it will save us one line\".\n\nAt the cost of a useless \"cat\" invocation, though. :)\n\n> The version in 'master' that does\n> \n>                 echo \"#!/bin/sh\" >exec.sh &&\n>                 chmod +x exec.sh &&\n> \n> should be equivalent, so dropping that hunk from the patch is the\n> right resolution perhaps?\n\nYeah, but I still don't understand why the original did not work on\nWindows, once all the other hunks from bcb11f1 are applied.\n\n-Peff\n"},{"id":"277761","messageId":"alpine.DEB.2.20.1602082056410.2964@virtualbox","threadId":"41366","inReplyTo":"20160208193509.GA30554@sigill.intra.peff.net","subject":"Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-08T19:59:31Z","receivedAt":"2016-02-08T19:59:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 8 Feb 2016, Jeff King wrote:\n\n> On Mon, Feb 08, 2016 at 08:31:54PM +0100, Johannes Schindelin wrote:\n> \n> > On Mon, 8 Feb 2016, Jeff King wrote:\n> > \n> > > Assuming your patch works on Windows\n> > \n> > If it re-introduces that chmod +x, it won't.\n> > \n> > Please note that my *original* patch actually only guarded the chmod +x,\n> > but Junio suggested switching to write_script and since it passed the test\n> > suite here, I though it would be safe.\n> > \n> > I still think write_script is the better alternative.\n> \n> I'm confused why it matters. write_script() unconditionally calls \"chmod\n> +x\", doesn't it?\n\nHmpf, you're right. I'll check tomorrow what's going wrong.\n\n> I just double-checked its definition in test-lib-function.sh; am I\n> missing some Windows-specific magic that kicks in?\n\nNo Windows magic I know of. But actually, the patch could be simplified to\n\n-- snip --\ndiff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh\nindex 56acc1e..ee85cc7 100755\n--- a/t/t9100-git-svn-basic.sh\n+++ b/t/t9100-git-svn-basic.sh\n@@ -30,7 +30,7 @@ test_expect_success \\\n \t\techo \"deep dir\" >dir/a/b/c/d/e/file &&\n \t\tmkdir bar &&\n \t\techo \"zzz\" >bar/zzz &&\n-\t\twrite_script exec.sh </dev/null &&\n+\t\twrite_script exec.sh /bin/sh </dev/null &&\n \t\tsvn_cmd import -m \"import for git svn\" . \"$svnrepo\" >/dev/null\n \t) &&\n \trm -rf import &&\n-- snap --\n\n> > So why not just prefix it with `SHELL_PATH=/bin/sh`?\n> \n> But then what is write_script buying us?\n\nwrite_script is a semantically unambiguous way to specify what we *want*.\nAnd it would allow us to handle chmod specifically for Windows *in one\nplace only*.\n\nBut as I said, I have to investigate what's going on.\n\nCiao,\nDscho\n"},{"id":"277762","messageId":"xmqqd1s77yhg.fsf@gitster.mtv.corp.google.com","threadId":"41366","inReplyTo":"alpine.DEB.2.20.1602082056410.2964@virtualbox","subject":"Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-08T20:12:27Z","receivedAt":"2016-02-08T20:12:27Z","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> write_script is a semantically unambiguous way to specify what we *want*.\n> And it would allow us to handle chmod specifically for Windows *in one\n> place only*.\n\nCorrect.  write_script, for the intended target of the helper, is a\nway to write a script that can later be invoked by the test with the\nname \"$1\".  It is conceivable for write_script on UNIX to be writing\ninto \"$1\" while Windows version to be writing into \"$1.bat\" and the\nscript, i.e. the user of the write_script helper, to do this\n\n\twrite_script foo <<EOF &&\n        ...\n        EOF\n\t...\n        foo\n\nwhich may result in foo.bat running on Windows without us having to\nadjust the test script.  So it indeed is a very nice abstraction to\nhave.\n\nBut the way the test uses this exec.sh script is not consistent with\nthat.  exec.sh for this test is merely a data, whose content must\nexactly match what later tests expect, i.e. it wants it to begin\nwith \"#!/bin/sh\" and its execute bit on, even though the test does\nnot have no intention to run it as a script.\n\nSo I think it was doubly wrong for me to suggest write_script\nwithout realizing that this is _not_ writing a script in the usual\nsense for us to write with write_script.\n\n\n\n        \t\n"},{"id":"277796","messageId":"alpine.DEB.2.20.1602091054430.2964@virtualbox","threadId":"41366","inReplyTo":"20160208195630.GB30693@sigill.intra.peff.net","subject":"Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-09T10:00:43Z","receivedAt":"2016-02-09T10:00:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff (and other interested parties),\n\nOn Mon, 8 Feb 2016, Jeff King wrote:\n\n> On Mon, Feb 08, 2016 at 11:43:19AM -0800, Junio C Hamano wrote:\n> \n> > The version in 'master' that does\n> > \n> >                 echo \"#!/bin/sh\" >exec.sh &&\n> >                 chmod +x exec.sh &&\n> > \n> > should be equivalent, so dropping that hunk from the patch is the\n> > right resolution perhaps?\n> \n> Yeah, but I still don't understand why the original did not work on\n> Windows, once all the other hunks from bcb11f1 are applied.\n\nAnd indeed it passes. With MSYS2. Because it simply ignores that chmod +x\ncannot flip an executable bit.\n\nThe original patch (the one that guarded the chmod behind the MINGW\nprereq) originated in the MSys (AKA MSys1) times, where 1) chmod would\nfail, and 2) POSIXPERM did not yet exist.\n\nBesides, I am pretty certain that there is a test in t9100 that *does*\ntest the executable bit, properly requiring POSIXPERM.\n\nSo I still would be in favor of using write_script: 1) our *intention* is\nto write a script, even if we do not currently execute it, and 2) if\nanybody is interested in supporting MSys1 (*cough* Hannes & Sebastian\n*cough*), they have a *much* easier time to fix it.\n\nCiao,\nDscho\n"},{"id":"277798","messageId":"alpine.DEB.2.20.1602091101250.2964@virtualbox","threadId":"41366","inReplyTo":"xmqqd1s77yhg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-09T10:07:40Z","receivedAt":"2016-02-09T10:07:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 8 Feb 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > write_script is a semantically unambiguous way to specify what we *want*.\n> > And it would allow us to handle chmod specifically for Windows *in one\n> > place only*.\n> \n> Correct.  write_script, for the intended target of the helper, is a\n> way to write a script that can later be invoked by the test with the\n> name \"$1\".\n\nAnd whose executable bit is set, contingent on the POSIXPERM prereq.\n\n> It is conceivable for write_script on UNIX to be writing\n> into \"$1\" while Windows version to be writing into \"$1.bat\"\n\nOy vey. Good thing you did not see my first reaction.\n\nShell scripts and batch scripts have *very* different semantics. Therefore\nit would be a major nightmare (for me, not for you) to support writing\nthem *using the same write_script invocation*.\n\nLet's just not go there.\n\n> But the way the test uses this exec.sh script is not consistent with\n> that.  exec.sh for this test is merely a data, whose content must\n> exactly match what later tests expect, i.e. it wants it to begin with\n> \"#!/bin/sh\" and its execute bit on, even though the test does not have\n> no intention to run it as a script.\n\nThe important part, of course, is \"and its execute bit on\" which makes it\na moot point to ask whether we intend to execute the script or not. A\nscript is what we want, and a script is what we write. Therefore,\nwrite_script is what we call. With the $2 fix-up to keep DrMicha happy.\n\nCiao,\nDscho\n"},{"id":"277803","messageId":"xmqqwpqd4wr6.fsf@gitster.mtv.corp.google.com","threadId":"41366","inReplyTo":"alpine.DEB.2.20.1602091054430.2964@virtualbox","subject":"Re: [PATCH] t9100: fix breakage when SHELL_PATH is not /bin/sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-09T17:30:21Z","receivedAt":"2016-02-09T17:30:21Z","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> Besides, I am pretty certain that there is a test in t9100 that *does*\n> test the executable bit, properly requiring POSIXPERM.\n>\n> So I still would be in favor of using write_script: 1) our *intention* is\n> to write a script, even if we do not currently execute it, and 2) if\n> anybody is interested in supporting MSys1 (*cough* Hannes & Sebastian\n> *cough*), they have a *much* easier time to fix it.\n\nI do not think our intention is to write a script there, though.\nYou have commented out a different test, where a file called \"file\"\nwhose executable bit is flipped, and the result of the test depends\non the filesystem actually flipping the bit, using POSIXPERM.  I do\nnot see anything more special in the way \"exec.sh\" is used in the\ntest compared to the use of that \"file\".  They both are merely used\nas a payload, with expected contents and expected setting of the\nexecutable bit at each points in the test sequence.\n\nBy using POSIXPERM, js/mingw-tests topic cleanly identifies and\nskips the tests that rely on the \"chmod +x\" acting as expected, but\nby hiding the creation and setting of the executable bit behind\nwrite_script for \"exec.sh\" (and for this file alone), the reader is\nstill forced to know the subtle linkage between write_script and\npermission bits.  On a system where executable bit is not needed to\nexecute a script (perhaps the file extension tells if a file is\nexecutable on such a system), it is a plausible enhancement to\nwrite_script to make it not to even attempt \"chmod +x\" on a system\nwithout executable bit--after all, the helper _is_ about writing a\nscript.  The executable bit is an implementation detail that nobody\ncares about.\n\nBut the way \"exec.sh\" is used by that test is different.  It merely\nwants to have a file with that name with a fixed contents that has\nexecutable bit set.  As I already said, I didn't notice that during\nthe review and incorrectly suggested use of write_script, but that\nwas a mistake.\n\nIdeally, it would have been better if the test was structured in\nsuch a way that a set of pure-contents tests that do not involve the\nexecutable bit and symbolic links are done first, with separate set\nof tests that require POSIXPERM and/or SYMLINK.  Then we wouldn't be\nhaving this conversation.  You would skip the whole thing in the\nlatter category.  As the test that originally was written needs\nPOSIXPERM (but curiously not SYMLINK) even in the very initial\n'setup' stage, a hack to make the set-up step behave differently\ndepending on POSIXPERM is unavoidable and tolerable, if we do not\nwant to rewrite the entire script (and I do not think neither of us\nwant to see that).\n\nBut that needs to be spelled out explicitly to allow people follow\nwhat is going on more easily, e.g.\n\n    echo \"#!/bin/sh\" >exec.sh &&\n    { test_have_prereq !POSIXPERM || chmod +x exec.sh } &&\n\nThe \"file\" test would not need such a construct as the whole thing\nis skipped without POSIXPERM (and SYMLINK where it is aliased to\nexec-2.sh).\n\nWhile reviewing the change to this script, another thing I noticed\nis this bit:\n\n    name='remove executable bit from a file'\n    test_expect_success POSIXPERM \"$name\" '\n            rm -f \"$GIT_DIR\"/index &&\n            git checkout -f -b mybranch5 ${remotes_git_svn} &&\n            chmod -x exec.sh &&\n            git update-index exec.sh &&\n            git commit -m \"$name\" &&\n            git svn set-tree --find-copies-harder --rmdir \\\n                    ${remotes_git_svn}..mybranch5 &&\n            svn_cmd up \"$SVN_TREE\" &&\n            test ! -x \"$SVN_TREE\"/exec.sh'\n\nThis uses \"chmod -x\", but it does not need to.  The only thing it\ncares about is that the tree object that contains exec.sh record\nthat path with executable bit off, so \"update-index --chmod\" would\nhave allowed a !POSIXPERM system to record the same result.\n\nThere probably are more instance of similar \"chmod\" that you do not\nhave to skip with !POSIXPERM.\n"}]}