| Message ID | 20190919214712.7348-7-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | name-rev: eliminate recursion | expand |
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
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 >
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
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(+)