{"thread":{"id":"49858","subject":"[PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure","startedAt":"2018-11-19T13:28:30Z","lastAt":"2018-11-20T11:37:32Z","messageCount":5,"participants":["SZEDER Gábor","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"363635","messageId":"20181119132818.3116-1-szeder.dev@gmail.com","threadId":"49858","inReplyTo":null,"subject":"[PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-11-19T13:28:18Z","receivedAt":"2018-11-19T13:28:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The 'test_cmp_rev' helper is merely a wrapper around 'test_cmp'\nchecking the output of two 'git rev-parse' commands, which means that\nits output on failure is not particularly informative, as it's\nbasically two OIDs with a bit of extra clutter of the diff header, but\nwithout any indication of which two revisions have caused the failure:\n\n  --- expect.rev  2018-11-17 14:02:11.569747033 +0000\n  +++ actual.rev  2018-11-17 14:02:11.569747033 +0000\n  @@ -1 +1 @@\n  -d79ce1670bdcb76e6d1da2ae095e890ccb326ae9\n  +139b20d8e6c5b496de61f033f642d0e3dbff528d\n\nIt also pollutes the test repo with these two intermediate files,\nthough that doesn't seem to cause any complications in our current\ntests (meaning that I couldn't find any tests that have to work around\nthe presence of these files by explicitly removing or ignoring them).\n\nEnhance 'test_cmp_rev' to provide a more useful output on failure with\nless clutter:\n\n  error: two revisions point to different objects:\n    'HEAD^': d79ce1670bdcb76e6d1da2ae095e890ccb326ae9\n    'extra': 139b20d8e6c5b496de61f033f642d0e3dbff528d\n\nDoing so is more convenient when storing the OIDs outputted by 'git\nrev-parse' in a local variable each, which, as a bonus, won't pollute\nthe repository with intermediate files.\n\nWhile at it, also ensure that 'test_cmp_rev' is invoked with the right\nnumber of parameters, namely two.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/test-lib-functions.sh | 20 +++++++++++++++++---\n 1 file changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex d158c8d0bf..fc84db67a1 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -854,9 +854,23 @@ test_must_be_empty () {\n \n # Tests that its two parameters refer to the same revision\n test_cmp_rev () {\n-\tgit rev-parse --verify \"$1\" >expect.rev &&\n-\tgit rev-parse --verify \"$2\" >actual.rev &&\n-\ttest_cmp expect.rev actual.rev\n+\tif test $# != 2\n+\tthen\n+\t\terror \"bug in the test script: test_cmp_rev requires two revisions, but got $#\"\n+\telse\n+\t\tlocal r1 r2\n+\t\tr1=$(git rev-parse --verify \"$1\") &&\n+\t\tr2=$(git rev-parse --verify \"$2\") &&\n+\t\tif test \"$r1\" != \"$r2\"\n+\t\tthen\n+\t\t\tcat >&4 <<-EOF\n+\t\t\terror: two revisions point to different objects:\n+\t\t\t  '$1': $r1\n+\t\t\t  '$2': $r2\n+\t\t\tEOF\n+\t\t\treturn 1\n+\t\tfi\n+\tfi\n }\n \n # Print a sequence of integers in increasing order, either with\n-- \n2.20.0.rc0.134.gf0022f8e60\n\n"},{"id":"363636","messageId":"20181119133102.GS30222@szeder.dev","threadId":"49858","inReplyTo":"20181119132818.3116-1-szeder.dev@gmail.com","subject":"Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-11-19T13:31:02Z","receivedAt":"2018-11-19T13:31:08Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Nov 19, 2018 at 02:28:18PM +0100, SZEDER Gábor wrote:\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index d158c8d0bf..fc84db67a1 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -854,9 +854,23 @@ test_must_be_empty () {\n>  \n>  # Tests that its two parameters refer to the same revision\n>  test_cmp_rev () {\n> -\tgit rev-parse --verify \"$1\" >expect.rev &&\n> -\tgit rev-parse --verify \"$2\" >actual.rev &&\n> -\ttest_cmp expect.rev actual.rev\n> +\tif test $# != 2\n> +\tthen\n> +\t\terror \"bug in the test script: test_cmp_rev requires two revisions, but got $#\"\n\nAnd this here is another \"bug in the test script\" error that should be\nturned into:\n\n  BUG \"test_cmp_rev requires two revisions, but got $#\"\n\nSee: https://public-inbox.org/git/20181119131326.2435-1-szeder.dev@gmail.com/\n\n"},{"id":"363665","messageId":"20181119194920.GB7330@sigill.intra.peff.net","threadId":"49858","inReplyTo":"20181119132818.3116-1-szeder.dev@gmail.com","subject":"Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-19T19:49:20Z","receivedAt":"2018-11-19T19:49:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 19, 2018 at 02:28:18PM +0100, SZEDER Gábor wrote:\n\n> The 'test_cmp_rev' helper is merely a wrapper around 'test_cmp'\n> checking the output of two 'git rev-parse' commands, which means that\n> its output on failure is not particularly informative, as it's\n> basically two OIDs with a bit of extra clutter of the diff header, but\n> without any indication of which two revisions have caused the failure:\n> \n>   --- expect.rev  2018-11-17 14:02:11.569747033 +0000\n>   +++ actual.rev  2018-11-17 14:02:11.569747033 +0000\n>   @@ -1 +1 @@\n>   -d79ce1670bdcb76e6d1da2ae095e890ccb326ae9\n>   +139b20d8e6c5b496de61f033f642d0e3dbff528d\n> \n> It also pollutes the test repo with these two intermediate files,\n> though that doesn't seem to cause any complications in our current\n> tests (meaning that I couldn't find any tests that have to work around\n> the presence of these files by explicitly removing or ignoring them).\n> \n> Enhance 'test_cmp_rev' to provide a more useful output on failure with\n> less clutter:\n> \n>   error: two revisions point to different objects:\n>     'HEAD^': d79ce1670bdcb76e6d1da2ae095e890ccb326ae9\n>     'extra': 139b20d8e6c5b496de61f033f642d0e3dbff528d\n> \n> Doing so is more convenient when storing the OIDs outputted by 'git\n> rev-parse' in a local variable each, which, as a bonus, won't pollute\n> the repository with intermediate files.\n> \n> While at it, also ensure that 'test_cmp_rev' is invoked with the right\n> number of parameters, namely two.\n\nThis is an improvement, in my opinion (and I agree that using your new\nBUG for this last part would be better still). It also saves a process\nin the common case.\n\nOne question:\n\n> +\telse\n> +\t\tlocal r1 r2\n> +\t\tr1=$(git rev-parse --verify \"$1\") &&\n> +\t\tr2=$(git rev-parse --verify \"$2\") &&\n> +\t\tif test \"$r1\" != \"$r2\"\n> +\t\tthen\n> +\t\t\tcat >&4 <<-EOF\n> +\t\t\terror: two revisions point to different objects:\n> +\t\t\t  '$1': $r1\n> +\t\t\t  '$2': $r2\n> +\t\t\tEOF\n> +\t\t\treturn 1\n> +\t\tfi\n\nWhy does this cat go to descriptor 4? I get why you'd want it to (it's\nmeant for the user's eyes, and that's where 4 goes), but we do not\nusually bother to do so for our helper functions (like test_cmp).\n\nI don't think it matters usually in practice, because nobody tries to\ncapture the stderr of test_cmp, etc. I don't think it would ever hurt,\nthough. Should we be doing that for all the others, too?\n\n-Peff\n"},{"id":"363725","messageId":"20181120112538.GX30222@szeder.dev","threadId":"49858","inReplyTo":"20181119194920.GB7330@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-11-20T11:25:38Z","receivedAt":"2018-11-20T11:25:45Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Nov 19, 2018 at 02:49:20PM -0500, Jeff King wrote:\n> On Mon, Nov 19, 2018 at 02:28:18PM +0100, SZEDER Gábor wrote:\n> \n> > The 'test_cmp_rev' helper is merely a wrapper around 'test_cmp'\n> > checking the output of two 'git rev-parse' commands, which means that\n> > its output on failure is not particularly informative, as it's\n> > basically two OIDs with a bit of extra clutter of the diff header, but\n> > without any indication of which two revisions have caused the failure:\n> > \n> >   --- expect.rev  2018-11-17 14:02:11.569747033 +0000\n> >   +++ actual.rev  2018-11-17 14:02:11.569747033 +0000\n> >   @@ -1 +1 @@\n> >   -d79ce1670bdcb76e6d1da2ae095e890ccb326ae9\n> >   +139b20d8e6c5b496de61f033f642d0e3dbff528d\n> > \n> > It also pollutes the test repo with these two intermediate files,\n> > though that doesn't seem to cause any complications in our current\n> > tests (meaning that I couldn't find any tests that have to work around\n> > the presence of these files by explicitly removing or ignoring them).\n> > \n> > Enhance 'test_cmp_rev' to provide a more useful output on failure with\n> > less clutter:\n> > \n> >   error: two revisions point to different objects:\n> >     'HEAD^': d79ce1670bdcb76e6d1da2ae095e890ccb326ae9\n> >     'extra': 139b20d8e6c5b496de61f033f642d0e3dbff528d\n> > \n> > Doing so is more convenient when storing the OIDs outputted by 'git\n> > rev-parse' in a local variable each, which, as a bonus, won't pollute\n> > the repository with intermediate files.\n> > \n> > While at it, also ensure that 'test_cmp_rev' is invoked with the right\n> > number of parameters, namely two.\n> \n> This is an improvement, in my opinion (and I agree that using your new\n> BUG for this last part would be better still). It also saves a process\n> in the common case.\n\nBut then it adds two subshells to the common case right away...  I'm\nnot sure which is worse on Windows, where it matters the most.\n\n> One question:\n> \n> > +\telse\n> > +\t\tlocal r1 r2\n> > +\t\tr1=$(git rev-parse --verify \"$1\") &&\n> > +\t\tr2=$(git rev-parse --verify \"$2\") &&\n> > +\t\tif test \"$r1\" != \"$r2\"\n> > +\t\tthen\n> > +\t\t\tcat >&4 <<-EOF\n> > +\t\t\terror: two revisions point to different objects:\n> > +\t\t\t  '$1': $r1\n> > +\t\t\t  '$2': $r2\n> > +\t\t\tEOF\n> > +\t\t\treturn 1\n> > +\t\tfi\n> \n> Why does this cat go to descriptor 4? I get why you'd want it to (it's\n> meant for the user's eyes, and that's where 4 goes),\n\nExactly.\n\n> but we do not\n> usually bother to do so for our helper functions (like test_cmp).\n\ntest_i18ngrep() does since your 03aa3783f2 (t: send verbose\ntest-helper output to fd 4, 2018-02-22).\n\n> I don't think it matters usually in practice, because nobody tries to\n> capture the stderr of test_cmp, etc.\n\nSee 54ce2e9be9 (t9400-git-cvsserver-server: don't rely on the output\nof 'test_cmp', 2018-03-08) for a fun example, I remember it caused a\nspike in WTF/minutes :)\n\n\n> I don't think it would ever hurt,\n> though. Should we be doing that for all the others, too?\n\nI think we should, and you agreed:\n\nhttps://public-inbox.org/git/20180303065648.GA17312@sigill.intra.peff.net/\n\nbut then went travelling, and then the whole thing got forgotten.\n\n"},{"id":"363727","messageId":"20181120113728.GB29319@sigill.intra.peff.net","threadId":"49858","inReplyTo":"20181120112538.GX30222@szeder.dev","subject":"Re: [PATCH] test-lib-functions: make 'test_cmp_rev' more informative on failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-20T11:37:29Z","receivedAt":"2018-11-20T11:37:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 20, 2018 at 12:25:38PM +0100, SZEDER Gábor wrote:\n\n> > but we do not\n> > usually bother to do so for our helper functions (like test_cmp).\n> \n> test_i18ngrep() does since your 03aa3783f2 (t: send verbose\n> test-helper output to fd 4, 2018-02-22).\n\nOh, indeed. Usually I find myself forgetting about patches I worked on\nfrom 5 years ago. Forgetting about one from 9 months ago is a new low.\n:)\n\n> > I don't think it would ever hurt,\n> > though. Should we be doing that for all the others, too?\n> \n> I think we should, and you agreed:\n> \n> https://public-inbox.org/git/20180303065648.GA17312@sigill.intra.peff.net/\n> \n> but then went travelling, and then the whole thing got forgotten.\n\nStupid vacation. :)\n\nOK, yes, I reaffirm my agreement that this is the right direction, then.\nSorry for my lack of memory.\n\n-Peff\n"}]}