{"thread":{"id":"50859","subject":"[GSoC][PATCH v1] t9811: avoid pipe in git commands in test script","startedAt":"2019-04-02T01:41:37Z","lastAt":"2019-04-02T20:53:01Z","messageCount":3,"participants":["Khalid Ali","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"372961","messageId":"20190402014115.22478-1-khalludi123@gmail.com","threadId":"50859","inReplyTo":null,"subject":"[GSoC][PATCH v1] t9811: avoid pipe in git commands in test script","fromName":"Khalid Ali","fromEmail":"khalludi123@gmail.com","sentAt":"2019-04-02T01:41:15Z","receivedAt":"2019-04-02T01:41:37Z","isPatch":true,"sender":{"key":"khalludi123@gmail.com","avatar":null},"body":"The exit code of the upstream in a pipe is ignored thus we\nshould avoid using it. By writing out the output of the git command to a\nfile, we can test the exit codes of both the commands.\n\nAside from the commit message, I plan to apply for GSoC. Planning to\nsolve the rebase/cherry-pick issue or adding functionality for the\nrebase interactive command. \n\nAny review on this commit would be appreciated!\n\nSigned-off-by: Khalid Ali <khalludi123@gmail.com>\n---\n t/t9811-git-p4-label-import.sh | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\nindex 602b0a5d5c..2f4e80ed55 100755\n--- a/t/t9811-git-p4-label-import.sh\n+++ b/t/t9811-git-p4-label-import.sh\n@@ -63,7 +63,8 @@ test_expect_success 'basic p4 labels' '\n \t\tgit checkout TAG_WITH\\$_SHELL_CHAR &&\n \t\ttest -f f1 && test -f f2 && test -f file_with_\\$metachar &&\n \n-\t\tgit show TAG_LONG_LABEL | grep -q \"A Label second line\"\n+\t\tgit show TAG_LONG_LABEL >actual &&\n+\t\tgrep -q \"A Label second line\" actual\n \t)\n '\n # Test some label corner cases:\n@@ -92,9 +93,9 @@ test_expect_success 'two labels on the same changelist' '\n \t\tcd \"$git\" &&\n \t\tgit p4 sync --import-labels &&\n \n-\t\tgit tag | grep TAG_F1 &&\n-\t\tgit tag | grep -q TAG_F1_1 &&\n-\t\tgit tag | grep -q TAG_F1_2 &&\n+\t\tgit tag >actual && grep TAG_F1 actual &&\n+\t\tgit tag >actual && grep -q TAG_F1_1 actual &&\n+\t\tgit tag >actual && grep -q TAG_F1_2 actual &&\n \n \t\tcd main &&\n \n@@ -205,7 +206,7 @@ test_expect_success 'use git config to enable import/export of tags' '\n \t\tgit p4 rebase --verbose &&\n \t\tgit p4 submit --verbose &&\n \t\tgit tag &&\n-\t\tgit tag | grep TAG_F1_1\n+\t\tgit tag >actual && grep TAG_F1_1 actual\n \t) &&\n \t(\n \t\tcd \"$cli\" &&\n-- \n2.21.0.196.g041f5ea1cf\n\n"},{"id":"372963","messageId":"xmqqk1gdt9ke.fsf@gitster-ct.c.googlers.com","threadId":"50859","inReplyTo":"20190402014115.22478-1-khalludi123@gmail.com","subject":"Re: [GSoC][PATCH v1] t9811: avoid pipe in git commands in test script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-02T02:39:13Z","receivedAt":"2019-04-02T02:39:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Khalid Ali <khalludi123@gmail.com> writes:\n\n> The exit code of the upstream in a pipe is ignored thus we\n> should avoid using it. By writing out the output of the git command to a\n> file, we can test the exit codes of both the commands.\n\nEnd the log message here by moving the next two paragraphs below the\nthree-dash line.\n\n>\n> Aside from the commit message, I plan to apply for GSoC. Planning to\n> solve the rebase/cherry-pick issue or adding functionality for the\n> rebase interactive command. \n>\n> Any review on this commit would be appreciated!\n>\n> Signed-off-by: Khalid Ali <khalludi123@gmail.com>\n> ---\n>  t/t9811-git-p4-label-import.sh | 11 ++++++-----\n>  1 file changed, 6 insertions(+), 5 deletions(-)\n>\n> diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\n> index 602b0a5d5c..2f4e80ed55 100755\n> --- a/t/t9811-git-p4-label-import.sh\n> +++ b/t/t9811-git-p4-label-import.sh\n> @@ -63,7 +63,8 @@ test_expect_success 'basic p4 labels' '\n>  \t\tgit checkout TAG_WITH\\$_SHELL_CHAR &&\n>  \t\ttest -f f1 && test -f f2 && test -f file_with_\\$metachar &&\n>  \n> -\t\tgit show TAG_LONG_LABEL | grep -q \"A Label second line\"\n> +\t\tgit show TAG_LONG_LABEL >actual &&\n> +\t\tgrep -q \"A Label second line\" actual\n\nThere is no mention of file 'actual' before this patch, so we can\nreasonably be sure that we are not breaking expectations of existing\ntest that a file with that name has contents different from what the\nabove command produces.  On the other hand, if later tests have\nthings like \"git add .\" or \"git status -s\" and expects that there is\nno such file called 'actual', this change may have broken the\nexpectation.\n\nI *think* the above is done after going inside $git directory, which\nwill be removed with \"test_when_finished cleanup_git\", so it is\nfairly clear that leaving an extra file 'actual' behind at this\npoint is safe.\n\nThis is not a problem with this conversion, but we tend to avoid\nsquelching standard output to help debugging tests.  An independent\nclean-up patch may want to replace \"grep -q\" with just \"grep\".\n\n> @@ -92,9 +93,9 @@ test_expect_success 'two labels on the same changelist' '\n>  \t\tcd \"$git\" &&\n>  \t\tgit p4 sync --import-labels &&\n>  \n> -\t\tgit tag | grep TAG_F1 &&\n> -\t\tgit tag | grep -q TAG_F1_1 &&\n> -\t\tgit tag | grep -q TAG_F1_2 &&\n> +\t\tgit tag >actual && grep TAG_F1 actual &&\n> +\t\tgit tag >actual && grep -q TAG_F1_1 actual &&\n> +\t\tgit tag >actual && grep -q TAG_F1_2 actual &&\n\nDitto.\n\n>  \n>  \t\tcd main &&\n>  \n> @@ -205,7 +206,7 @@ test_expect_success 'use git config to enable import/export of tags' '\n>  \t\tgit p4 rebase --verbose &&\n>  \t\tgit p4 submit --verbose &&\n>  \t\tgit tag &&\n> -\t\tgit tag | grep TAG_F1_1\n> +\t\tgit tag >actual && grep TAG_F1_1 actual\n>  \t) &&\n\nI notice that this test piece does not call cleanup_git when it is\ndone.  I think that is a mistake (not introduced by this patch) that\nma want to get fixed independently.\n\n>  \t(\n>  \t\tcd \"$cli\" &&\n"},{"id":"372995","messageId":"CAD8q83TFHyTeVMUTfv8nJryGtBDKrz7xNbDirK3gyzYUihwzXA@mail.gmail.com","threadId":"50859","inReplyTo":"xmqqk1gdt9ke.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][PATCH v1] t9811: avoid pipe in git commands in test script","fromName":"Khalid Ali","fromEmail":"khalludi123@gmail.com","sentAt":"2019-04-02T20:52:46Z","receivedAt":"2019-04-02T20:53:01Z","isPatch":true,"sender":{"key":"khalludi123@gmail.com","avatar":null},"body":"Thanks for the feedback Junio.\n\nI was wondering if I should resubmit the patch without my comments.\nAlso, I was wondering if I should be adding a call to check the exit codes\nof this test and the file called 'actual' at the end of the script.\n\nOn Mon, Apr 1, 2019 at 10:39 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Khalid Ali <khalludi123@gmail.com> writes:\n>\n> > The exit code of the upstream in a pipe is ignored thus we\n> > should avoid using it. By writing out the output of the git command to a\n> > file, we can test the exit codes of both the commands.\n>\n> End the log message here by moving the next two paragraphs below the\n> three-dash line.\n>\n> >\n> > Aside from the commit message, I plan to apply for GSoC. Planning to\n> > solve the rebase/cherry-pick issue or adding functionality for the\n> > rebase interactive command.\n> >\n> > Any review on this commit would be appreciated!\n> >\n> > Signed-off-by: Khalid Ali <khalludi123@gmail.com>\n> > ---\n> >  t/t9811-git-p4-label-import.sh | 11 ++++++-----\n> >  1 file changed, 6 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/t/t9811-git-p4-label-import.sh b/t/t9811-git-p4-label-import.sh\n> > index 602b0a5d5c..2f4e80ed55 100755\n> > --- a/t/t9811-git-p4-label-import.sh\n> > +++ b/t/t9811-git-p4-label-import.sh\n> > @@ -63,7 +63,8 @@ test_expect_success 'basic p4 labels' '\n> >               git checkout TAG_WITH\\$_SHELL_CHAR &&\n> >               test -f f1 && test -f f2 && test -f file_with_\\$metachar &&\n> >\n> > -             git show TAG_LONG_LABEL | grep -q \"A Label second line\"\n> > +             git show TAG_LONG_LABEL >actual &&\n> > +             grep -q \"A Label second line\" actual\n>\n> There is no mention of file 'actual' before this patch, so we can\n> reasonably be sure that we are not breaking expectations of existing\n> test that a file with that name has contents different from what the\n> above command produces.  On the other hand, if later tests have\n> things like \"git add .\" or \"git status -s\" and expects that there is\n> no such file called 'actual', this change may have broken the\n> expectation.\n>\n> I *think* the above is done after going inside $git directory, which\n> will be removed with \"test_when_finished cleanup_git\", so it is\n> fairly clear that leaving an extra file 'actual' behind at this\n> point is safe.\n>\n> This is not a problem with this conversion, but we tend to avoid\n> squelching standard output to help debugging tests.  An independent\n> clean-up patch may want to replace \"grep -q\" with just \"grep\".\n>\n> > @@ -92,9 +93,9 @@ test_expect_success 'two labels on the same changelist' '\n> >               cd \"$git\" &&\n> >               git p4 sync --import-labels &&\n> >\n> > -             git tag | grep TAG_F1 &&\n> > -             git tag | grep -q TAG_F1_1 &&\n> > -             git tag | grep -q TAG_F1_2 &&\n> > +             git tag >actual && grep TAG_F1 actual &&\n> > +             git tag >actual && grep -q TAG_F1_1 actual &&\n> > +             git tag >actual && grep -q TAG_F1_2 actual &&\n>\n> Ditto.\n>\n> >\n> >               cd main &&\n> >\n> > @@ -205,7 +206,7 @@ test_expect_success 'use git config to enable import/export of tags' '\n> >               git p4 rebase --verbose &&\n> >               git p4 submit --verbose &&\n> >               git tag &&\n> > -             git tag | grep TAG_F1_1\n> > +             git tag >actual && grep TAG_F1_1 actual\n> >       ) &&\n>\n> I notice that this test piece does not call cleanup_git when it is\n> done.  I think that is a mistake (not introduced by this patch) that\n> ma want to get fixed independently.\n>\n> >       (\n> >               cd \"$cli\" &&\n"}]}