{"thread":{"id":"57458","subject":"[PATCH 0/2] microproject: avoid using pipes in test","startedAt":"2022-02-22T11:43:28Z","lastAt":"2022-02-24T18:22:40Z","messageCount":31,"participants":["Shubham Mishra","Derrick Stolee","Ævar Arnfjörð Bjarmason","Taylor Blau","Christian Couder","Shaoxuan Yuan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"449112","messageId":"20220222114313.14921-1-shivam828787@gmail.com","threadId":"57458","inReplyTo":null,"subject":"[PATCH 0/2] microproject: avoid using pipes in test","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-22T11:43:11Z","receivedAt":"2022-02-22T11:43:28Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"pipes doesn't care about error codes and ignore them thus we should not use them in tests.\nAs an easy alternative, I am using a tmp file to write from git command so we can test the exit code.\n\nThis is my first contribution that's why I am keeping diff short with an intention to understand the process instead of making impactful change in first attempt. Later, I will send more patches to fix the issue for other files.\nShubham Mishra (2):\n  t0001: remove pipes\n  t0003: remove pipes\n\n t/t0001-init.sh       | 4 ++--\n t/t0003-attributes.sh | 6 +++---\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\n\nbase-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e\n-- \n2.25.1\n\n"},{"id":"449113","messageId":"20220222114313.14921-2-shivam828787@gmail.com","threadId":"57458","inReplyTo":"20220222114313.14921-1-shivam828787@gmail.com","subject":"[PATCH 1/2] t0001: remove pipes","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-22T11:43:12Z","receivedAt":"2022-02-22T11:43:43Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"pipes doesn't care about error codes and ignore them thus we should not use them in tests.\nAs an easy alternative, I am using a tmp file to write from git command so we can test the exit code.\n\nSigned-off-by: Shubham Mishra <shivam828787@gmail.com>\n---\n t/t0001-init.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 3235ab4d53..9a8f209648 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -489,11 +489,11 @@ test_expect_success 're-init from a linked worktree' '\n \t\tgit worktree add ../linked-worktree &&\n \t\tmv .git/info/exclude expected-exclude &&\n \t\tcp .git/config expected-config &&\n-\t\tfind .git/worktrees -print | sort >expected &&\n+\t\tfind .git/worktrees -print >tmp && sort tmp >expected &&\n \t\tgit -C ../linked-worktree init &&\n \t\ttest_cmp expected-exclude .git/info/exclude &&\n \t\ttest_cmp expected-config .git/config &&\n-\t\tfind .git/worktrees -print | sort >actual &&\n+\t\tfind .git/worktrees -print >tmp && sort tmp >actual &&\n \t\ttest_cmp expected actual\n \t)\n '\n-- \n2.25.1\n\n"},{"id":"449114","messageId":"20220222114313.14921-3-shivam828787@gmail.com","threadId":"57458","inReplyTo":"20220222114313.14921-1-shivam828787@gmail.com","subject":"[PATCH 2/2] t0003: remove pipes","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-22T11:43:13Z","receivedAt":"2022-02-22T11:43:48Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"pipes doesn't care about error codes and ignore them thus we should not use them in tests.\nAs an easy alternative, I am using a tmp file to write from git command so we can test the exit code.\n\nSigned-off-by: Shubham Mishra <shivam828787@gmail.com>\n---\n t/t0003-attributes.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex b9ed612af1..51f9f6503c 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -205,15 +205,15 @@ test_expect_success 'attribute test: read paths from stdin' '\n test_expect_success 'attribute test: --all option' '\n \tgrep -v unspecified <expect-all | sort >specified-all &&\n \tsed -e \"s/:.*//\" <expect-all | uniq >stdin-all &&\n-\tgit check-attr --stdin --all <stdin-all | sort >actual &&\n+\tgit check-attr --stdin --all <stdin-all >tmp && sort tmp >actual &&\n \ttest_cmp specified-all actual\n '\n \n test_expect_success 'attribute test: --cached option' '\n-\tgit check-attr --cached --stdin --all <stdin-all | sort >actual &&\n+\tgit check-attr --cached --stdin --all <stdin-all >tmp && sort tmp >actual &&\n \ttest_must_be_empty actual &&\n \tgit add .gitattributes a/.gitattributes a/b/.gitattributes &&\n-\tgit check-attr --cached --stdin --all <stdin-all | sort >actual &&\n+\tgit check-attr --cached --stdin --all <stdin-all >tmp && sort tmp >actual &&\n \ttest_cmp specified-all actual\n '\n \n-- \n2.25.1\n\n"},{"id":"449130","messageId":"2e40461e-0b36-af67-33d5-a5d37f335025@github.com","threadId":"57458","inReplyTo":"20220222114313.14921-1-shivam828787@gmail.com","subject":"Re: [PATCH 0/2] microproject: avoid using pipes in test","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-02-22T13:50:26Z","receivedAt":"2022-02-22T13:50:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/22/2022 6:43 AM, Shubham Mishra wrote:\n\nWelcome, Subham!\n\n> pipes doesn't care about error codes and ignore them thus we should not use them in tests.\n> As an easy alternative, I am using a tmp file to write from git command so we can test the exit code.\n\nThis is the correct way to convert the pipes into something that\nproperly notices Git failures. The only issue I have with your\npatches is that you should insert a newline after your &&.\n\n(I'll include an example on patch 1.)\n \n> This is my first contribution that's why I am keeping diff short with an intention to understand the process instead of making impactful change in first attempt. Later, I will send more patches to fix the issue for other files.\n\nI think this is a great start! After fixing the formatting that\nI saw, you should be good to start making more changes in a\nsingle series. These patches are a good size.\n\nThanks,\n-Stolee\n"},{"id":"449131","messageId":"9d08d2a6-82ed-7d6c-6a82-0e89c55bd94a@github.com","threadId":"57458","inReplyTo":"20220222114313.14921-2-shivam828787@gmail.com","subject":"Re: [PATCH 1/2] t0001: remove pipes","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-02-22T13:54:48Z","receivedAt":"2022-02-22T13:54:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/22/2022 6:43 AM, Shubham Mishra wrote:\n> pipes doesn't care about error codes and ignore them thus we should not use them in tests.\n> As an easy alternative, I am using a tmp file to write from git command so we can test the exit code.\n\nPlease be careful about the length of your lines in your\ncommit message. vim should auto-wrap as you write.\n\nThere are also some grammar issues, so here is an update\nto what you wrote:\n\n  Pipes ignore error codes and thus we should not use them\n  in tests. As an alternative, use a 'tmp' file to write the\n  Git output so we can test the exit code.\n\n(My wrapping is probably too short here.)\n\n> Signed-off-by: Shubham Mishra <shivam828787@gmail.com>\n> ---\n>  t/t0001-init.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/t0001-init.sh b/t/t0001-init.sh\n> index 3235ab4d53..9a8f209648 100755\n> --- a/t/t0001-init.sh\n> +++ b/t/t0001-init.sh\n> @@ -489,11 +489,11 @@ test_expect_success 're-init from a linked worktree' '\n>  \t\tgit worktree add ../linked-worktree &&\n>  \t\tmv .git/info/exclude expected-exclude &&\n>  \t\tcp .git/config expected-config &&\n> -\t\tfind .git/worktrees -print | sort >expected &&\n> +\t\tfind .git/worktrees -print >tmp && sort tmp >expected &&\n\nSplit each part of the &&-chain across lines, like this:\n\n+\t\tfind .git/worktrees -print >tmp &&\n+\t\tsort tmp >expected &&\n\n>  \t\tgit -C ../linked-worktree init &&\n>  \t\ttest_cmp expected-exclude .git/info/exclude &&\n>  \t\ttest_cmp expected-config .git/config &&\n> -\t\tfind .git/worktrees -print | sort >actual &&\n> +\t\tfind .git/worktrees -print >tmp && sort tmp >actual &&\n\nHere, too.\n\nI could make the same comments on patch 2, but I think you\nhave enough info to go on.\n\nThanks,\n-Stolee\n"},{"id":"449136","messageId":"220222.86pmnf6ket.gmgdl@evledraar.gmail.com","threadId":"57458","inReplyTo":"20220222114313.14921-1-shivam828787@gmail.com","subject":"Re: [PATCH 0/2] microproject: avoid using pipes in test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-22T14:24:31Z","receivedAt":"2022-02-22T14:33:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Feb 22 2022, Shubham Mishra wrote:\n\n> pipes doesn't care about error codes and ignore them thus we should not use them in tests.\n\nAside from what Derrick Stolee mentioned in his feedback, all of which I\nagree with.\n\nI think these changes are good, but it's not the case that we try to\navoid using pipes at all in our tests.\n\nIt's often a hassle, and just not worth it, e.g.:\n\n    oid=$(echo foo | git hash-object --stdin -w) &&\n\nSure, we can make that:\n\n    echo foo >in &&\n    oid=$(git hash-object --stdin -w <in) &&\n\nBut in the general case it's not worth worrying about.\n\nWhat we *do* try to avoid, and what's actually important is to never\ninvoke \"git\" or other programs we invoke on the LHS of a pipe, or to\notherwise do so in a way that hides potential errors.\n\nThat's not isolated to just pipes, but e.g. calling it within helper\nfunctions that don't &&-chain, but pipes are probably the most common\noffender.\n\nThe reason we do that is because in hacking git we may make it error,\nsegfault etc. If it's on the LHS of a pipe that failure becomes\nindistinguishable from success.\n\nAnd if the test is really checking e.g. \"when I run git like this, it\nproduces no output\" printing nothing with an exit of 0 will become the\nsame as a segafault for the purposes of test.\n\nAnd that's bad.\n\nBut just invoking things on the LHS of a pipe? Sometimes it's a good\nthing not do do that, because we'll be able to see a failure more\nincrementally, and with intermediate files.\n\nBut it's generally not a problem, our test suite assumes that the OS is\nbasically sane. We're not going to call every invocation of \"sed\",\n\"grep\" etc. with a wrapper like \"test_must_fail grep\" instead of \"!\ngrep\".\n\nThe same goes for our own helper utility functions such as \"q_to_nul\"\netc, as long as (and this is critical) that they're implemented by\nshelling out to \"sed\", \"grep\", \"perl\" or whatever, and not to \"git\" or\n\"test-tool\" etc. Then we need to start being as paranoid about them as\n\"git\" on the LHS of pipes.\n\n"},{"id":"449175","messageId":"CAC316V5CcB_UxpVYdAYZDPpY_ebauu4K23TrU1zLnTdgXg0PNQ@mail.gmail.com","threadId":"57458","inReplyTo":"9d08d2a6-82ed-7d6c-6a82-0e89c55bd94a@github.com","subject":"Re: [PATCH 1/2] t0001: remove pipes","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-22T18:16:14Z","receivedAt":"2022-02-22T18:16:30Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"> > pipes doesn't care about error codes and ignore them thus we should not use them in tests.\n> > As an easy alternative, I am using a tmp file to write from git command so we can test the exit code.\n>\n> Please be careful about the length of your lines in your\n> commit message. vim should auto-wrap as you write.\n\nOops, I mostly use Nano, I think it's time to use vim. I will take care of it.\n\n> There are also some grammar issues, so here is an update\n> to what you wrote:\n>\n>   Pipes ignore error codes and thus we should not use them\n>   in tests. As an alternative, use a 'tmp' file to write the\n>   Git output so we can test the exit code.\n\nThanks for pointing this out. I will update it. I am a non-native\nEnglish speaker, and looking to improve my command in language with\ntime :)\n\n> Split each part of the &&-chain across lines, like this:\n>\n> +               find .git/worktrees -print >tmp &&\n> +               sort tmp >expected &&\n>\n> >               git -C ../linked-worktree init &&\n> >               test_cmp expected-exclude .git/info/exclude &&\n> >               test_cmp expected-config .git/config &&\n> > -             find .git/worktrees -print | sort >actual &&\n> > +             find .git/worktrees -print >tmp && sort tmp >actual &&\n>\n> Here, too.\n>\n> I could make the same comments on patch 2, but I think you\n> have enough info to go on.\n\nSure, I get it! I will consider all these suggestions in v2.\n\nThanks,\nShubham\n"},{"id":"449188","messageId":"CAC316V4ptAWJDgkcDrN8Gt7fSm4MtJFB7xqC4JTcEX0OW8a6rg@mail.gmail.com","threadId":"57458","inReplyTo":"220222.86pmnf6ket.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 0/2] microproject: avoid using pipes in test","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-22T19:12:01Z","receivedAt":"2022-02-22T19:12:19Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"> I think these changes are good, but it's not the case that we try to\n> avoid using pipes at all in our tests.\n>\n> It's often a hassle, and just not worth it, e.g.:\n>\n>     oid=$(echo foo | git hash-object --stdin -w) &&\n>\n> Sure, we can make that:\n>\n>     echo foo >in &&\n>     oid=$(git hash-object --stdin -w <in) &&\n>\n> But in the general case it's not worth worrying about.\n>\n> What we *do* try to avoid, and what's actually important is to never\n> invoke \"git\" or other programs we invoke on the LHS of a pipe, or to\n> otherwise do so in a way that hides potential errors.\n\nSorry, I have not mentioned it properly in the message but my\nintention is not to remove all pipes but to remove only those, which\nhave \"git\" command in LHS.\n\n> That's not isolated to just pipes, but e.g. calling it within helper\n> functions that don't &&-chain, but pipes are probably the most common\n> offender.\n>\n> The reason we do that is because in hacking git we may make it error,\n> segfault etc. If it's on the LHS of a pipe that failure becomes\n> indistinguishable from success.\n>\n> And if the test is really checking e.g. \"when I run git like this, it\n> produces no output\" printing nothing with an exit of 0 will become the\n> same as a segafault for the purposes of test.\n>\n> And that's bad.\n>\n> But just invoking things on the LHS of a pipe? Sometimes it's a good\n> thing not do do that, because we'll be able to see a failure more\n> incrementally, and with intermediate files.\n>\n> But it's generally not a problem, our test suite assumes that the OS is\n> basically sane. We're not going to call every invocation of \"sed\",\n> \"grep\" etc. with a wrapper like \"test_must_fail grep\" instead of \"!\n> grep\".\n>\n> The same goes for our own helper utility functions such as \"q_to_nul\"\n> etc, as long as (and this is critical) that they're implemented by\n> shelling out to \"sed\", \"grep\", \"perl\" or whatever, and not to \"git\" or\n> \"test-tool\" etc. Then we need to start being as paranoid about them as\n> \"git\" on the LHS of pipes.\n\nThanks here for providing me with a broader context of the problem.\nWhat I understand,\nIt's not just about \"git\" on LHS of pipes but more broader to anything\ncustom where\nWe can miss exit codes but I think as a low hanging fruit I can start\nwith \"git\" on LHS of pipe\nand as I will understand the codebase more I can work on other custom\nhelpers too.\n\nThanks,\nShubham\n"},{"id":"449190","messageId":"YhU76TpyP6p56/oV@nand.local","threadId":"57458","inReplyTo":"220222.86pmnf6ket.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 0/2] microproject: avoid using pipes in test","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-22T19:39:21Z","receivedAt":"2022-02-22T19:39:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 22, 2022 at 03:24:31PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Tue, Feb 22 2022, Shubham Mishra wrote:\n>\n> > pipes doesn't care about error codes and ignore them thus we should not use them in tests.\n>\n> Aside from what Derrick Stolee mentioned in his feedback, all of which I\n> agree with.\n>\n> I think these changes are good, but it's not the case that we try to\n> avoid using pipes at all in our tests.\n>\n> It's often a hassle, and just not worth it, e.g.:\n>\n>     oid=$(echo foo | git hash-object --stdin -w) &&\n>\n> Sure, we can make that:\n>\n>     echo foo >in &&\n>     oid=$(git hash-object --stdin -w <in) &&\n>\n> But in the general case it's not worth worrying about.\n\nAgreed, and I would add that we don't necessarily need to worry about\nnon-Git commands on the left-hand side of a pipe. So something like:\n\n    find ... | sort >actual\n\nisn't a problem for us, because our test suite assumes that something\nlike find will not fail. So leaving instances of those alone is OK,\nbut...\n\n> What we *do* try to avoid, and what's actually important is to never\n> invoke \"git\" or other programs we invoke on the LHS of a pipe, or to\n> otherwise do so in a way that hides potential errors.\n>\n> That's not isolated to just pipes, but e.g. calling it within helper\n> functions that don't &&-chain, but pipes are probably the most common\n> offender.\n>\n> The reason we do that is because in hacking git we may make it error,\n> segfault etc. If it's on the LHS of a pipe that failure becomes\n> indistinguishable from success.\n>\n> And if the test is really checking e.g. \"when I run git like this, it\n> produces no output\" printing nothing with an exit of 0 will become the\n> same as a segafault for the purposes of test.\n\n...yes, we do care about Git failures. So something like:\n\n    git ls-files | grep \"want\"\n\nwould be no-good, since any failures running 'git ls-files' would be\nquashed by the pipe.\n\nThanks,\nTaylor\n"},{"id":"449194","messageId":"CAP8UFD1FVuoe7V3JU+hExsbjWTkNZFCZFV4qc65YLd4ow-Da5w@mail.gmail.com","threadId":"57458","inReplyTo":"20220222114313.14921-2-shivam828787@gmail.com","subject":"Re: [PATCH 1/2] t0001: remove pipes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-22T20:32:30Z","receivedAt":"2022-02-22T20:32:47Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Feb 22, 2022 at 3:08 PM Shubham Mishra <shivam828787@gmail.com> wrote:\n>\n> pipes doesn't care about error codes and ignore them thus we should not use them in tests.\n\nOnly the exit code of the command before the pipe is ignored.\n\nAlso it's ok to use pipes if the command before the pipe is not `git`.\nWe trust regular commands to just work and we test only `git`.\n\n> As an easy alternative, I am using a tmp file to write from git command so we can test the exit code.\n\nIn general, when improving the code in a way that has already been\nused by others, it's a good idea to take a look at previous commits\ndoing the same thing. See for example 66c0c44df6 (t0000: avoid masking\ngit exit value through pipes, 2021-09-16). I am not saying that you\nshould copy paste the commit message though.\n\n> Signed-off-by: Shubham Mishra <shivam828787@gmail.com>\n> ---\n>  t/t0001-init.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t0001-init.sh b/t/t0001-init.sh\n> index 3235ab4d53..9a8f209648 100755\n> --- a/t/t0001-init.sh\n> +++ b/t/t0001-init.sh\n> @@ -489,11 +489,11 @@ test_expect_success 're-init from a linked worktree' '\n>                 git worktree add ../linked-worktree &&\n>                 mv .git/info/exclude expected-exclude &&\n>                 cp .git/config expected-config &&\n> -               find .git/worktrees -print | sort >expected &&\n> +               find .git/worktrees -print >tmp && sort tmp >expected &&\n\nPlease put the `find` and `sort` commands on 2 different lines when\nthey are separated with &&.\n\n>                 git -C ../linked-worktree init &&\n>                 test_cmp expected-exclude .git/info/exclude &&\n>                 test_cmp expected-config .git/config &&\n> -               find .git/worktrees -print | sort >actual &&\n> +               find .git/worktrees -print >tmp && sort tmp >actual &&\n\nIdem.\n\n>                 test_cmp expected actual\n>         )\n>  '\n> --\n> 2.25.1\n>\n"},{"id":"449234","messageId":"20220223115347.3083-1-shivam828787@gmail.com","threadId":"57458","inReplyTo":"20220222114313.14921-1-shivam828787@gmail.com","subject":"[PATCH v2 0/2] avoid pipes with Git on LHS","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-23T11:53:45Z","receivedAt":"2022-02-23T11:54:07Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"fixed commit messages and endline after '&&' formatting from previous\nversion.\n\nShubham Mishra (2):\n  t0001: avoid pipes with Git on LHS\n  t0003: avoid pipes with Git on LHS\n\n t/t0001-init.sh       | 6 ++++--\n t/t0003-attributes.sh | 9 ++++++---\n 2 files changed, 10 insertions(+), 5 deletions(-)\n\nRange-diff against v1:\n1:  620a5b991f = 1:  620a5b991f t0001: avoid pipes with Git on LHS\n2:  c17d49d802 = 2:  c17d49d802 t0003: avoid pipes with Git on LHS\n-- \n2.25.1\n\n"},{"id":"449235","messageId":"20220223115347.3083-2-shivam828787@gmail.com","threadId":"57458","inReplyTo":"20220223115347.3083-1-shivam828787@gmail.com","subject":"[PATCH v2 1/2] t0001: avoid pipes with Git on LHS","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-23T11:53:46Z","receivedAt":"2022-02-23T11:54:32Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"Pipes ignore error codes of LHS command and thu`s we should not use\nthem with Git in tests. As an alternative, use a 'tmp' file to write\nthe Git output so we can test the exit code.\n\nSigned-off-by: Shubham Mishra <shivam828787@gmail.com>\n---\n t/t0001-init.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 3235ab4d53..c7dd91cb80 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -489,11 +489,13 @@ test_expect_success 're-init from a linked worktree' '\n \t\tgit worktree add ../linked-worktree &&\n \t\tmv .git/info/exclude expected-exclude &&\n \t\tcp .git/config expected-config &&\n-\t\tfind .git/worktrees -print | sort >expected &&\n+\t\tfind .git/worktrees -print >tmp &&\n+\t\tsort tmp >expected &&\n \t\tgit -C ../linked-worktree init &&\n \t\ttest_cmp expected-exclude .git/info/exclude &&\n \t\ttest_cmp expected-config .git/config &&\n-\t\tfind .git/worktrees -print | sort >actual &&\n+\t\tfind .git/worktrees -print >tmp &&\n+\t\tsort tmp >actual &&\n \t\ttest_cmp expected actual\n \t)\n '\n-- \n2.25.1\n\n"},{"id":"449236","messageId":"20220223115347.3083-3-shivam828787@gmail.com","threadId":"57458","inReplyTo":"20220223115347.3083-1-shivam828787@gmail.com","subject":"[PATCH v2 2/2] t0003: avoid pipes with Git on LHS","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-23T11:53:47Z","receivedAt":"2022-02-23T11:54:37Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"Pipes ignore error codes of LHS command and thus we should not use them\nwith Git in tests. As an alternative, use a 'tmp' file to write the Git\noutput so we can test the exit code.\n\nSigned-off-by: Shubham Mishra <shivam828787@gmail.com>\n---\n t/t0003-attributes.sh | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex b9ed612af1..143f100517 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -205,15 +205,18 @@ test_expect_success 'attribute test: read paths from stdin' '\n test_expect_success 'attribute test: --all option' '\n \tgrep -v unspecified <expect-all | sort >specified-all &&\n \tsed -e \"s/:.*//\" <expect-all | uniq >stdin-all &&\n-\tgit check-attr --stdin --all <stdin-all | sort >actual &&\n+\tgit check-attr --stdin --all <stdin-all >tmp &&\n+\tsort tmp >actual &&\n \ttest_cmp specified-all actual\n '\n \n test_expect_success 'attribute test: --cached option' '\n-\tgit check-attr --cached --stdin --all <stdin-all | sort >actual &&\n+\tgit check-attr --cached --stdin --all <stdin-all >tmp &&\n+\tsort tmp >actual &&\n \ttest_must_be_empty actual &&\n \tgit add .gitattributes a/.gitattributes a/b/.gitattributes &&\n-\tgit check-attr --cached --stdin --all <stdin-all | sort >actual &&\n+\tgit check-attr --cached --stdin --all <stdin-all >tmp &&\n+\tsort tmp >actual &&\n \ttest_cmp specified-all actual\n '\n \n-- \n2.25.1\n\n"},{"id":"449239","messageId":"CAC316V57tHux-+MWMJuP5wNtwYrc2AinZj+ffnoyy-aWQWDukg@mail.gmail.com","threadId":"57458","inReplyTo":"20220223115347.3083-1-shivam828787@gmail.com","subject":"Re: [PATCH v2 0/2] avoid pipes with Git on LHS","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-23T12:08:54Z","receivedAt":"2022-02-23T12:10:14Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"Hi,\nI am using mails for code review for the first time, I have some\ndoubts, Can someone please clarify them? -\n1. It looks like the cover letter (Including \"Range-diff\" section) is\nonly for context sharing with reviewers, nothing from it gets merged\nto the \"seek\" or any other branch.\n2. I wanted to know how the merging process takes place. Once the\npatch is accepted, do we merge all previous versions of it one after\nanother or every patch is independent so we have to just merge the\nlast accepted patch?\n3. How does a particular patch set is linked to its previous version?\nI added the flag \"--in-reply-to\" while sending this mail but I can't\nfind any linking here on the mail. Not sure If I am missing something\nhere.\n"},{"id":"449247","messageId":"CAJyCBOTiWxAgkZPB6M5jPw=t=32zrnNH5tfnOc9Kdj9mKayrSA@mail.gmail.com","threadId":"57458","inReplyTo":"CAC316V57tHux-+MWMJuP5wNtwYrc2AinZj+ffnoyy-aWQWDukg@mail.gmail.com","subject":"Re: [PATCH v2 0/2] avoid pipes with Git on LHS","fromName":"Shaoxuan Yuan","fromEmail":"shaoxuan.yuan02@gmail.com","sentAt":"2022-02-23T13:19:23Z","receivedAt":"2022-02-23T13:19:38Z","isPatch":true,"sender":{"key":"shaoxuan.yuan02@gmail.com","avatar":"https://avatars.githubusercontent.com/u/46557895?v=4"},"body":"On Wed, Feb 23, 2022 at 8:42 PM Shubham Mishra <shivam828787@gmail.com> wrote:\n>\n> Hi,\n\nHi,\n\n> I am using mails for code review for the first time, I have some\n> doubts, Can someone please clarify them? -\n> 1. It looks like the cover letter (Including \"Range-diff\" section) is\n> only for context sharing with reviewers, nothing from it gets merged\n> to the \"seek\" or any other branch.\n\nThe cover letter stands for an introduction/summary to your patches.\nYou can also put helpful context in it for better understanding. According\nto my knowledge, it will not be in the commit messages.\n\n> 2. I wanted to know how the merging process takes place. Once the\n> patch is accepted, do we merge all previous versions of it one after\n> another or every patch is independent so we have to just merge the\n> last accepted patch?\n\nNot so sure about this question. My two cents: generally the most agreed-upon\npatch will be merged, but the exact merging process could vary based\non the circumstances. Probably Junio can have a better answer to this.\n\n> 3. How does a particular patch set is linked to its previous version?\n\n\"--in-reply-to\" will do.\n\n> I added the flag \"--in-reply-to\" while sending this mail but I can't\n> find any linking here on the mail. Not sure If I am missing something\n> here.\n\nJust by looking at the current thread, I think you are doing it right.\n\nAdding \"--in-reply-to\" to the Message-ID of your previous patch can\ngive you a threaded view of the patch history on the public-inbox. For\ninstance, by always \"--in-reply-to\" the first patch version (\"v1\"), you can\nline up your patches as they are threaded on the same level. You also\ncan choose to always reply to the previous patch's cover letter (just an\nexample), so that your patch history cascades. I think you always go\nthe way that best illustrates the history and helps reviewers.\n\nI'm also new to the community, that's the best I can answer.\nInaccuracies may be found above.\n\n-- \nThanks & Regards,\nShaoxuan\n"},{"id":"449278","messageId":"YhZ0qOwtY1vUVTiN@nand.local","threadId":"57458","inReplyTo":"20220223115347.3083-2-shivam828787@gmail.com","subject":"Re: [PATCH v2 1/2] t0001: avoid pipes with Git on LHS","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-23T17:53:44Z","receivedAt":"2022-02-23T17:53:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Feb 23, 2022 at 05:23:46PM +0530, Shubham Mishra wrote:\n> Pipes ignore error codes of LHS command and thu`s we should not use\n> them with Git in tests. As an alternative, use a 'tmp' file to write\n> the Git output so we can test the exit code.\n\nThis patch does preserve the existing behavior. But I'm hesitant to\nrecommend that we apply this patch, since our test suite assumes that\ncommands like find will work, and so we aren't concerned about squashing\nany potential error codes when it's on the left-hand side of the pipe,\nsince we assume that it won't fail in general.\n\n(That's notably different from the second patch in this series, where\nthe thing on the left-hand side of the pipe is a git invocation. In that\ncase, we really _do_ want to avoid having it on the left-hand side of\nthe pipe, because we don't have the same error-free expectation there,\nand want to know when it fails).\n\nI think that Ævar gave a nice summary of the above in [1]\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/220222.86pmnf6ket.gmgdl@evledraar.gmail.com/\n"},{"id":"449279","messageId":"YhZ0yXFIZNw/us6d@nand.local","threadId":"57458","inReplyTo":"20220223115347.3083-3-shivam828787@gmail.com","subject":"Re: [PATCH v2 2/2] t0003: avoid pipes with Git on LHS","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-23T17:54:17Z","receivedAt":"2022-02-23T17:54:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Feb 23, 2022 at 05:23:47PM +0530, Shubham Mishra wrote:\n>  t/t0003-attributes.sh | 9 ++++++---\n>  1 file changed, 6 insertions(+), 3 deletions(-)\n\nThis patch looks great to me, thanks!\n\n    Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\nThanks,\nTaylor\n"},{"id":"449292","messageId":"YhZ2Zpl3XGkTKxuI@nand.local","threadId":"57458","inReplyTo":"CAJyCBOTiWxAgkZPB6M5jPw=t=32zrnNH5tfnOc9Kdj9mKayrSA@mail.gmail.com","subject":"Re: [PATCH v2 0/2] avoid pipes with Git on LHS","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-23T18:01:10Z","receivedAt":"2022-02-23T18:01:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Feb 23, 2022 at 09:19:23PM +0800, Shaoxuan Yuan wrote:\n> On Wed, Feb 23, 2022 at 8:42 PM Shubham Mishra <shivam828787@gmail.com> wrote:\n> >\n> > Hi,\n>\n> Hi,\n>\n> > I am using mails for code review for the first time, I have some\n> > doubts, Can someone please clarify them? -\n> > 1. It looks like the cover letter (Including \"Range-diff\" section) is\n> > only for context sharing with reviewers, nothing from it gets merged\n> > to the \"seek\" or any other branch.\n>\n> The cover letter stands for an introduction/summary to your patches.\n> You can also put helpful context in it for better understanding. According\n> to my knowledge, it will not be in the commit messages.\n\nRight; the cover letter (along with any notes below the '---' in your\npatches do not make it into the commit history).\n\nThe range-diff you posted is empty and doesn't look quite right to me...\nwhen I applied both versions of your patches locally and generated a\nrange-diff myself, I got the expected (non-empty) results.\n\nI'm not sure exactly how you're generating the range-diff locally, but\nyou may want to make sure that you're picking the previous version\ncorrectly (and not doing something like `git range-diff master HEAD\nHEAD`, which is what I suspect may have happened).\n\n> > 2. I wanted to know how the merging process takes place. Once the\n> > patch is accepted, do we merge all previous versions of it one after\n> > another or every patch is independent so we have to just merge the\n> > last accepted patch?\n>\n> Not so sure about this question. My two cents: generally the most agreed-upon\n> patch will be merged, but the exact merging process could vary based\n> on the circumstances. Probably Junio can have a better answer to this.\n\nEmily Shaffer did some great work a couple of years ago on a \"My First\nContribution\" tutorial, which you can find in\nDocumentation/MyFirstContribution.txt.\n\nThe section \"My Patch Got Emailed - Now What?\" provides a good overview\nof the review and queuing process.\n\nThanks,\nTaylor\n"},{"id":"449293","messageId":"CAC316V5nWB8_ctk6Wz9j_xPMogomktuDdXZkPz2TodqRVXZPhQ@mail.gmail.com","threadId":"57458","inReplyTo":"YhZ0qOwtY1vUVTiN@nand.local","subject":"Re: [PATCH v2 1/2] t0001: avoid pipes with Git on LHS","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-23T18:01:41Z","receivedAt":"2022-02-23T18:01:57Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"Ah, It's dumb, I'm sorry. This was unintended. The regex I used gave\nme all Git on LHS of pipe including this.\nI understand it's not a Git command so we don't need to fix this. I\nwould say We shouldn't merge it.\nI will take care to have a final eye on my patch before sending.\nSorry Again.\n\nThanks,\nShubham\n\nOn Wed, Feb 23, 2022 at 11:23 PM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> On Wed, Feb 23, 2022 at 05:23:46PM +0530, Shubham Mishra wrote:\n> > Pipes ignore error codes of LHS command and thu`s we should not use\n> > them with Git in tests. As an alternative, use a 'tmp' file to write\n> > the Git output so we can test the exit code.\n>\n> This patch does preserve the existing behavior. But I'm hesitant to\n> recommend that we apply this patch, since our test suite assumes that\n> commands like find will work, and so we aren't concerned about squashing\n> any potential error codes when it's on the left-hand side of the pipe,\n> since we assume that it won't fail in general.\n>\n> (That's notably different from the second patch in this series, where\n> the thing on the left-hand side of the pipe is a git invocation. In that\n> case, we really _do_ want to avoid having it on the left-hand side of\n> the pipe, because we don't have the same error-free expectation there,\n> and want to know when it fails).\n>\n> I think that Ævar gave a nice summary of the above in [1]\n>\n> Thanks,\n> Taylor\n>\n> [1]: https://lore.kernel.org/git/220222.86pmnf6ket.gmgdl@evledraar.gmail.com/\n"},{"id":"449294","messageId":"YhZ207PO0jOmaD6V@nand.local","threadId":"57458","inReplyTo":"CAC316V5nWB8_ctk6Wz9j_xPMogomktuDdXZkPz2TodqRVXZPhQ@mail.gmail.com","subject":"Re: [PATCH v2 1/2] t0001: avoid pipes with Git on LHS","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-23T18:02:59Z","receivedAt":"2022-02-23T18:03:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Feb 23, 2022 at 11:31:41PM +0530, Shubham Mishra wrote:\n> Ah, It's dumb, I'm sorry. This was unintended. The regex I used gave\n> me all Git on LHS of pipe including this.\n\nIt's certainly not dumb :-). You are doing a great job on your first\ncontribution!\n\nThanks,\nTaylor\n"},{"id":"449333","messageId":"CAC316V4NkdnqM99POmUELUeSHukgQDyGk_h_BcLs0Ud5igvnFw@mail.gmail.com","threadId":"57458","inReplyTo":"YhZ0yXFIZNw/us6d@nand.local","subject":"Re: [PATCH v2 2/2] t0003: avoid pipes with Git on LHS","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-23T19:59:47Z","receivedAt":"2022-02-23T20:00:03Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"Thanks for reviewing it :)\n\nOn Wed, Feb 23, 2022 at 11:24 PM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> On Wed, Feb 23, 2022 at 05:23:47PM +0530, Shubham Mishra wrote:\n> >  t/t0003-attributes.sh | 9 ++++++---\n> >  1 file changed, 6 insertions(+), 3 deletions(-)\n>\n> This patch looks great to me, thanks!\n>\n>     Reviewed-by: Taylor Blau <me@ttaylorr.com>\n>\n> Thanks,\n> Taylor\n"},{"id":"449334","messageId":"CAC316V51meeyvY=xU=a+aQ_K=m0ns=wJdqYKRDWOUmYrXX+B7w@mail.gmail.com","threadId":"57458","inReplyTo":"YhZ2Zpl3XGkTKxuI@nand.local","subject":"Re: [PATCH v2 0/2] avoid pipes with Git on LHS","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-23T20:04:10Z","receivedAt":"2022-02-23T20:04:26Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"Thanks to both of you, your responded were really insightful.\n\n> The range-diff you posted is empty and doesn't look quite right to me...\n> when I applied both versions of your patches locally and generated a\n> range-diff myself, I got the expected (non-empty) results.\n\nThis is a new command for me, I am looking on the web about range-diff\nto know what I did wrong :)\n\nThanks,\nShubham\n"},{"id":"449339","messageId":"xmqqsfs9thq7.fsf@gitster.g","threadId":"57458","inReplyTo":"CAP8UFD1FVuoe7V3JU+hExsbjWTkNZFCZFV4qc65YLd4ow-Da5w@mail.gmail.com","subject":"Re: [PATCH 1/2] t0001: remove pipes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-23T21:07:12Z","receivedAt":"2022-02-23T21:07:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> Only the exit code of the command before the pipe is ignored.\n>\n> Also it's ok to use pipes if the command before the pipe is not `git`.\n> We trust regular commands to just work and we test only `git`.\n\nBoth very good points.  So ...\n\n>> -               find .git/worktrees -print | sort >expected &&\n>> +               find .git/worktrees -print >tmp && sort tmp >expected &&\n>\n> Please put the `find` and `sort` commands on 2 different lines when\n> they are separated with &&.\n\nWe do not want to split this pipeline into two commands.\n"},{"id":"449340","messageId":"xmqqo82xthjy.fsf@gitster.g","threadId":"57458","inReplyTo":"20220223115347.3083-2-shivam828787@gmail.com","subject":"Re: [PATCH v2 1/2] t0001: avoid pipes with Git on LHS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-23T21:10:57Z","receivedAt":"2022-02-23T21:11:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shubham Mishra <shivam828787@gmail.com> writes:\n\n> Pipes ignore error codes of LHS command and thu`s we should not use\n> them with Git in tests. As an alternative, use a 'tmp' file to write\n> the Git output so we can test the exit code.\n\nI do not know what thu`s is, but the above describes a sensible\ncriterion to decide which pipe to touch and which pipe to leave\nalone.\n\n> -\t\tfind .git/worktrees -print | sort >expected &&\n> +\t\tfind .git/worktrees -print >tmp &&\n> +\t\tsort tmp >expected &&\n\nAnd this change or ...\n\n>  \t\tgit -C ../linked-worktree init &&\n>  \t\ttest_cmp expected-exclude .git/info/exclude &&\n>  \t\ttest_cmp expected-config .git/config &&\n> -\t\tfind .git/worktrees -print | sort >actual &&\n> +\t\tfind .git/worktrees -print >tmp &&\n> +\t\tsort tmp >actual &&\n\n... this change squarely contradict the reasoning written in the\nproposed log message.  These pipelines place \"find\" on the upstream\nof their pipe, and \"find\" is not something we are worried about\nintroducing new bug into.\n\n>  \t\ttest_cmp expected actual\n>  \t)\n>  '\n"},{"id":"449383","messageId":"CAP8UFD0miSWhksE8aWUY6awhf4sCNSO1mfPni2EEi94fzFnyoA@mail.gmail.com","threadId":"57458","inReplyTo":"xmqqsfs9thq7.fsf@gitster.g","subject":"Re: [PATCH 1/2] t0001: remove pipes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-24T03:28:58Z","receivedAt":"2022-02-24T03:29:14Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Feb 23, 2022 at 10:07 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n> > Only the exit code of the command before the pipe is ignored.\n> >\n> > Also it's ok to use pipes if the command before the pipe is not `git`.\n> > We trust regular commands to just work and we test only `git`.\n>\n> Both very good points.  So ...\n>\n> >> -               find .git/worktrees -print | sort >expected &&\n> >> +               find .git/worktrees -print >tmp && sort tmp >expected &&\n> >\n> > Please put the `find` and `sort` commands on 2 different lines when\n> > they are separated with &&.\n>\n> We do not want to split this pipeline into two commands.\n\nYeah, right.\n"},{"id":"449385","messageId":"CAP8UFD2wNSJmiohfAGrihm5sO_q4a6UpLMsVNAF5uWyuWt7R9Q@mail.gmail.com","threadId":"57458","inReplyTo":"20220222114313.14921-1-shivam828787@gmail.com","subject":"Re: [PATCH 0/2] microproject: avoid using pipes in test","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-24T03:43:23Z","receivedAt":"2022-02-24T03:43:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Feb 22, 2022 at 6:01 PM Shubham Mishra <shivam828787@gmail.com> wrote:\n>\n> pipes doesn't care about error codes and ignore them thus we should not use them in tests.\n> As an easy alternative, I am using a tmp file to write from git command so we can test the exit code.\n\n(Others have already rightly commented on many things, so I am only\nfocusing on microproject related things.)\n\nAs the subject of this cover letter starts with \"microproject:\" I\nwould suggest taking a look (or maybe reading again) our page about\nmicroprojects: https://git.github.io/General-Microproject-Information/\n\nPlease note that we ask that all related emails start with “[GSoC]” or\n“[Outreachy]” or something similar, not \"microproject:\".\n\n> This is my first contribution that's why I am keeping diff short with an intention to understand the process instead of making impactful change in first attempt. Later, I will send more patches to fix the issue for other files.\n\nPlease read the \"Only ONE quality focused microproject per student\"\nsection of the above mentioned page.\n\nThanks for your first contribution!\n"},{"id":"449386","messageId":"CAC316V7QDgZL0jOzgE-Ae86Phmf4XpeQdbKfNS3uA8uL_0uzKw@mail.gmail.com","threadId":"57458","inReplyTo":"xmqqo82xthjy.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] t0001: avoid pipes with Git on LHS","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-24T05:14:09Z","receivedAt":"2022-02-24T05:14:26Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"HI Junio,\n I acknowledge, this patch is here by mistake, we don't need to merge it.\n\nThanks,\nShubham\n\nOn Thu, Feb 24, 2022 at 2:40 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Shubham Mishra <shivam828787@gmail.com> writes:\n>\n> > Pipes ignore error codes of LHS command and thu`s we should not use\n> > them with Git in tests. As an alternative, use a 'tmp' file to write\n> > the Git output so we can test the exit code.\n>\n> I do not know what thu`s is, but the above describes a sensible\n> criterion to decide which pipe to touch and which pipe to leave\n> alone.\n>\n> > -             find .git/worktrees -print | sort >expected &&\n> > +             find .git/worktrees -print >tmp &&\n> > +             sort tmp >expected &&\n>\n> And this change or ...\n>\n> >               git -C ../linked-worktree init &&\n> >               test_cmp expected-exclude .git/info/exclude &&\n> >               test_cmp expected-config .git/config &&\n> > -             find .git/worktrees -print | sort >actual &&\n> > +             find .git/worktrees -print >tmp &&\n> > +             sort tmp >actual &&\n>\n> ... this change squarely contradict the reasoning written in the\n> proposed log message.  These pipelines place \"find\" on the upstream\n> of their pipe, and \"find\" is not something we are worried about\n> introducing new bug into.\n>\n> >               test_cmp expected actual\n> >       )\n> >  '\n"},{"id":"449387","messageId":"CAC316V5x54Ah566haa5wZ8zE=gkhYtziC+FVjXoBXn7xY_-WsQ@mail.gmail.com","threadId":"57458","inReplyTo":"CAP8UFD2wNSJmiohfAGrihm5sO_q4a6UpLMsVNAF5uWyuWt7R9Q@mail.gmail.com","subject":"Re: [PATCH 0/2] microproject: avoid using pipes in test","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-24T05:22:06Z","receivedAt":"2022-02-24T05:22:30Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"> (Others have already rightly commented on many things, so I am only\n> focusing on microproject related things.)\n>\n> As the subject of this cover letter starts with \"microproject:\" I\n> would suggest taking a look (or maybe reading again) our page about\n> microprojects: https://git.github.io/General-Microproject-Information/\n>\n> Please note that we ask that all related emails start with “[GSoC]” or\n> “[Outreachy]” or something similar, not \"microproject:\".\n\nAck.\n\n> > This is my first contribution that's why I am keeping diff short with an intention to understand the process instead of making impactful change in first attempt. Later, I will send more patches to fix the issue for other files.\n>\n> Please read the \"Only ONE quality focused microproject per student\"\n> section of the above mentioned page.\n\nhere I mean in next patches, I will be fixing the same \"Git on LHS of\npipe\" for other tests. I think that will be considered as the same\nmicroproject and I am still eligible to fix the same thing for other\nfiles?\n\nThanks,\nShubham\n"},{"id":"449403","messageId":"CAP8UFD1dfXHYudpT=0e_2DZdmZMKyqKO13EbPozwpZkEYewxNw@mail.gmail.com","threadId":"57458","inReplyTo":"CAC316V5x54Ah566haa5wZ8zE=gkhYtziC+FVjXoBXn7xY_-WsQ@mail.gmail.com","subject":"Re: [PATCH 0/2] microproject: avoid using pipes in test","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-24T09:29:26Z","receivedAt":"2022-02-24T09:29:42Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Feb 24, 2022 at 6:22 AM Shubham Mishra <shivam828787@gmail.com> wrote:\n\n> > > This is my first contribution that's why I am keeping diff short with an intention to understand the process instead of making impactful change in first attempt. Later, I will send more patches to fix the issue for other files.\n> >\n> > Please read the \"Only ONE quality focused microproject per student\"\n> > section of the above mentioned page.\n>\n> here I mean in next patches, I will be fixing the same \"Git on LHS of\n> pipe\" for other tests. I think that will be considered as the same\n> microproject and I am still eligible to fix the same thing for other\n> files?\n\nIn the \"Only ONE quality focused microproject per student\" section, there is:\n\n=> This means that for a microproject that consist in refactoring or\nrewriting a small amount of code, your patch should change only ONE\nfile, or perhaps 2 files if they are closely related, like “foo.c” and\n“foo.h”.\n\nSo, no, we really prefer you to focus on 1 file (or 2 files when they\nare strongly related) and then move on to your application or regular\npatches, reviews, discussions, etc.\n"},{"id":"449435","messageId":"CAC316V42eP2GEuT+=yvi+Rbdni4KTJzhzUGHgCtcdWkGE9O-Ug@mail.gmail.com","threadId":"57458","inReplyTo":"CAP8UFD1dfXHYudpT=0e_2DZdmZMKyqKO13EbPozwpZkEYewxNw@mail.gmail.com","subject":"Re: [PATCH 0/2] microproject: avoid using pipes in test","fromName":"Shubham Mishra","fromEmail":"shivam828787@gmail.com","sentAt":"2022-02-24T10:13:28Z","receivedAt":"2022-02-24T10:13:48Z","isPatch":true,"sender":{"key":"shivam828787@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42690084?v=4"},"body":"> So, no, we really prefer you to focus on 1 file (or 2 files when they\n> are strongly related) and then move on to your application or regular\n> patches, reviews, discussions, etc.\n\nI misunderstood this :(. I will not submit any further patches related\nto microprojects but I already sentone patch at Morning -\nhttps://lore.kernel.org/git/20220224054720.23996-1-shivam828787@gmail.com.\nCan I finish this?.\nThank you for clarifying.\n\nThanks,\nShubham.\n"},{"id":"449489","messageId":"CAP8UFD0cCDhfeO8xZCrhdodym2sMAGQfH0O0U-rPo-NqyBvu9g@mail.gmail.com","threadId":"57458","inReplyTo":"CAC316V42eP2GEuT+=yvi+Rbdni4KTJzhzUGHgCtcdWkGE9O-Ug@mail.gmail.com","subject":"Re: [PATCH 0/2] microproject: avoid using pipes in test","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-24T18:22:17Z","receivedAt":"2022-02-24T18:22:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Feb 24, 2022 at 11:13 AM Shubham Mishra <shivam828787@gmail.com> wrote:\n>\n> > So, no, we really prefer you to focus on 1 file (or 2 files when they\n> > are strongly related) and then move on to your application or regular\n> > patches, reviews, discussions, etc.\n>\n> I misunderstood this :(. I will not submit any further patches related\n> to microprojects but I already sentone patch at Morning -\n> https://lore.kernel.org/git/20220224054720.23996-1-shivam828787@gmail.com.\n> Can I finish this?.\n\nYeah, you can finish it. As I said to another applicant, we are even\nlikely to help you finish it.\n\nBest,\nChristian.\n"}]}