Re: [PATCH 1/2] t6200-fmt-merge-msg: Exercise 'merge.log' to configure shortlog length

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Aug 28, 2010, 10:35 UTC
Message-ID: <20100828103536.GA12205@kytes>
In-Reply-To: <20100828020018.GD2004@burratino>

Hi Jonathan,

Jonathan Nieder writes:

> I suspect this test would be easier to understand if split up (yes, I
> know I'm guilty of the same in other tests from this file).

I sort of tried to follow what was done in the rest of the file. If this style is desired instead, I'll post an alternative series instead.

Show 9 quoted lines

> 	test_expect_success 'setup: clear [merge] configuration' '
> 		test_might_fail git config --unset-all merge.log &&
> 		test_might_fail git config --unset-all merge.summary
> 	'
> 
> 	test_expect_success 'set up FETCH_HEAD' '
> 		git checkout master &&
> 		git fetch . left
> 	'

Okay.

> 	test_expect_success 'merge.log=3 limits shortlog length' '
> 		test_might_fail git config --unset merge.log &&
> 		test_might_fail git config --unset merge.summary &&

Redundant, as you pointed out in another email.

Show 9 quoted lines

> 		cat >expected <<-\EOF &&
> 		  Left #3
> 		  ...
> 		EOF
> 		git -c merge.log=3 fmt-merge-msg <.git/FETCH_HEAD >msg &&
> 
> 		tail -n 2 msg >actual &&
> 		test_cmp expected actual
> 	'

Ok. I don't like the `tail` thing. Why are you doing it instead of comparing full outputs like the tests in the rest of the file?

> 	test_expect_success 'shortlog length defaults to 20' '
> 		[...]
> 	'

Actually, we can't test this unless we actually create a branch with 20 commits and merge it. I've dropped this- you might like to add it during the cleanup.

> 	test_expect_success 'merge.log=5 does not limit shortlog length' '
> 		[...]
> 	'

Okay.

> 	test_expect_success 'merge.log=6 does not limit shortlog length' '
> 		[...]
> 	'

I thought this was a bit of an overkill, so I removed it.

> 	test_expect_success 'merge.log=0 disables shortlog' '
> 		[...]
> 	'

Okay.

> 	test_expect_success 'merge.log=-1 does something sane' '
> 		[...]
> 	'

Thanks! I found a bug and fixed it (patch will come in a moment).

> I can clean up the earlier tests afterwards. :)

Okay, sounds good.

-- Ram