Re: [PATCH v3 1/5] pretty: add conditional %C?colorname placeholders

From: Junio C Hamano <gitster@pobox.com>
Date: May 2, 2010, 03:12 UTC
Message-ID: <7vvdb7yp1u.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1272656128-2002-2-git-send-email-wmpalmer@gmail.com>

Will Palmer <wmpalmer@gmail.com> writes:

Show 10 quoted lines

> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> new file mode 100755
> index 0000000..b7ec943
> --- /dev/null
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -0,0 +1,52 @@
> +#!/bin/sh
> +#
> +# Released into Public Domain by Will Palmer 2010
> +#

Hmm...

Show 12 quoted lines

> +test_description='Test pretty formats'
> +. ./test-lib.sh
> +
> +test_expect_success "set up basic repos" \
> +	">foo &&
> +	>bar &&
> +	git add foo &&
> +	test_tick &&
> +	git commit -m initial &&
> +	git add bar &&
> +	test_tick &&
> +	git commit -m 'add bar'"

Just a style thing, but it is easier to read to write:

        test_expect_success 'set up basic repos' '
                >foo &&
                >bar &&
                ...
        '

Show 9 quoted lines

> +for flag in false true always; do
> +for color in red green blue reset; do
> +
> +	make_expected="git config --get-color no.such.slot $color >expected"
> +	test_expect_success "%C$color with color.ui $flag" \
> +		"$make_expected &&
> +		git config color.ui $flag &&
> +		git log -1 --pretty=format:'%C$color' > actual &&
> +		test_cmp expected actual"

I would really prefer to see a test that checks output with real contents, not just "switch to color". E.g. --format="Title: %C${color}%s%C(normal)" or something like that.

Also you probably would want to make this one round into a shell function e.g.

	make_expected ()
        {
        	git config --get-color no.such.slot "$1"
	}

	run_format_test ()
        {
		flag=$1 color=$2
                test_expect_success "%C$color with ..." '
			make_expected $color >expected
                        git config color.ui $flag &&
                        git log -1 --pretty=format:'%C($color)' >actual &&
                        test_cmp expected actual"
                '
                ...
	}

and then run this loop:

	for flag in flase true always
        do
        	for color in red green blue reset
                do
                	run_format_test $flag $color
		done
	done