{"thread":{"id":"45306","subject":"[PATCH] t*: avoid using pipes","startedAt":"2017-03-07T16:39:09Z","lastAt":"2017-03-09T05:26:59Z","messageCount":7,"participants":["Prathamesh Chavan","Stefan Beller","Johannes Sixt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"313432","messageId":"CAME+mvUe7itzg7JLu9_131smzHHE0JsN-z7q8_dTY1qEdugYWw@mail.gmail.com","threadId":"45306","inReplyTo":null,"subject":"[PATCH] t*: avoid using pipes","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-03-07T16:10:15Z","receivedAt":"2017-03-07T16:39:09Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"Hi,\nI'm Prathamesh Chavan. As a part of my micropraoject I have been working on\n\"Avoid pipes for git related commands in test suites\". I tried sending the\npatch, but it got blocked since the mail contained more than 100 000\ncharacters.\nHence I'll like to attach the link to my branch 'micro-proj', where I did the\nrequired changes.\n\nhttps://github.com/pratham-pc/git/tree/micro-proj\n\nThanks.\n"},{"id":"313434","messageId":"CAGZ79kaYi1OLuOKvbCmDrMCq0fZnO2Ry7JML=Puwmx6TTtEYog@mail.gmail.com","threadId":"45306","inReplyTo":"CAME+mvUe7itzg7JLu9_131smzHHE0JsN-z7q8_dTY1qEdugYWw@mail.gmail.com","subject":"Re: [PATCH] t*: avoid using pipes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-07T17:21:08Z","receivedAt":"2017-03-07T17:30:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 7, 2017 at 8:10 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:\n> Hi,\n> I'm Prathamesh Chavan. As a part of my micropraoject I have been working on\n> \"Avoid pipes for git related commands in test suites\".\n\nThanks for working on that microproject!\n\n> I tried sending the\n> patch, but it got blocked since the mail contained more than 100 000\n> characters.\n\nYeah, even the github UI seems to have trouble with that commit.\n(A bit slow, not showing the full content, but rather I needed to click\non \"load diff\" for tests files 7000+)\n\nThis is a lot of change (in terms of lines) for a micro project. :)\nI'd have two competing advices:\n* keep it micro first, e.g. just convert one file,\n  send to list, wait for reviewers feedback and incorporate that\n  (optional step after having done the full development cycle:\n  convert all the other files; each as its own patch)\n* split up this one patch into multiple patches, e.g. one\n  file per patch, then send a patch series.\n\nThe outcome will be the same, but in the first you get feedback\nquicker, such that hopefully you only need to touch the rest of\nfiles after the first file just once.\n\n\n> Hence I'll like to attach the link to my branch 'micro-proj', where I did the\n> required changes.\n>\n> https://github.com/pratham-pc/git/tree/micro-proj\n\nWhile I did look at that, not everyone here in the git community\ndoes so. (Also for getting your change in, Junio seems to strongly prefer\npatches on list instead of e.g. fetching and cherry-picking from your\ngithub)\n\nWhen looking at the content, the conversion seems a bit mechanical\n(which is fine for a micro project), such as:\n...\n- test \"$(git show --pretty=format:%s | head -n 1)\" = \"one\"\n+ test \"$(git show --pretty=format:%s >out && head -n 1 <out)\" = \"one\"\n...\n\nspecifically for the \"head\" command I don't think it makes a\ndifference in correctness whether you pipe the file into the tool\nor give the filename, i.e.  \"head -n 1 out\" would work just as fine.\n\nThere is a difference in readability, though. For consistency I'd\nsuggest to drop the \"<\", as the numbers might support:\n\n$ cd t\n$ git grep head |wc -l\n# This also counts other occurrences of the string,\n# not just the invocation of the head tool\n2871\n$ git grep head |grep \"<\" |wc -l\n# same here\n58\n\nAnother aspect might be performance at scale as the \"<\" will\nlet the shell open the file and pipe the content via stdin to the\nhead tool, whereas when giving a filename the head tool needs\nto open the file. Both times the file doesn't need to be read completely,\nbut \"head -n 1\" can close the file handle early in the game.\nI dunno.\n\nThanks,\nStefan\n\n>\n> Thanks.\n"},{"id":"313451","messageId":"3026648b-a26c-bc67-62dc-170217d6c2ca@kdbg.org","threadId":"45306","inReplyTo":"CAGZ79kaYi1OLuOKvbCmDrMCq0fZnO2Ry7JML=Puwmx6TTtEYog@mail.gmail.com","subject":"Re: [PATCH] t*: avoid using pipes","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-03-07T20:39:39Z","receivedAt":"2017-03-07T20:44:23Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 07.03.2017 um 18:21 schrieb Stefan Beller:\n> On Tue, Mar 7, 2017 at 8:10 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:\n>> I'm Prathamesh Chavan. As a part of my micropraoject I have been working on\n>> \"Avoid pipes for git related commands in test suites\".\n\nWelcome to the Git community!\n\n> * keep it micro first, e.g. just convert one file,\n>   send to list, wait for reviewers feedback and incorporate that\n>   (optional step after having done the full development cycle:\n>   convert all the other files; each as its own patch)\n> * split up this one patch into multiple patches, e.g. one\n>   file per patch, then send a patch series.\n\nActually, being a *micro* project, it should stay so. Not doing all of \nthe changes would leave some tasks for other apprentices to get warm \nwith our review process.\n\n>> https://github.com/pratham-pc/git/tree/micro-proj\n>\n> While I did look at that, not everyone here in the git community\n> does so. (Also for getting your change in, Junio seems to strongly prefer\n> patches on list instead of e.g. fetching and cherry-picking from your\n> github)\n\nThank you, Stefan, for digging out one particularly interesting case.\n\n> When looking at the content, the conversion seems a bit mechanical\n> (which is fine for a micro project), such as:\n> ...\n> - test \"$(git show --pretty=format:%s | head -n 1)\" = \"one\"\n> + test \"$(git show --pretty=format:%s >out && head -n 1 <out)\" = \"one\"\n> ...\n>\n> specifically for the \"head\" command I don't think it makes a\n> difference in correctness whether you pipe the file into the tool\n> or give the filename, i.e.  \"head -n 1 out\" would work just as fine.\n\nTrue, but!\n\nThe intent of removing git invocations from the upstream of a pipe is \nthat a failure exit code is able to stop a && chain.\n\nThe example above does not achieve this even after removal of the pipe. \nThe reason is that exit code of process expansions $(...) is usually \nignored. To meet the intent, further changes are necessary, for example to:\n\n\tgit show --pretty=format:%s >out &&\n\ttest \"$(head -n 1 out)\" = \"one\"\n\nSide note: There is one exception where the exit code of $(...) is not \nignored: when it occurs in the last variable assignment of a command \nthat consists only of variable assignments.\n\n-- Hannes\n\n"},{"id":"313452","messageId":"CAGZ79kZhfRiUQndEGB=b34WMCPv0KDjpDix0Ly85aFeyOQAwWA@mail.gmail.com","threadId":"45306","inReplyTo":"3026648b-a26c-bc67-62dc-170217d6c2ca@kdbg.org","subject":"Re: [PATCH] t*: avoid using pipes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-07T20:52:49Z","receivedAt":"2017-03-07T21:17:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 7, 2017 at 12:39 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n\n> Welcome to the Git community!\n\n>\n> Actually, being a *micro* project, it should stay so. Not doing all of the\n> changes would leave some tasks for other apprentices to get warm with our\n> review process.\n\nright, so just pick one file.\n\n\n> Thank you, Stefan, for digging out one particularly interesting case.\n>\n>> When looking at the content, the conversion seems a bit mechanical\n>> (which is fine for a micro project), such as:\n>> ...\n>> - test \"$(git show --pretty=format:%s | head -n 1)\" = \"one\"\n>> + test \"$(git show --pretty=format:%s >out && head -n 1 <out)\" = \"one\"\n>> ...\n>>\n>> specifically for the \"head\" command I don't think it makes a\n>> difference in correctness whether you pipe the file into the tool\n>> or give the filename, i.e.  \"head -n 1 out\" would work just as fine.\n>\n>\n> True, but!\n>\n> The intent of removing git invocations from the upstream of a pipe is that a\n> failure exit code is able to stop a && chain.\n\nDoh! I was so fixated over discussing whether to use \"<\" or not,\nto miss looking for the actual goal.\n\nThanks for spotting!\nStefan\n"},{"id":"313485","messageId":"20170308060334.6ilcjgaxgycuhpxu@sigill.intra.peff.net","threadId":"45306","inReplyTo":"CAGZ79kZhfRiUQndEGB=b34WMCPv0KDjpDix0Ly85aFeyOQAwWA@mail.gmail.com","subject":"Re: [PATCH] t*: avoid using pipes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-08T06:03:34Z","receivedAt":"2017-03-08T06:30:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 07, 2017 at 12:52:49PM -0800, Stefan Beller wrote:\n\n> On Tue, Mar 7, 2017 at 12:39 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> \n> > Welcome to the Git community!\n> \n> >\n> > Actually, being a *micro* project, it should stay so. Not doing all of the\n> > changes would leave some tasks for other apprentices to get warm with our\n> > review process.\n> \n> right, so just pick one file.\n\nI also wonder if we really want all invocations of git to be marked up\nin this way. If the primary goal of the test is checking that a certain\ngit command runs successfully and generates the expected output, then I\nthink it is a good candidate for conversion.\n\nSo in a hunk like this:\n\n   test_expect_success 'git commit-tree records the correct tree in a commit' '\n   \tcommit0=$(echo NO | git commit-tree $P) &&\n  -\ttree=$(git show --pretty=raw $commit0 |\n  -\t\t sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n  +\ttree=$(git show --pretty=raw $commit0 >out &&\n  +\tsed -n -e \"s/^tree //p\" -e \"/^author /q\" <out) &&\n   \ttest \"z$tree\" = \"z$P\"\n\nwe are interested in testing commit-tree, not \"git show\". Is it worth\navoiding pipes there? I admit the cost to using the intermediate file is\nnot huge there, but it feels more awkward and un-shell-like to me as a\nreader.\n\n-Peff\n"},{"id":"313496","messageId":"CAME+mvUzR6--AeUff6yGZ69GN-hE6AyDP-CkdFxnFAwccpn2yg@mail.gmail.com","threadId":"45306","inReplyTo":"20170308060334.6ilcjgaxgycuhpxu@sigill.intra.peff.net","subject":"Re: [PATCH] t*: avoid using pipes","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-03-08T13:32:27Z","receivedAt":"2017-03-08T14:39:43Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"On Wed, Mar 8, 2017 at 11:33 AM, Jeff King <peff@peff.net> wrote:\n> On Tue, Mar 07, 2017 at 12:52:49PM -0800, Stefan Beller wrote:\n>\n>> On Tue, Mar 7, 2017 at 12:39 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n>>\n>> > Welcome to the Git community!\n>>\n>> >\n>> > Actually, being a *micro* project, it should stay so. Not doing all of the\n>> > changes would leave some tasks for other apprentices to get warm with our\n>> > review process.\n>>\n>> right, so just pick one file.\n>\n> I also wonder if we really want all invocations of git to be marked up\n> in this way. If the primary goal of the test is checking that a certain\n> git command runs successfully and generates the expected output, then I\n> think it is a good candidate for conversion.\n>\n> So in a hunk like this:\n>\n>    test_expect_success 'git commit-tree records the correct tree in a commit' '\n>         commit0=$(echo NO | git commit-tree $P) &&\n>   -     tree=$(git show --pretty=raw $commit0 |\n>   -              sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n>   +     tree=$(git show --pretty=raw $commit0 >out &&\n>   +     sed -n -e \"s/^tree //p\" -e \"/^author /q\" <out) &&\n>         test \"z$tree\" = \"z$P\"\n>\n> we are interested in testing commit-tree, not \"git show\". Is it worth\n> avoiding pipes there? I admit the cost to using the intermediate file is\n> not huge there, but it feels more awkward and un-shell-like to me as a\n> reader.\n>\n> -Peff\n\nThank you everyone, for reviewing my changes. And as said in the\nreviews, I'll send a single patch file as my microproject, leaving the other\nfiles as low hanging fruit for the others to look at. Also, I try to include as\nmany suggested improvements as possible and will also remember them for\nmy future patches.\n"},{"id":"313613","messageId":"CAME+mvWRSEJ6kp4MpELx20DkeR+pDED-KGB2o6yQ-26FmLvqWw@mail.gmail.com","threadId":"45306","inReplyTo":"CAME+mvUzR6--AeUff6yGZ69GN-hE6AyDP-CkdFxnFAwccpn2yg@mail.gmail.com","subject":"Re: [PATCH] t*: avoid using pipes","fromName":"Prathamesh Chavan","fromEmail":"pc44800@gmail.com","sentAt":"2017-03-09T05:26:51Z","receivedAt":"2017-03-09T05:26:59Z","isPatch":true,"sender":{"key":"pc44800@gmail.com","avatar":"https://avatars.githubusercontent.com/u/17272661?v=4"},"body":"I have created the required changes and submitted a single file patch.\nAlso I tried my best to include each of the suggestions in that patch.\n\nhttps://public-inbox.org/git/0102015aae7b8536-00c57d0a-1d48-4153-a202-87c4ea9e0e19-000000@eu-west-1.amazonses.com/\n\nOn Wed, Mar 8, 2017 at 7:02 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:\n> On Wed, Mar 8, 2017 at 11:33 AM, Jeff King <peff@peff.net> wrote:\n>> On Tue, Mar 07, 2017 at 12:52:49PM -0800, Stefan Beller wrote:\n>>\n>>> On Tue, Mar 7, 2017 at 12:39 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n>>>\n>>> > Welcome to the Git community!\n>>>\n>>> >\n>>> > Actually, being a *micro* project, it should stay so. Not doing all of the\n>>> > changes would leave some tasks for other apprentices to get warm with our\n>>> > review process.\n>>>\n>>> right, so just pick one file.\n>>\n>> I also wonder if we really want all invocations of git to be marked up\n>> in this way. If the primary goal of the test is checking that a certain\n>> git command runs successfully and generates the expected output, then I\n>> think it is a good candidate for conversion.\n>>\n>> So in a hunk like this:\n>>\n>>    test_expect_success 'git commit-tree records the correct tree in a commit' '\n>>         commit0=$(echo NO | git commit-tree $P) &&\n>>   -     tree=$(git show --pretty=raw $commit0 |\n>>   -              sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n>>   +     tree=$(git show --pretty=raw $commit0 >out &&\n>>   +     sed -n -e \"s/^tree //p\" -e \"/^author /q\" <out) &&\n>>         test \"z$tree\" = \"z$P\"\n>>\n>> we are interested in testing commit-tree, not \"git show\". Is it worth\n>> avoiding pipes there? I admit the cost to using the intermediate file is\n>> not huge there, but it feels more awkward and un-shell-like to me as a\n>> reader.\n>>\n>> -Peff\n>\n> Thank you everyone, for reviewing my changes. And as said in the\n> reviews, I'll send a single patch file as my microproject, leaving the other\n> files as low hanging fruit for the others to look at. Also, I try to include as\n> many suggested improvements as possible and will also remember them for\n> my future patches.\n"}]}