Re: [PATCH] describe: add tests for unusual graphs

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 9, 2016, 23:12 UTC
Message-ID: <xmqq60msr92w.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161209131141.3096-1-quinn@quinngrier.com>

Quinn Grier <quinn@quinngrier.com> writes:

> git describe may give incorrect results if there are backdated commits
> or multiple roots. This commit adds two test_expect_failure tests that
> demonstrate these problems.

I am not sure if this is a good patch to take. test_expect_failure is to demonstrate an incorrect behaviour that we wish to correct later, but I do not think these demonstrate incorrect behaviours to begin with.

For example, the latter one seems to expect that by asking to describe D in this picture

Show 5 quoted lines

> +#
> +# A---B*--D master
> +#        /
> +#       C* other
> +#

you seem to expect the description is based on B.

It is not at all clear why it is incorrect if the description were made based on C. If D were described relative to A, ignoring B, then I understand why that result is incorrect and I would agree that describing D in terms of B is more correct. But I do not think that is what the test is trying to demonstrate.

But it is hard to guess only from looking at the test and the proposed log message, because it does not say what makes you think the behaviour you saw was incorrect.