Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only

[Koosha Khajehmoogahi <koosha@posteo.de>]

On 03/22/2015 11:40 PM, Eric Sunshine wrote:
> On Sun, Mar 22, 2015 at 6:07 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
>> On 03/22/2015 08:57 PM, Torsten Bögershausen wrote:
>>> On 22.03.15 19:28, Koosha Khajehmoogahi wrote:
>>>> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
>>>> ---
>>>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>>>> index 5f2b290..ab6f371 100755
>>>> --- a/t/t4202-log.sh
>>>> +++ b/t/t4202-log.sh
>>>> @@ -428,6 +428,147 @@ cat > expect <<\EOF
>>>>  * initial
>>>>  EOF
>>>>
>>>> +test_expect_success 'log with config log.merges=show' '
>>>> +    git config log.merges show &&
>>>> +    git log --pretty=tformat:%s >actual &&
>>>> +    test_cmp both_commits_merges actual &&
>>>> +    git config --unset log.merges
>>> Do we need the unset here?
>>> The log.merges is nicely set up before each test case, so can we drop the unset lines ?
>>> (Or do I miss something ?)
>>
>> Good point; we can drop only those unset lines whose next test sets the log.merges.
>> However, if the next test does not set it, we must unset it as it affects the
>> default behavior of git-log.
> 
> Such an approach would be too fragile. Tests may be re-ordered, added,
> or removed. Better is to make each test as self-contained as possible.
> See my review[1] of this patch for alternate suggestions.
> 
> [1]: http://article.gmane.org/gmane.comp.version-control.git/266099
> 

That's why I wrote them this way actually. Also, thanks for your
review. I will refactor my code to consider your suggestions.

Thanks.
-- 
Koosha