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

From: Derrick Stolee <stolee@gmail.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 06/15] t6120: add a test to cover inner conditions in 'git name-rev's name_rev()
Date: Fri, 20 Sep 2019 11:14:56 -0400	[thread overview]
Message-ID: <2e18a216-f6f8-2307-2524-a2e1f44b26c7@gmail.com> (raw)
In-Reply-To: <20190919214712.7348-7-szeder.dev@gmail.com>

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