From: Philip Oakley <philipoakley@iee.org>
Date: Mon, 20 Nov 2017 17:26:51 GMT
Subject: Re: [PATCH v3 5/5] Testing: provide tests requiring them with ellipses after SHA-1 values
Message-ID: <5AE7AD53CF184A27BF8F484D415083D9@PhilipOakley>
In-Reply-To: <xmqqzi7hlhkx.fsf@gitster.mtv.corp.google.com>

From: "Junio C Hamano" <gitster@pobox.com>
> Ann T Ropea <bedhanger@gmx.de> writes:
>
>> *1* We are being overly generous in t4013-diff-various.sh because we do
>> not want to destroy/take apart the here-document.  Given that all this a
>> temporary measure, we should get away with it.

So, the need to reformat the test for the future post-deprecation period is 
being deferred to the time that the PRINT_SHA1_ELLIPSIS env variable, and 
all ellipis, is removed - is that the case? Maybe it just needs saying 
plainly.

Or is the env variable being retained as a fallback 'forever'? I'm half 
guessing that it may tend toward the latter as it's an easier backward 
compatibility decision.

[apologioes this is mid thread, I'm catching up on 2 weeks of emails]

>
> I do not think the patch is being particularly generous.  If
> anything, it is being unnecessarily sloppy by not adding new checks
> to verify the updated behaviour.
>
> The above comment mentions "destroy/take apart" the here-document,
> but I do see no need to destroy anything.  All you need to do is to
> enhance and extend.  For example, you could do it like so (this is
> written in my e-mail client, and not an output of diff, so the
> indentation etc. may be all off, but should be sufficient to
> illustrate the idea):
>
>    while read cmd
>    do
>            case "$cmd" in
>            '' | '#'*) continue ;;
>            esac
>            test=$(echo "$cmd" | sed -e 's|[/ ][/ ]*|_|g')
>            pfx=$(printf "%04d" $test_count)
>            expect="$TEST_DIRECTORY/t4013/diff.$test"
>            actual="$pfx-diff.$test"
>   +        case "$cmd" in
>   +        X*) cmd=${cmd#X}; no_ellipses=" (no ellipses)" ;;
>   +        *) no_ellipses= ;;
>   +        esac
>   -        test_expect_success "git $cmd" '
>   +        test_expect_success "git $cmd$no_ellipses" '
>                {
>                        echo "\$ git $cmd"
>   -                    git $cmd |
>   +                    if test -n "$no_ellipses"
>   +                    then
>   +                            git $cmd
>   +                    else
>   +                            PRINT_SHA1_ELLIPSES=yes git $cmd
>   +                    fi |
>                        sed -e ....
>        ...
>    done <<\EOF
>    diff-tree initial
>    diff-tree -r initial
>    diff-tree -r --abbrev initial
>    diff-tree -r --abbrev=4 initial
>   +Xdiff-tree -r --abbrev=4 initial
>    ...
>    EOF
>
> There is a new and duplicated line with a prefix X for one existing
> test in the above.  The idea is that the ones marked as such will
> test and verify the effect of this new behaviour by not setting the
> environment variable.  The expected and actual test output for the
> new test will have X prefixed to it.  t4013 is arranged in such a
> way that it is easy to add a new test like this---you only need to
> add an expected output in a new file in t/t4013/. directory.  And
> the output with these ellipses removed will be something we would
> expect see in the new world (without the escape hatch environment
> variable), we would need to add a new file there to record what the
> expected output from the command is.
>
> I singled out the diff-tree invocation with --abbrev=4 as an example
> in the above, but in a more thorough final version, we'd need to
> cover both "abbreviation with ellipses" and "abbreviation without
> ellipses" output for other lines in the test case listed in the
> here-document.