{"thread":{"id":"42669","subject":"[PATCH] t5614: don't use subshells","startedAt":"2016-06-20T17:22:14Z","lastAt":"2016-06-21T20:22:26Z","messageCount":4,"participants":["Stefan Beller","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"289687","messageId":"1466443278-21591-1-git-send-email-sbeller@google.com","threadId":"42669","inReplyTo":null,"subject":"[PATCH] t5614: don't use subshells","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-20T17:21:18Z","receivedAt":"2016-06-20T17:22:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Using a subshell for just one git command is both a waste in compute\noverhead (create a new process) as well as in line count.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n Junio,\n\n This goes on top of the patch that I just sent\n \"[PATCH] shallow clone to not imply shallow submodules\" (which is to\n be squashed into \"clone: do not let --depth imply --shallow-submodules\".\n \n Unlike the prior patch we would not want this patch to fall through\n to origin/maint fast, but allow cooking?\n \n Thanks,\n Stefan\n\n t/t5614-clone-submodules.sh | 70 +++++++++++++--------------------------------\n 1 file changed, 20 insertions(+), 50 deletions(-)\n\ndiff --git a/t/t5614-clone-submodules.sh b/t/t5614-clone-submodules.sh\nindex a9aaa01..da2a67f 100755\n--- a/t/t5614-clone-submodules.sh\n+++ b/t/t5614-clone-submodules.sh\n@@ -25,76 +25,46 @@ test_expect_success 'setup' '\n test_expect_success 'nonshallow clone implies nonshallow submodule' '\n \ttest_when_finished \"rm -rf super_clone\" &&\n \tgit clone --recurse-submodules \"file://$pwd/.\" super_clone &&\n-\t(\n-\t\tcd super_clone &&\n-\t\tgit log --oneline >lines &&\n-\t\ttest_line_count = 3 lines\n-\t) &&\n-\t(\n-\t\tcd super_clone/sub &&\n-\t\tgit log --oneline >lines &&\n-\t\ttest_line_count = 3 lines\n-\t)\n+\tgit -C super_clone log --oneline >lines &&\n+\ttest_line_count = 3 lines &&\n+\tgit -C super_clone/sub log --oneline >lines &&\n+\ttest_line_count = 3 lines\n '\n \n test_expect_success 'shallow clone with shallow submodule' '\n \ttest_when_finished \"rm -rf super_clone\" &&\n \tgit clone --recurse-submodules --depth 2 --shallow-submodules \"file://$pwd/.\" super_clone &&\n-\t(\n-\t\tcd super_clone &&\n-\t\tgit log --oneline >lines &&\n-\t\ttest_line_count = 2 lines\n-\t) &&\n-\t(\n-\t\tcd super_clone/sub &&\n-\t\tgit log --oneline >lines &&\n-\t\ttest_line_count = 1 lines\n-\t)\n+\tgit -C super_clone log --oneline >lines &&\n+\ttest_line_count = 2 lines &&\n+\tgit -C super_clone/sub log --oneline >lines &&\n+\ttest_line_count = 1 lines\n '\n \n test_expect_success 'shallow clone does not imply shallow submodule' '\n \ttest_when_finished \"rm -rf super_clone\" &&\n \tgit clone --recurse-submodules --depth 2 \"file://$pwd/.\" super_clone &&\n-\t(\n-\t\tcd super_clone &&\n-\t\tgit log --oneline >lines &&\n-\t\ttest_line_count = 2 lines\n-\t) &&\n-\t(\n-\t\tcd super_clone/sub &&\n-\t\tgit log --oneline >lines &&\n-\t\ttest_line_count = 3 lines\n-\t)\n+\tgit -C super_clone log --oneline >lines &&\n+\ttest_line_count = 2 lines &&\n+\tgit -C super_clone/sub log --oneline >lines &&\n+\ttest_line_count = 3 lines\n '\n \n test_expect_success 'shallow clone with non shallow submodule' '\n \ttest_when_finished \"rm -rf super_clone\" &&\n \tgit clone --recurse-submodules --depth 2 --no-shallow-submodules \"file://$pwd/.\" super_clone &&\n-\t(\n-\t\tcd super_clone &&\n-\t\tgit log --oneline >lines &&\n-\t\ttest_line_count = 2 lines\n-\t) &&\n-\t(\n-\t\tcd super_clone/sub &&\n-\t\tgit log --oneline >lines &&\n-\t\ttest_line_count = 3 lines\n-\t)\n+\tgit -C super_clone log --oneline >lines &&\n+\ttest_line_count = 2 lines &&\n+\tgit -C super_clone/sub log --oneline >lines &&\n+\ttest_line_count = 3 lines\n '\n \n test_expect_success 'non shallow clone with shallow submodule' '\n \ttest_when_finished \"rm -rf super_clone\" &&\n \tgit clone --recurse-submodules --no-local --shallow-submodules \"file://$pwd/.\" super_clone &&\n-\t(\n-\t\tcd super_clone &&\n-\t\tgit log --oneline >lines &&\n-\t\ttest_line_count = 3 lines\n-\t) &&\n-\t(\n-\t\tcd super_clone/sub &&\n-\t\tgit log --oneline >lines &&\n-\t\ttest_line_count = 1 lines\n-\t)\n+\tgit -C super_clone log --oneline >lines &&\n+\ttest_line_count = 3 lines &&\n+\tgit -C super_clone/sub log --oneline >lines &&\n+\ttest_line_count = 1 lines\n '\n \n test_done\n-- \n2.7.0.rc0.40.g5328432.dirty\n\n"},{"id":"289797","messageId":"xmqqh9cmpcze.fsf@gitster.mtv.corp.google.com","threadId":"42669","inReplyTo":"1466443278-21591-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] t5614: don't use subshells","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-21T19:18:29Z","receivedAt":"2016-06-21T19:18:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>  Unlike the prior patch we would not want this patch to fall through\n>  to origin/maint fast, but allow cooking?\n\nI do not see anything that makes this treated differently from the\nother fix.  The only difference in behaviour is that \"lines\" file is\nnow created at the root level of the trash repository's working tree\ninstead of tested clones', and I do not think any test depends on\nthe number of untracked paths in the trash working tree or tries to\nmake sure a file called \"lines\" is not in there.\n\n> diff --git a/t/t5614-clone-submodules.sh b/t/t5614-clone-submodules.sh\n> index a9aaa01..da2a67f 100755\n> --- a/t/t5614-clone-submodules.sh\n> +++ b/t/t5614-clone-submodules.sh\n> @@ -25,76 +25,46 @@ test_expect_success 'setup' '\n>  test_expect_success 'nonshallow clone implies nonshallow submodule' '\n>  \ttest_when_finished \"rm -rf super_clone\" &&\n>  \tgit clone --recurse-submodules \"file://$pwd/.\" super_clone &&\n> -\t(\n> -\t\tcd super_clone &&\n> -\t\tgit log --oneline >lines &&\n> -\t\ttest_line_count = 3 lines\n> -\t) &&\n> -\t(\n> -\t\tcd super_clone/sub &&\n> -\t\tgit log --oneline >lines &&\n> -\t\ttest_line_count = 3 lines\n> -\t)\n> +\tgit -C super_clone log --oneline >lines &&\n> +\ttest_line_count = 3 lines &&\n> +\tgit -C super_clone/sub log --oneline >lines &&\n> +\ttest_line_count = 3 lines\n>  '\n\nHaving said that, I wonder if we want further reduction of the\nrepetition.  Each test except \"setup\" in this script does an\nidentical thing with very small set of parameters:\n\n    - make sure super_clone will be removed when done.\n    - clone file://$pwd/. to super_clone but with different set of options.\n    - check the commits in super_clone and super_clone/sub.\n\nSo, the above would ideally become something like\n\ndo_test 3 3 --recurse-submodules\n\nwhere the helper would look like\n\ndo_test () {\n\tcnt_super=$1 cnt_sub=$2 &&\n        shift 2 &&\n        test_when_finished \"rm -fr super_clone\" &&\n        git clone \"$@\" \"file://$pwd/.\" super_clone &&\n        git -C super_clone log --oneline >lines &&\n        test_line_count = $cnt_super lines &&\n        git -C super_clone/sub log --oneline >lines &&\n        test_line_count = $cnt_sub lines\n}\n\nWould it rob too much flexibility from future tests to be added to\nthis script if we did it that way?\n\n"},{"id":"289801","messageId":"20160621193824.GA22183@sigill.intra.peff.net","threadId":"42669","inReplyTo":"xmqqh9cmpcze.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t5614: don't use subshells","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-21T19:38:24Z","receivedAt":"2016-06-21T19:39:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 21, 2016 at 12:18:29PM -0700, Junio C Hamano wrote:\n\n> Stefan Beller <sbeller@google.com> writes:\n> \n> >  Unlike the prior patch we would not want this patch to fall through\n> >  to origin/maint fast, but allow cooking?\n> \n> I do not see anything that makes this treated differently from the\n> other fix.  The only difference in behaviour is that \"lines\" file is\n> now created at the root level of the trash repository's working tree\n> instead of tested clones', and I do not think any test depends on\n> the number of untracked paths in the trash working tree or tries to\n> make sure a file called \"lines\" is not in there.\n\nI think it is only that the other patch is actually fixing something,\nwhereas this is cleanup. So the cost/benefit equation is different. I\nagree neither is high-risk and a test cleanup is generally OK for maint\n(the other is a serious-ish regression IMHO).\n\n> Having said that, I wonder if we want further reduction of the\n> repetition.  Each test except \"setup\" in this script does an\n> identical thing with very small set of parameters:\n> \n>     - make sure super_clone will be removed when done.\n>     - clone file://$pwd/. to super_clone but with different set of options.\n>     - check the commits in super_clone and super_clone/sub.\n> \n> So, the above would ideally become something like\n> \n> do_test 3 3 --recurse-submodules\n> \n> where the helper would look like\n> \n> do_test () {\n> \tcnt_super=$1 cnt_sub=$2 &&\n>         shift 2 &&\n>         test_when_finished \"rm -fr super_clone\" &&\n>         git clone \"$@\" \"file://$pwd/.\" super_clone &&\n>         git -C super_clone log --oneline >lines &&\n>         test_line_count = $cnt_super lines &&\n>         git -C super_clone/sub log --oneline >lines &&\n>         test_line_count = $cnt_sub lines\n> }\n> \n> Would it rob too much flexibility from future tests to be added to\n> this script if we did it that way?\n\nI think that's an improvement, too. Even if we add further tests, they\ndon't have to follow the same format. I would give the function a better\nname than \"do_test\" though. :P\n\n-Peff\n"},{"id":"289803","messageId":"CAGZ79kZ4nVeJ6qi9LED-HOxYeP6Scg9j7+nYa4yJYyFxFT-WEA@mail.gmail.com","threadId":"42669","inReplyTo":"20160621193824.GA22183@sigill.intra.peff.net","subject":"Re: [PATCH] t5614: don't use subshells","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-06-21T20:14:17Z","receivedAt":"2016-06-21T20:22:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jun 21, 2016 at 12:38 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Jun 21, 2016 at 12:18:29PM -0700, Junio C Hamano wrote:\n>\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>> >  Unlike the prior patch we would not want this patch to fall through\n>> >  to origin/maint fast, but allow cooking?\n>>\n>> I do not see anything that makes this treated differently from the\n>> other fix.  The only difference in behaviour is that \"lines\" file is\n>> now created at the root level of the trash repository's working tree\n>> instead of tested clones', and I do not think any test depends on\n>> the number of untracked paths in the trash working tree or tries to\n>> make sure a file called \"lines\" is not in there.\n>\n> I think it is only that the other patch is actually fixing something,\n> whereas this is cleanup. So the cost/benefit equation is different. I\n> agree neither is high-risk and a test cleanup is generally OK for maint\n> (the other is a serious-ish regression IMHO).\n\nI agree on the cost/benefit equation being different. I considered this patch\na \"normal risk\" thing, which by our standards is not going through to maint\ndirectly, but is cooked at various heat levels before.\n\n>\n>> Having said that, I wonder if we want further reduction of the\n>> repetition.  Each test except \"setup\" in this script does an\n>> identical thing with very small set of parameters:\n>>\n>>     - make sure super_clone will be removed when done.\n>>     - clone file://$pwd/. to super_clone but with different set of options.\n>>     - check the commits in super_clone and super_clone/sub.\n>>\n>> So, the above would ideally become something like\n>>\n>> do_test 3 3 --recurse-submodules\n>>\n>> where the helper would look like\n>>\n>> do_test () {\n>>       cnt_super=$1 cnt_sub=$2 &&\n>>         shift 2 &&\n>>         test_when_finished \"rm -fr super_clone\" &&\n>>         git clone \"$@\" \"file://$pwd/.\" super_clone &&\n>>         git -C super_clone log --oneline >lines &&\n>>         test_line_count = $cnt_super lines &&\n>>         git -C super_clone/sub log --oneline >lines &&\n>>         test_line_count = $cnt_sub lines\n>> }\n>>\n>> Would it rob too much flexibility from future tests to be added to\n>> this script if we did it that way?\n>\n> I think that's an improvement, too. Even if we add further tests, they\n> don't have to follow the same format. I would give the function a better\n> name than \"do_test\" though. :P\n\nI thought about implementing this, but I was unsure how much flexibility it\nis robbing, as we don't know in which direction we're going to extend the\ntests if at all. (Are we testing more options or do we want to inject more\ncommands like \"git submodule update --keep-configured-depth\" ?\nThat kept me away from doing the super short do_test)\n\nThanks,\nStefan\n\n\n>\n> -Peff\n"}]}