Re: [PATCH v2/GSoC/MICRO] revision: forbid combining --graph and --no-walk

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 8, 2015, 19:40 UTC
Message-ID: <xmqqwq2rqnvp.fsf@gitster.dls.corp.google.com>
In-Reply-To: <3de5837561c07fbf8d6187578fc37b3dbf2ea5f7.1425702676.git.dongcan.jiang@gmail.com>

Dongcan Jiang <dongcan.jiang@gmail.com> writes:

> Because --graph is about connected history while
> --no-walk is about discrete points. [1]

The convention around here is that the title of the patch on the Subject line is *not* the beginning part of the first sentence, you would need to phrase the above more like:

        Because "--graph" is about ... discrete points, it does not
        make sense to allow giving these two options at the same
        time.

to make it a complete sentence.

> It's a pity that git-show has to allow such combination
> in order to make t4052-stat-output.sh compatible. [2]

If you feel "It's a pity", it actually is OK to make a good argument for "fixing" the test without adding the workaround. A replacement commit message for the above two lines for such an approach might look like:

        This change makes a few calls to "show --graph" fail in
        t4052, but asking to show one commit _with_ graph is a
        nonsensical thing to do.  Correct these offending tests that
        expected "--graph" to be useful by removing "--graph" in
        their invocations (and adjusting the expected output, of
        course).

That way, people who do find it actually useful that "show --graph HEAD" that shows something like this

        $ git show --graph -s 52d5bf778
        *   commit 52d5bf77875275bbfc1bf1d7b690f355d5c869e4
        |\  Merge: 36ab768 189c860
        | | Author: Junio C Hamano <gitster@pobox.com>
        | | Date:   Fri Mar 6 15:02:33 2015 -0800
        | | 
        | |     Merge branch 'bw/kwset-use-unsigned'
        ...

can react and argue why it is useful to them.

It is important to notice that your "It's a pity" will no longer apply, if we are keeping the feature because it is useful. It would be clear that we would be keeping the feature not because we are too lazy to correct tests, but because it is actually useful.

> Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
>
> Thanks-to: Eric Sunshine, René Scharfe, Junio C Hamano

These look unusual for a few reasons: S-o-b is not at the end, we usually say Helped-by: instead, and we do not use Thanks-to: with multiple names on a single line.

Please do not try to be original without a good reason. We may start counting the number of times people appear on these footers to see how much contribution those who do not directly author commits (read: those who mentor others) are making.

Show 13 quoted lines

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 5f2b290..fed162e 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -887,4 +887,10 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '
>  	grep "^| | gpg: Good signature" actual
>  '
>
> +test_expect_success 'log --graph --no-walk is forbidden' '
> +	echo "fatal: cannot combine --no-walk with --graph" >expect-error &&
> +	test_must_fail git log --graph --no-walk 2>error &&
> +	test_cmp expect-error error
> +'

I do not think we want to check exact phrasing of the error message here. Just the second line (without the " 2>error &&" at the end) should be sufficient.

Show 5 quoted lines

> +test_expect_success 'rev-list --graph --no-walk is forbidden' '
> +	echo "fatal: cannot combine --no-walk with --graph" >expect-error &&
> +	test_must_fail git rev-list --graph --no-walk 2>error &&
> +	test_cmp expect-error error
> +'

The same comment (about preferring not to check the error message) applies here, and more importantly, this is not a good test because "git rev-list --graph" without the forbidden "--no-walk" would fail for other reasons. Perhaps

	test_must_fail git rev-list --graph --no-walk HEAD

or something, assuming that there is already a commit pointed by HEAD at this point in the test (I didn't check).

Thanks.