Re: [PATCH 03/24] t7600 (merge): do not launch gitk for --debug

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 18, 2010, 08:55 UTC
Message-ID: <AANLkTikBmYH1xC_7iiXQK8-vo0yavwX_ELR=z1kfVSLj@mail.gmail.com>
In-Reply-To: <20100818022931.GA19868@burratino>

On Wed, Aug 18, 2010 at 02:29, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 41 quoted lines

> Sverre Rabbelier wrote:
>> On Tue, Aug 17, 2010 at 01:54, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> -test_debug 'gitk --all'
>>> +test_debug 'git log --graph --decorate --oneline --all'
>>
>>> -test_debug 'gitk --all'
>>> +test_debug 'git log --graph --decorate --oneline --all'
> [...]
>> Really? Why can't you stuff the command to execute in a variable? On
>> that note, why wasn't that done in the first place?! This kind of
>> repetition is not very DRY.
>
> My thought was that
>
>  test_debug '$debug_command'
>  ...
>  test_debug '$debug_command'
>  ...
>  [etc]
>
> is not very DRY either, so I am hoping some configurable version of
>
> -- 8< --
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index e8f21d5..65b0e35 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -373,6 +373,7 @@ test_run_ () {
>        if test "$verbose" = "t" && test -n "$HARNESS_ACTIVE"; then
>                echo ""
>        fi
> +       test_debug "git log --graph --decorate --oneline --all"
>        return 0
>  }
>
> -- >8 --
>
> appears to replace it.  In the short term, the "s/gitk --all/git log
> --graph --decorate --oneline --all/" was just meant as a quick fix
> so as not to drop the feature.  Ideas?

The main issue here is that --debug means two things. It runs test_debug() code AND instructs the test-lib not to remove the trash directory when it's done.

If we had a separate --keep-trash option most of this problem would go away, no?

Aside from that it's better to use git log rather than gitk in test_debug, since it's in text form it can be easily sent along with a failing test, which is not the case for dozens of gitk invocations.