{"thread":{"id":"59927","subject":"[PATCH] t4205: correctly test %(describe:abbrev=...)","startedAt":"2023-06-28T18:18:22Z","lastAt":"2023-06-29T13:41:05Z","messageCount":4,"participants":["Kousik Sanagavarapu","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"478972","messageId":"20230628181753.10384-1-five231003@gmail.com","threadId":"59927","inReplyTo":null,"subject":"[PATCH] t4205: correctly test %(describe:abbrev=...)","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-06-28T18:16:59Z","receivedAt":"2023-06-28T18:18:22Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"The pretty format %(describe:abbrev=) tells describe to use only\n characters of the oid to generate the human-readable format of\nthe commit-ish.\n\nThis is not apparent in the test for %(describe:abbrev=...) because we\ndirectly tag HEAD and use that, in which case the human-readable format\nis just the tag name. So, create a new commit and use that instead.\n\nMentored-by: Christian Couder \nMentored-by: Hariom Verma \nSigned-off-by: Kousik Sanagavarapu \n---\n t/t4205-log-pretty-formats.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 4cf8a77667..b631b5a142 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -1011,8 +1011,7 @@ test_expect_success '%(describe:tags) vs git describe --tags' '\n '\n \n test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '\n-\ttest_when_finished \"git tag -d tagname\" &&\n-\tgit tag -a -m tagged tagname &&\n+\ttest_commit --no-tag file &&\n \tgit describe --abbrev=15 >expect &&\n \tgit log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n \ttest_cmp expect actual\n-- \n2.41.0.29.g8148156d44.dirty\n\n"},{"id":"478995","messageId":"xmqqv8f7b7h1.fsf@gitster.g","threadId":"59927","inReplyTo":"20230628181753.10384-1-five231003@gmail.com","subject":"Re: [PATCH] t4205: correctly test %(describe:abbrev=...)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-28T21:30:18Z","receivedAt":"2023-06-28T21:30:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu writes:\n\n> The pretty format %(describe:abbrev=) tells describe to use only\n> characters of the oid to generate the human-readable format of\n> the commit-ish.\n\nIs that *only* correct? I thought it was \"at least hexdigits\"\nto allow for future growth of the project.\n\n> This is not apparent in the test for %(describe:abbrev=...) because we\n> directly tag HEAD and use that, in which case the human-readable format\n> is just the tag name. So, create a new commit and use that instead.\n\nNice. How was this found, I have to wonder, and more importantly,\nhow would we have written this test in the first place to avoid\ntesting \"the wrong thing\", to learn from this experience?\n\n> test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '\n> -\ttest_when_finished \"git tag -d tagname\" &&\n> -\tgit tag -a -m tagged tagname &&\n> +\ttest_commit --no-tag file &&\n> \tgit describe --abbrev=15 >expect &&\n> \tgit log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n> \ttest_cmp expect actual\n\nThe current test checks that the output in the case where the number\nof commits since the tag is 0, and \"describe --abbrev=15\" and \"log\n--format='%(describe:abbrev=15)'\" give exactly the same result.\nWhich is a good thing to test.\n\nBut we *also* want to test a more typical case where there are\ncommits between HEAD and the tag that is used to describe it. \n\nAnd we *also* want to make sure that the hexadecimal object name\nsuffix used in the description is at least 15 hexdigits long, if not\nmore.\n\nThe updated test drops test #1 (which is questionable), adds test #2\n(which is good), and still omits test #3 (which is not so good). \n\nSo, perhaps\n\n test_when_finished \"git tag -d tagname\" &&\n- git tag -a -m tagged tagname &&\n test_commit --no-tag file &&\n git describe --abbrev=15 >expect &&\n git log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n test_cmp expect actual &&\n+ sed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n+ test 16 -le $(wc -c expect &&\n+ git log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n+ test_cmp expect actual &&\n+ test tagname = $(cat actual)\n\nor something along the line? First we test with a commit that is\nnot tagged at all to have some commits between the tag and HEAD with\nthe original comparison (this is for #2), then we make sure the\nlength of the hexpart (new---this is for #3), and then we add the\ntag to see the \"exact\" case also works (this is for #1).\n\nThanks.\n"},{"id":"479006","messageId":"ZJ1K-3ZFtmMJtW3r@five231003","threadId":"59927","inReplyTo":"xmqqv8f7b7h1.fsf@gitster.g","subject":"Re: [PATCH] t4205: correctly test %(describe:abbrev=...)","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-06-29T09:12:27Z","receivedAt":"2023-06-29T09:12:53Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Wed, Jun 28, 2023 at 02:30:18PM -0700, Junio C Hamano wrote:\n> Kousik Sanagavarapu writes:\n>\n> > The pretty format %(describe:abbrev=) tells describe to use only\n> > characters of the oid to generate the human-readable format of\n> > the commit-ish.\n>\n> Is that *only* correct? I thought it was \"at least hexdigits\"\n> to allow for future growth of the project.\n\nYeah, this is wrong. It should be \"at least\" for being more precise as\nwe may need more than in some cases. Will change that. Thanks\nfor catching it.\n\n> > This is not apparent in the test for %(describe:abbrev=...) because we\n> > directly tag HEAD and use that, in which case the human-readable format\n> > is just the tag name. So, create a new commit and use that instead.\n>\n> Nice. How was this found, I have to wonder, and more importantly,\n\nI was working on duplicating \"%(describe)\" from pretty, in ref-filter\nand was writing tests for it. While going through the trash directory\nfor some other breakage, I found this. So it was kind of a chance.\n\n> how would we have written this test in the first place to avoid\n> testing \"the wrong thing\", to learn from this experience?\n\nI don't have a clue :).\n\nIn this particular test, this is not \"the wrong thing\" anyways, as you\nexplain below. We just fail to test it wholly (we missed some cases).\n\n> > test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '\n> > - test_when_finished \"git tag -d tagname\" &&\n> > - git tag -a -m tagged tagname &&\n> > + test_commit --no-tag file &&\n> > git describe --abbrev=15 >expect &&\n> > git log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n> > test_cmp expect actual\n>\n> The current test checks that the output in the case where the number\n> of commits since the tag is 0, and \"describe --abbrev=15\" and \"log\n> --format='%(describe:abbrev=15)'\" give exactly the same result.\n> Which is a good thing to test.\n>\n> But we *also* want to test a more typical case where there are\n> commits between HEAD and the tag that is used to describe it.\n>\n> And we *also* want to make sure that the hexadecimal object name\n> suffix used in the description is at least 15 hexdigits long, if not\n> more.\n>\n> The updated test drops test #1 (which is questionable), adds test #2\n> (which is good), and still omits test #3 (which is not so good).\n>\n> So, perhaps\n>\n> test_when_finished \"git tag -d tagname\" &&\n> - git tag -a -m tagged tagname &&\n> test_commit --no-tag file &&\n> git describe --abbrev=15 >expect &&\n> git log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n> test_cmp expect actual &&\n> + sed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n> + test 16 -le $(wc -c +\n> + git tag -a -m tagged tagname &&\n> + git describe --abbrev=15 >expect &&\n> + git log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n> + test_cmp expect actual &&\n> + test tagname = $(cat actual)\n>\n> or something along the line? First we test with a commit that is\n> not tagged at all to have some commits between the tag and HEAD with\n> the original comparison (this is for #2), then we make sure the\n> length of the hexpart (new---this is for #3), and then we add the\n> tag to see the \"exact\" case also works (this is for #1).\n\nYeah, makes sense. Thanks for such a nice explanation.\n\nI have applied this and it works. I'll reroll with this change and\nalso the change in the log message (and also maybe add some comments\nabout these cases).\n\nI'll make sure to do this in the tests for ref-filter too, about which I\nmentioned above.\n\nThanks\n"},{"id":"479014","messageId":"20230629133841.18784-2-five231003@gmail.com","threadId":"59927","inReplyTo":"20230628181753.10384-1-five231003@gmail.com","subject":"[PATCH v2] t4205: correctly test %(describe:abbrev=...)","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-06-29T13:18:08Z","receivedAt":"2023-06-29T13:41:05Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"The pretty format %(describe:abbrev=) tells describe to use\nat least digits of the oid to generate the human-readable\nformat of the commit-ish.\n\nThere are three things to test here:\n - Check that we can describe a commit that is not tagged (that is,\n for example our HEAD is at least one commit ahead of some reachable\n commit which is tagged) with at least digits of the oid\n being used for describing it.\n\n - Check that when using such a commit-ish, we always use at least\n digits of the oid to describe it.\n\n - Check that we can describe a tag. This just gives the name of the\n tag irrespective of abbrev (abbrev doesn't make sense here).\n\nDo this, instead of the current test which only tests the last case.\n\nHelped-by: Junio C Hamano \nMentored-by: Christian Couder \nMentored-by: Hariom Verma \nSigned-off-by: Kousik Sanagavarapu \n---\n\nChanges since v1:\n- Changed the log message\n- Added things to be tested as commented by Junio\n\nRange-diff vs v1:\n1: 2c10de6c11 ! 1: 76c3e38033 t4205: correctly test\n%(describe:abbrev=...)\n @@ Metadata\n ## Commit message ##\n t4205: correctly test %(describe:abbrev=...)\n \n - The pretty format %(describe:abbrev=) tells describe to\n use only\n - characters of the oid to generate the human-readable\n format of\n - the commit-ish.\n + The pretty format %(describe:abbrev=) tells describe to\nuse\n + at least digits of the oid to generate the\nhuman-readable\n + format of the commit-ish.\n \n - This is not apparent in the test for %(describe:abbrev=...)\n because we\n - directly tag HEAD and use that, in which case the\n human-readable format\n - is just the tag name. So, create a new commit and use that\n instead.\n + There are three things to test here:\n + - Check that we can describe a commit that is not tagged\n(that is,\n + for example our HEAD is at least one commit ahead of some\nreachable\n + commit which is tagged) with at least digits of\nthe oid\n + being used for describing it.\n \n + - Check that when using such a commit-ish, we always use at\nleast\n + digits of the oid to describe it.\n +\n + - Check that we can describe a tag. This just gives the name\nof the\n + tag irrespective of abbrev (abbrev doesn't make sense\nhere).\n +\n + Do this, instead of the current test which only tests the last\ncase.\n +\n + Helped-by: Junio C Hamano \n Mentored-by: Christian Couder \n Mentored-by: Hariom Verma \n Signed-off-by: Kousik Sanagavarapu \n \n ## t/t4205-log-pretty-formats.sh ##\n @@ t/t4205-log-pretty-formats.sh: test_expect_success\n'%(describe:tags) vs git describe --tags' '\n - '\n \n test_expect_success '%(describe:abbrev=...) vs git describe\n--abbrev=...' '\n -- test_when_finished \"git tag -d tagname\" &&\n -- git tag -a -m tagged tagname &&\n + test_when_finished \"git tag -d tagname\" &&\n ++\n ++ # Case 1: We have commits between HEAD and the most recent tag\n ++ # reachable from it\n + test_commit --no-tag file &&\n ++ git describe --abbrev=15 >expect &&\n ++ git log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n ++ test_cmp expect actual &&\n ++\n ++ # Make sure the hash used is at least 15 digits long\n ++ sed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n ++ test 16 -le $(wc -c expect &&\n git log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n - test_cmp expect actual\n +- test_cmp expect actual\n ++ test_cmp expect actual &&\n ++ test tagname = $(cat actual)\n + '\n + \n + test_expect_success 'log --pretty with space stealing' '\n\n t/t4205-log-pretty-formats.sh | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 4cf8a77667..dd9035aa38 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -1012,10 +1012,25 @@ test_expect_success '%(describe:tags) vs git describe --tags' '\n \n test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '\n \ttest_when_finished \"git tag -d tagname\" &&\n+\n+\t# Case 1: We have commits between HEAD and the most recent tag\n+\t#\t reachable from it\n+\ttest_commit --no-tag file &&\n+\tgit describe --abbrev=15 >expect &&\n+\tgit log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Make sure the hash used is at least 15 digits long\n+\tsed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n+\ttest 16 -le $(wc -c expect &&\n \tgit log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\ttest tagname = $(cat actual)\n '\n \n test_expect_success 'log --pretty with space stealing' '\n-- \n2.41.0.29.g8148156d44.dirty\n\n"}]}