{"thread":{"id":"40219","subject":"[PATCH] t7300: fix broken && chains","startedAt":"2015-08-30T09:18:09Z","lastAt":"2015-09-01T17:36:51Z","messageCount":7,"participants":["Erik Elfström","Junio C Hamano","Jeff King","erik elfström"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"268929","messageId":"1440926289-5899-1-git-send-email-erik.elfstrom@gmail.com","threadId":"40219","inReplyTo":null,"subject":"[PATCH] t7300: fix broken && chains","fromName":"Erik Elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-08-30T09:18:09Z","receivedAt":"2015-08-30T09:18:09Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"While we are here, remove some boilerplate by using test_commit.\n\nSigned-off-by: Erik Elfström \n---\n t/t7300-clean.sh | 16 ++++------------\n 1 file changed, 4 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 27557d6..86ceb38 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -432,9 +432,7 @@ test_expect_success 'nested git work tree' '\n \t(\n \t\tcd foo &&\n \t\tgit init &&\n-\t\t>hello.world\n-\t\tgit add . &&\n-\t\tgit commit -a -m nested\n+\t\ttest_commit nested hello.world\n \t) &&\n \t(\n \t\tcd bar &&\n@@ -443,9 +441,7 @@ test_expect_success 'nested git work tree' '\n \t(\n \t\tcd baz/boo &&\n \t\tgit init &&\n-\t\t>deeper.world\n-\t\tgit add . &&\n-\t\tgit commit -a -m deeply.nested\n+\t\ttest_commit deeply.nested deeper.world\n \t) &&\n \tgit clean -f -d &&\n \ttest -f foo/.git/index &&\n@@ -601,9 +597,7 @@ test_expect_success 'force removal of nested git work tree' '\n \t(\n \t\tcd foo &&\n \t\tgit init &&\n-\t\t>hello.world\n-\t\tgit add . &&\n-\t\tgit commit -a -m nested\n+\t\ttest_commit nested hello.world\n \t) &&\n \t(\n \t\tcd bar &&\n@@ -612,9 +606,7 @@ test_expect_success 'force removal of nested git work tree' '\n \t(\n \t\tcd baz/boo &&\n \t\tgit init &&\n-\t\t>deeper.world\n-\t\tgit add . &&\n-\t\tgit commit -a -m deeply.nested\n+\t\ttest_commit deeply.nested deeper.world\n \t) &&\n \tgit clean -f -f -d &&\n \t! test -d foo &&\n-- \n2.4.4.410.gd6567e3\n"},{"id":"269003","messageId":"xmqqzj17wg1i.fsf@gitster.mtv.corp.google.com","threadId":"40219","inReplyTo":"1440926289-5899-1-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH] t7300: fix broken && chains","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-31T16:58:49Z","receivedAt":"2015-08-31T16:58:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Elfström writes:\n\n> While we are here, remove some boilerplate by using test_commit.\n>\n> Signed-off-by: Erik Elfström \n> ---\n\nMany of the constructs we see here shows clearly that this is an\nancient part of the codebase ;-), as we would be using the one\nparameter form of \"git init\" and more test_* helpers if we were\nwriting this script in today's Git codebase. It may have been\nbetter if you didn't do \"while we are here\" and corrected only the\n&&-chain in patch 1/2 and then updated the style of the tests to\ntake advantage of the newer facilities recent test-lib has in a\nseparate patch 2/2, but this will do at least for now.\n\nWill queue.\n\nThanks.\n\n\n> t/t7300-clean.sh | 16 ++++------------\n> 1 file changed, 4 insertions(+), 12 deletions(-)\n>\n> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> index 27557d6..86ceb38 100755\n> --- a/t/t7300-clean.sh\n> +++ b/t/t7300-clean.sh\n> @@ -432,9 +432,7 @@ test_expect_success 'nested git work tree' '\n> \t(\n> \t\tcd foo &&\n> \t\tgit init &&\n> -\t\t>hello.world\n> -\t\tgit add . &&\n> -\t\tgit commit -a -m nested\n> +\t\ttest_commit nested hello.world\n> \t) &&\n> \t(\n> \t\tcd bar &&\n> @@ -443,9 +441,7 @@ test_expect_success 'nested git work tree' '\n> \t(\n> \t\tcd baz/boo &&\n> \t\tgit init &&\n> -\t\t>deeper.world\n> -\t\tgit add . &&\n> -\t\tgit commit -a -m deeply.nested\n> +\t\ttest_commit deeply.nested deeper.world\n> \t) &&\n> \tgit clean -f -d &&\n> \ttest -f foo/.git/index &&\n> @@ -601,9 +597,7 @@ test_expect_success 'force removal of nested git work tree' '\n> \t(\n> \t\tcd foo &&\n> \t\tgit init &&\n> -\t\t>hello.world\n> -\t\tgit add . &&\n> -\t\tgit commit -a -m nested\n> +\t\ttest_commit nested hello.world\n> \t) &&\n> \t(\n> \t\tcd bar &&\n> @@ -612,9 +606,7 @@ test_expect_success 'force removal of nested git work tree' '\n> \t(\n> \t\tcd baz/boo &&\n> \t\tgit init &&\n> -\t\t>deeper.world\n> -\t\tgit add . &&\n> -\t\tgit commit -a -m deeply.nested\n> +\t\ttest_commit deeply.nested deeper.world\n> \t) &&\n> \tgit clean -f -f -d &&\n> \t! test -d foo &&\n"},{"id":"269022","messageId":"20150831185414.GB20555@sigill.intra.peff.net","threadId":"40219","inReplyTo":"1440926289-5899-1-git-send-email-erik.elfstrom@gmail.com","subject":"Re: [PATCH] t7300: fix broken && chains","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-31T18:54:14Z","receivedAt":"2015-08-31T18:54:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 30, 2015 at 11:18:09AM +0200, Erik Elfström wrote:\n\n> While we are here, remove some boilerplate by using test_commit.\n\nDarn, I thought we were done finding these after flipping on\nGIT_TEST_CHAIN_LINT. :)\n\n> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> index 27557d6..86ceb38 100755\n> --- a/t/t7300-clean.sh\n> +++ b/t/t7300-clean.sh\n> @@ -432,9 +432,7 @@ test_expect_success 'nested git work tree' '\n> \t(\n> \t\tcd foo &&\n> \t\tgit init &&\n> -\t\t>hello.world\n> -\t\tgit add . &&\n> -\t\tgit commit -a -m nested\n> +\t\ttest_commit nested hello.world\n> \t) &&\n\nUnfortunately CHAIN_LINT cannot reach inside a nested subshell. I cannot\nthink of a way to make it do so, and besides, that is also the way to\noverride the &&-chain precedence. So I think it is not really possible\nto get better coverage here. And such cases as these are not really very\ninteresting (e.g., here we exclude only minor minor setup steps from the\n&&-chain).\n\n-Peff\n"},{"id":"269080","messageId":"CAMpP7NZ=K62XVkuuJHpDruBy7b2gECHrJ30L43LzdE54vTVB2A@mail.gmail.com","threadId":"40219","inReplyTo":"20150831185414.GB20555@sigill.intra.peff.net","subject":"Re: [PATCH] t7300: fix broken && chains","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-09-01T06:27:59Z","receivedAt":"2015-09-01T06:27:59Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Mon, Aug 31, 2015 at 8:54 PM, Jeff King wrote:\n> On Sun, Aug 30, 2015 at 11:18:09AM +0200, Erik Elfström wrote:\n>\n> Unfortunately CHAIN_LINT cannot reach inside a nested subshell. I cannot\n> think of a way to make it do so, and besides, that is also the way to\n> override the &&-chain precedence. So I think it is not really possible\n> to get better coverage here. And such cases as these are not really very\n> interesting (e.g., here we exclude only minor minor setup steps from the\n> &&-chain).\n>\n> -Peff\n\nI actually hacked together an ugly script to check for more broken\nchains. It's not working very well and I haven't checked the output\nthoroughly but I did see quite a few broken chains. Many are of the\n(mostly harmless) form:\n\n (\n echo \"100644 $o5 0 a\"\n echo \"100644 $o0 0 c\"\n echo \"160000 $c1 0 d\"\n ) >expected &&\n\nI'd estimate that there are hundreds of these (see t3030 for\nexamples). I'm not sure if you care about these? As you say they are\nnot really very interesting cases.\n\n/Erik\n"},{"id":"269081","messageId":"CAMpP7NZMMYyCbmnzhLTqa73TD_mqk+rO8=D0v9qb0z_UmBQ-rg@mail.gmail.com","threadId":"40219","inReplyTo":"xmqqzj17wg1i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t7300: fix broken && chains","fromName":"erik elfström","fromEmail":"erik.elfstrom@gmail.com","sentAt":"2015-09-01T06:31:37Z","receivedAt":"2015-09-01T06:31:37Z","isPatch":true,"sender":{"key":"erik.elfstrom@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13905184?v=4"},"body":"On Mon, Aug 31, 2015 at 6:58 PM, Junio C Hamano wrote:\n>\n> Many of the constructs we see here shows clearly that this is an\n> ancient part of the codebase ;-), as we would be using the one\n> parameter form of \"git init\" and more test_* helpers if we were\n> writing this script in today's Git codebase. It may have been\n> better if you didn't do \"while we are here\" and corrected only the\n> &&-chain in patch 1/2 and then updated the style of the tests to\n> take advantage of the newer facilities recent test-lib has in a\n> separate patch 2/2, but this will do at least for now.\n>\n> Will queue.\n>\n> Thanks.\n\n\nI can do a re-roll with the chain fix in the first patch and a more\nthorough modernization of t7300 in separate patches if you'd like? I\nalmost went this way for v1 but decided to limit the scope for the\nfirst version.\n\n(Forgot to include the list in my first reply, sorry... And forgot to\nturn off HTML in the second... sigh, sorry for the spam)\n\n/Erik\n"},{"id":"269100","messageId":"xmqqbndmt8fc.fsf@gitster.mtv.corp.google.com","threadId":"40219","inReplyTo":"CAMpP7NZMMYyCbmnzhLTqa73TD_mqk+rO8=D0v9qb0z_UmBQ-rg@mail.gmail.com","subject":"Re: [PATCH] t7300: fix broken && chains","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-01T16:23:51Z","receivedAt":"2015-09-01T16:23:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"erik elfström writes:\n\n> On Mon, Aug 31, 2015 at 6:58 PM, Junio C Hamano wrote:\n>>\n>> ... It may have been\n>> better if you didn't do \"while we are here\" and corrected only the\n>> &&-chain in patch 1/2 and then updated the style of the tests to\n>> take advantage of the newer facilities recent test-lib has in a\n>> separate patch 2/2, but this will do at least for now.\n>>\n>> Will queue.\n>>\n>> Thanks.\n>\n> I can do a re-roll with the chain fix in the first patch and a more\n> thorough modernization of t7300 in separate patches if you'd like? I\n> almost went this way for v1 but decided to limit the scope for the\n> first version.\n\nI'd say it is too much work for something that we already have\nreviewed and queued ;-) Let's have this patch as-is.\n\nModernization and clean-up from time to time is a good thing to do,\nbut we only have limited review bandwidth, so let's not overdo it.\n\nThanks.\n"},{"id":"269111","messageId":"20150901173650.GA5723@sigill.intra.peff.net","threadId":"40219","inReplyTo":"CAMpP7NZ=K62XVkuuJHpDruBy7b2gECHrJ30L43LzdE54vTVB2A@mail.gmail.com","subject":"Re: [PATCH] t7300: fix broken && chains","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-01T17:36:51Z","receivedAt":"2015-09-01T17:36:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 01, 2015 at 08:27:59AM +0200, erik elfström wrote:\n\n> (\n> echo \"100644 $o5 0 a\"\n> echo \"100644 $o0 0 c\"\n> echo \"160000 $c1 0 d\"\n> ) >expected &&\n> \n> I'd estimate that there are hundreds of these (see t3030 for\n> examples). I'm not sure if you care about these? As you say they are\n> not really very interesting cases.\n\nI think patches for these are OK, but no, I don't consider it a high\npriority if they are harmless (IMHO the real value of the patches is\nthat it removes the noise from the output of your script, so you can\nfind any cases that _do_ matter).\n\n-Peff\n"}]}