From: Junio C Hamano <gitster@pobox.com>
Date: Sat, 06 Aug 2016 17:02:53 GMT
Subject: Re: [PATCHv2 2/6] t7408: merge short tests, factor out testing method
Message-ID: <xmqqshuhj0ea.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160806012318.17968-3-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> +test_alternate_usage() {

According to Documentation/CodingGuidelines, this should be:

    test_alternate_usage () {

Somehow the helper name sounds as if it is testing if an alternate
is used correctly (i.e. the machinery may attempt to use alternate
but not in a correct way), not testing if an alternate is correctly
used (i.e. the machinery incorrectly forgets to use an alternate at
all), but maybe it is just me.

> +test_expect_success 'updating superproject keeps alternates' '
> +	test_when_finished "rm -rf super-clone" &&

This one is new; we do not remove A, B or super.  According to the
previous round of review, this is a deliberate design, that needs to
be spelled out by having a comment block before this test so that
other people who add more tests can understand why they need to
clean when they follow suit.  Perhaps something like:

    ###############################################################
    # The tests up to this point, and repositories created by them
    # (A, B, super and super/sub), are about setting up the stage
    # forsubsequent tests and meant to be kept throughout the
    # remainder of the test.
    # Tests from here on, if they create their own test repository,
    # are expected to clean after themselves.

    test_expect_success 'updating superproject keeps alternates' '
            test_when_finished "rm -rf super-clone" &&