[06/15] t6120: add a test to cover inner conditions in 'git name-rev's name_rev()
diff mbox series

Message ID 20190919214712.7348-7-szeder.dev@gmail.com
State New
Headers show
Series
  • name-rev: eliminate recursion
Related show

Commit Message

SZEDER Gábor Sept. 19, 2019, 9:47 p.m. UTC
In 'builtin/name-rev.c' in the name_rev() function there is a loop
iterating over all parents of the given commit, and the loop body
looks like this:

  if (parent_number > 1) {
    if (generation > 0)
      // do stuff #1
    else
      // do stuff #2
  } else {
     // do stuff #3
  }

These conditions are not covered properly in the test suite.  As far
as purely test coverage goes, they are all executed several times over
in 't6120-describe.sh'.  However, they don't directly influence the
command's output, because the repository used in that test script
contains several branches and tags pointing somewhere into the middle
of the commit DAG, and thus result in a better name for the
to-be-named commit.  In an early version of this patch series I
managed to mess up those conditions (every single one of them at
once!), but the whole test suite still passed successfully.

So add a new test case that operates on the following history:

    -----------master
   /          /
  A----------M2
   \        /
    \---M1-C
     \ /
      B

and names the commit 'B', where:

  - The merge commit at master makes sure that the 'do stuff #3'
    affects the final name.

  - The merge commit M2 make sure that the 'do stuff #1' part
    affects the final name.

  - And M1 makes sure that the 'do stuff #2' part affects the final
    name.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t6120-describe.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

Comments

Derrick Stolee Sept. 20, 2019, 3:14 p.m. UTC | #1
On 9/19/2019 5:47 PM, SZEDER Gábor wrote:
> In 'builtin/name-rev.c' in the name_rev() function there is a loop
> iterating over all parents of the given commit, and the loop body
> looks like this:
> 
>   if (parent_number > 1) {
>     if (generation > 0)
>       // do stuff #1
>     else
>       // do stuff #2
>   } else {
>      // do stuff #3
>   }
> 
> These conditions are not covered properly in the test suite.  As far
> as purely test coverage goes, they are all executed several times over
> in 't6120-describe.sh'.  However, they don't directly influence the
> command's output, because the repository used in that test script
> contains several branches and tags pointing somewhere into the middle
> of the commit DAG, and thus result in a better name for the
> to-be-named commit.  In an early version of this patch series I
> managed to mess up those conditions (every single one of them at
> once!), but the whole test suite still passed successfully.
> 
> So add a new test case that operates on the following history:
> 
>     -----------master
>    /          /
>   A----------M2
>    \        /
>     \---M1-C
>      \ /
>       B
> 
> and names the commit 'B', where:
> 
>   - The merge commit at master makes sure that the 'do stuff #3'
>     affects the final name.
> 
>   - The merge commit M2 make sure that the 'do stuff #1' part
>     affects the final name.
> 
>   - And M1 makes sure that the 'do stuff #2' part affects the final
>     name.
> 
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  t/t6120-describe.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> index 07e6793e84..2a0f2204c4 100755
> --- a/t/t6120-describe.sh
> +++ b/t/t6120-describe.sh
> @@ -421,4 +421,47 @@ test_expect_success 'describe complains about missing object' '
>  	test_must_fail git describe $ZERO_OID
>  '
>  
> +#   -----------master
> +#  /          /
> +# A----------M2
> +#  \        /
> +#   \---M1-C
> +#    \ /
> +#     B
> +test_expect_success 'test' '
> +	git init repo &&
> +	(
> +		cd repo &&
> +
> +		echo A >file &&
> +		git add file &&
> +		git commit -m A &&
> +		A=$(git rev-parse HEAD) &&

Is it not enough to do something like test_commit here?

> +
> +		git checkout --detach &&
> +		echo B >file &&
> +		git commit -m B file &&
> +		B=$(git rev-parse HEAD) &&
> +
> +		git checkout $A &&
> +		git merge --no-ff $B &&  # M1
> +
> +		echo C >file &&
> +		git commit -m C file &&
> +
> +		git checkout $A &&
> +		git merge --no-ff HEAD@{1} && # M2
> +
> +		git checkout master &&
> +		git merge --no-ff HEAD@{1} &&
> +
> +		git log --graph --oneline &&
> +
> +		echo "$B master^2^2~1^2" >expect &&
> +		git name-rev $B >actual &&

This matches your description.

Thanks,
-Stolee
SZEDER Gábor Sept. 20, 2019, 3:44 p.m. UTC | #2
On Fri, Sep 20, 2019 at 11:14:56AM -0400, Derrick Stolee wrote:
> On 9/19/2019 5:47 PM, SZEDER Gábor wrote:
> > These conditions are not covered properly in the test suite.  As far
> > as purely test coverage goes, they are all executed several times over
> > in 't6120-describe.sh'.  However, they don't directly influence the
> > command's output, because the repository used in that test script
> > contains several branches and tags pointing somewhere into the middle
> > of the commit DAG, and thus result in a better name for the
> > to-be-named commit.

> > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> > index 07e6793e84..2a0f2204c4 100755
> > --- a/t/t6120-describe.sh
> > +++ b/t/t6120-describe.sh
> > @@ -421,4 +421,47 @@ test_expect_success 'describe complains about missing object' '
> >  	test_must_fail git describe $ZERO_OID
> >  '
> >  
> > +#   -----------master
> > +#  /          /
> > +# A----------M2
> > +#  \        /
> > +#   \---M1-C
> > +#    \ /
> > +#     B
> > +test_expect_success 'test' '
> > +	git init repo &&
> > +	(
> > +		cd repo &&
> > +
> > +		echo A >file &&
> > +		git add file &&
> > +		git commit -m A &&
> > +		A=$(git rev-parse HEAD) &&
> 
> Is it not enough to do something like test_commit here?

No, because 'test_commit' adds branches and tags pointing to commits
somewhere in the middle of the history, and those will serve as better
starting point for the resulting name.

> > +
> > +		git checkout --detach &&
> > +		echo B >file &&
> > +		git commit -m B file &&
> > +		B=$(git rev-parse HEAD) &&
> > +
> > +		git checkout $A &&
> > +		git merge --no-ff $B &&  # M1
> > +
> > +		echo C >file &&
> > +		git commit -m C file &&
> > +
> > +		git checkout $A &&
> > +		git merge --no-ff HEAD@{1} && # M2
> > +
> > +		git checkout master &&
> > +		git merge --no-ff HEAD@{1} &&
> > +
> > +		git log --graph --oneline &&
> > +
> > +		echo "$B master^2^2~1^2" >expect &&
> > +		git name-rev $B >actual &&
> 
> This matches your description.
> 
> Thanks,
> -Stolee
>

Patch
diff mbox series

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 07e6793e84..2a0f2204c4 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -421,4 +421,47 @@  test_expect_success 'describe complains about missing object' '
 	test_must_fail git describe $ZERO_OID
 '
 
+#   -----------master
+#  /          /
+# A----------M2
+#  \        /
+#   \---M1-C
+#    \ /
+#     B
+test_expect_success 'test' '
+	git init repo &&
+	(
+		cd repo &&
+
+		echo A >file &&
+		git add file &&
+		git commit -m A &&
+		A=$(git rev-parse HEAD) &&
+
+		git checkout --detach &&
+		echo B >file &&
+		git commit -m B file &&
+		B=$(git rev-parse HEAD) &&
+
+		git checkout $A &&
+		git merge --no-ff $B &&  # M1
+
+		echo C >file &&
+		git commit -m C file &&
+
+		git checkout $A &&
+		git merge --no-ff HEAD@{1} && # M2
+
+		git checkout master &&
+		git merge --no-ff HEAD@{1} &&
+
+		git log --graph --oneline &&
+
+		echo "$B master^2^2~1^2" >expect &&
+		git name-rev $B >actual &&
+
+		test_cmp expect actual
+	)
+'
+
 test_done