[PATCH] t4205: correctly test %(describe:abbrev=...)

[PATCH] t4205: correctly test %(describe:abbrev=...)

[Kousik Sanagavarapu]

The pretty format %(describe:abbrev=<number>) tells describe to use only
<number> characters of the oid to generate the human-readable format of
the commit-ish.

This is not apparent in the test for %(describe:abbrev=...) because we
directly tag HEAD and use that, in which case the human-readable format
is just the tag name. So, create a new commit and use that instead.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
 t/t4205-log-pretty-formats.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 4cf8a77667..b631b5a142 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -1011,8 +1011,7 @@ test_expect_success '%(describe:tags) vs git describe --tags' '
 '
 
 test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '
-	test_when_finished "git tag -d tagname" &&
-	git tag -a -m tagged tagname &&
+	test_commit --no-tag file &&
 	git describe --abbrev=15 >expect &&
 	git log -1 --format="%(describe:abbrev=15)" >actual &&
 	test_cmp expect actual
-- 
2.41.0.29.g8148156d44.dirty

Re: [PATCH] t4205: correctly test %(describe:abbrev=...)

[Junio C Hamano]

Kousik Sanagavarapu <five231003@gmail.com> writes:

> The pretty format %(describe:abbrev=<number>) tells describe to use only
> <number> characters of the oid to generate the human-readable format of
> the commit-ish.

Is that *only* correct?  I thought it was "at least <number> hexdigits"
to allow for future growth of the project.

> This is not apparent in the test for %(describe:abbrev=...) because we
> directly tag HEAD and use that, in which case the human-readable format
> is just the tag name. So, create a new commit and use that instead.

Nice.  How was this found, I have to wonder, and more importantly,
how would we have written this test in the first place to avoid
testing "the wrong thing", to learn from this experience?

>  test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '
> -	test_when_finished "git tag -d tagname" &&
> -	git tag -a -m tagged tagname &&
> +	test_commit --no-tag file &&
>  	git describe --abbrev=15 >expect &&
>  	git log -1 --format="%(describe:abbrev=15)" >actual &&
>  	test_cmp expect actual

The current test checks that the output in the case where the number
of commits since the tag is 0, and "describe --abbrev=15" and "log
--format='%(describe:abbrev=15)'" give exactly the same result.
Which is a good thing to test.

But we *also* want to test a more typical case where there are
commits between HEAD and the tag that is used to describe it.  

And we *also* want to make sure that the hexadecimal object name
suffix used in the description is at least 15 hexdigits long, if not
more.

The updated test drops test #1 (which is questionable), adds test #2
(which is good), and still omits test #3 (which is not so good).  

So, perhaps

    test_when_finished "git tag -d tagname" &&
-   git tag -a -m tagged tagname &&
    test_commit --no-tag file &&
    git describe --abbrev=15 >expect &&
    git log -1 --format="%(describe:abbrev=15)" >actual &&
    test_cmp expect actual &&
+   sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
+   test 16 -le $(wc -c <hexpart) &&
+
+   git tag -a -m tagged tagname &&
+   git describe --abbrev=15 >expect &&
+   git log -1 --format="%(describe:abbrev=15)" >actual &&
+   test_cmp expect actual &&
+   test tagname = $(cat actual)

or something along the line?  First we test with a commit that is
not tagged at all to have some commits between the tag and HEAD with
the original comparison (this is for #2), then we make sure the
length of the hexpart (new---this is for #3), and then we add the
tag to see the "exact" case also works (this is for #1).

Thanks.

Re: [PATCH] t4205: correctly test %(describe:abbrev=...)

[Kousik Sanagavarapu]

On Wed, Jun 28, 2023 at 02:30:18PM -0700, Junio C Hamano wrote:
> Kousik Sanagavarapu <five231003@gmail.com> writes:
>
> > The pretty format %(describe:abbrev=<number>) tells describe to use only
> > <number> characters of the oid to generate the human-readable format of
> > the commit-ish.
>
> Is that *only* correct?  I thought it was "at least <number> hexdigits"
> to allow for future growth of the project.

Yeah, this is wrong. It should be "at least" for being more precise as
we may need more than <number> in some cases. Will change that. Thanks
for catching it.

> > This is not apparent in the test for %(describe:abbrev=...) because we
> > directly tag HEAD and use that, in which case the human-readable format
> > is just the tag name. So, create a new commit and use that instead.
>
> Nice.  How was this found, I have to wonder, and more importantly,

I was working on duplicating "%(describe)" from pretty, in ref-filter
and was writing tests for it. While going through the trash directory
for some other breakage, I found this. So it was kind of a chance.

> how would we have written this test in the first place to avoid
> testing "the wrong thing", to learn from this experience?

I don't have a clue :).

In this particular test, this is not "the wrong thing" anyways, as you
explain below. We just fail to test it wholly (we missed some cases).

> >  test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '
> > -   test_when_finished "git tag -d tagname" &&
> > -   git tag -a -m tagged tagname &&
> > +   test_commit --no-tag file &&
> >     git describe --abbrev=15 >expect &&
> >     git log -1 --format="%(describe:abbrev=15)" >actual &&
> >     test_cmp expect actual
>
> The current test checks that the output in the case where the number
> of commits since the tag is 0, and "describe --abbrev=15" and "log
> --format='%(describe:abbrev=15)'" give exactly the same result.
> Which is a good thing to test.
>
> But we *also* want to test a more typical case where there are
> commits between HEAD and the tag that is used to describe it.
>
> And we *also* want to make sure that the hexadecimal object name
> suffix used in the description is at least 15 hexdigits long, if not
> more.
>
> The updated test drops test #1 (which is questionable), adds test #2
> (which is good), and still omits test #3 (which is not so good).
>
> So, perhaps
>
>     test_when_finished "git tag -d tagname" &&
> -   git tag -a -m tagged tagname &&
>     test_commit --no-tag file &&
>     git describe --abbrev=15 >expect &&
>     git log -1 --format="%(describe:abbrev=15)" >actual &&
>     test_cmp expect actual &&
> +   sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
> +   test 16 -le $(wc -c <hexpart) &&
> +
> +   git tag -a -m tagged tagname &&
> +   git describe --abbrev=15 >expect &&
> +   git log -1 --format="%(describe:abbrev=15)" >actual &&
> +   test_cmp expect actual &&
> +   test tagname = $(cat actual)
>
> or something along the line?  First we test with a commit that is
> not tagged at all to have some commits between the tag and HEAD with
> the original comparison (this is for #2), then we make sure the
> length of the hexpart (new---this is for #3), and then we add the
> tag to see the "exact" case also works (this is for #1).

Yeah, makes sense. Thanks for such a nice explanation.

I have applied this and it works. I'll reroll with this change and
also the change in the log message (and also maybe add some comments
about these cases).

I'll make sure to do this in the tests for ref-filter too, about which I
mentioned above.

Thanks

[PATCH v2] t4205: correctly test %(describe:abbrev=...)

[Kousik Sanagavarapu]

The pretty format %(describe:abbrev=<number>) tells describe to use
at least <number> digits of the oid to generate the human-readable
format of the commit-ish.

There are three things to test here:
  - Check that we can describe a commit that is not tagged (that is,
    for example our HEAD is at least one commit ahead of some reachable
    commit which is tagged) with at least <number> digits of the oid
    being used for describing it.

  - Check that when using such a commit-ish, we always use at least
    <number> digits of the oid to describe it.

  - Check that we can describe a tag. This just gives the name of the
    tag irrespective of abbrev (abbrev doesn't make sense here).

Do this, instead of the current test which only tests the last case.

Helped-by: Junio C Hamano <gitster@pobox.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---

Changes since v1:
- Changed the log message
- Added things to be tested as commented by Junio

Range-diff vs v1:
1:  2c10de6c11 ! 1:  76c3e38033 t4205: correctly test
%(describe:abbrev=...)
    @@ Metadata
      ## Commit message ##
         t4205: correctly test %(describe:abbrev=...)
     
    -    The pretty format %(describe:abbrev=<number>) tells describe to
         use only
    -    <number> characters of the oid to generate the human-readable
         format of
    -    the commit-ish.
    +    The pretty format %(describe:abbrev=<number>) tells describe to
use
    +    at least <number> digits of the oid to generate the
human-readable
    +    format of the commit-ish.
     
    -    This is not apparent in the test for %(describe:abbrev=...)
         because we
    -    directly tag HEAD and use that, in which case the
         human-readable format
    -    is just the tag name. So, create a new commit and use that
         instead.
    +    There are three things to test here:
    +      - Check that we can describe a commit that is not tagged
(that is,
    +        for example our HEAD is at least one commit ahead of some
reachable
    +        commit which is tagged) with at least <number> digits of
the oid
    +        being used for describing it.
     
    +      - Check that when using such a commit-ish, we always use at
least
    +        <number> digits of the oid to describe it.
    +
    +      - Check that we can describe a tag. This just gives the name
of the
    +        tag irrespective of abbrev (abbrev doesn't make sense
here).
    +
    +    Do this, instead of the current test which only tests the last
case.
    +
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
         Mentored-by: Christian Couder <christian.couder@gmail.com>
         Mentored-by: Hariom Verma <hariom18599@gmail.com>
         Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
     
      ## t/t4205-log-pretty-formats.sh ##
     @@ t/t4205-log-pretty-formats.sh: test_expect_success
'%(describe:tags) vs git describe --tags' '
    - '
      
      test_expect_success '%(describe:abbrev=...) vs git describe
--abbrev=...' '
    --  test_when_finished "git tag -d tagname" &&
    --  git tag -a -m tagged tagname &&
    +   test_when_finished "git tag -d tagname" &&
    ++
    ++  # Case 1: We have commits between HEAD and the most recent tag
    ++  #         reachable from it
     +  test_commit --no-tag file &&
    ++  git describe --abbrev=15 >expect &&
    ++  git log -1 --format="%(describe:abbrev=15)" >actual &&
    ++  test_cmp expect actual &&
    ++
    ++  # Make sure the hash used is at least 15 digits long
    ++  sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
    ++  test 16 -le $(wc -c <hexpart) &&
    ++
    ++  # Case 2: We have a tag at HEAD, describe directly gives the
    ++  #         name of the tag
    +   git tag -a -m tagged tagname &&
        git describe --abbrev=15 >expect &&
        git log -1 --format="%(describe:abbrev=15)" >actual &&
    -   test_cmp expect actual
    +-  test_cmp expect actual
    ++  test_cmp expect actual &&
    ++  test tagname = $(cat actual)
    + '
    + 
    + test_expect_success 'log --pretty with space stealing' '

 t/t4205-log-pretty-formats.sh | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 4cf8a77667..dd9035aa38 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -1012,10 +1012,25 @@ test_expect_success '%(describe:tags) vs git describe --tags' '
 
 test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '
 	test_when_finished "git tag -d tagname" &&
+
+	# Case 1: We have commits between HEAD and the most recent tag
+	#	  reachable from it
+	test_commit --no-tag file &&
+	git describe --abbrev=15 >expect &&
+	git log -1 --format="%(describe:abbrev=15)" >actual &&
+	test_cmp expect actual &&
+
+	# Make sure the hash used is at least 15 digits long
+	sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart &&
+	test 16 -le $(wc -c <hexpart) &&
+
+	# Case 2: We have a tag at HEAD, describe directly gives the
+	#	  name of the tag
 	git tag -a -m tagged tagname &&
 	git describe --abbrev=15 >expect &&
 	git log -1 --format="%(describe:abbrev=15)" >actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test tagname = $(cat actual)
 '
 
 test_expect_success 'log --pretty with space stealing' '
-- 
2.41.0.29.g8148156d44.dirty