Re: [PATCH v9 2/2] pull --rebase: add --[no-]autostash flag

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 20, 2016, 02:01 UTC
Message-ID: <CAPig+cQOHAWLnDi0LmfJH2vb7mv9CABcx=dyU8DezDunZbfpVQ@mail.gmail.com>
In-Reply-To: <CA+DCAeRe4ak1Ni7CADpJdd-=+dvKTTepHY6dYNfQV3ADpz5f6Q@mail.gmail.com>

On Fri, Mar 18, 2016 at 11:17 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:

Show 24 quoted lines

> On Fri, Mar 18, 2016 at 9:54 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> +test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '
>>
>> Why do titles of some of the new test titles have a ":" after "rebase"
>> while other don't?
>>
>> Also, how about normalizing the titles so that the reader knows in
>> which tests rebase.autostash is 'true' and in which it is 'false'?
>> Presently, it's difficult to decipher what's being tested based only
>> on the titles.
>
> If it's so then how about the tests titles to be the following:
>
> * pull --rebase: --autostash works with rebase.autoStash set true
>
> * pull --rebase: --autostash works with rebase.autoStash set false
>
> * pull --rebase: --no-autostash works with rebase.autoStash set true
>
> * pull --rebase: --no-autostash works with rebase.autoStash set false
>
> Earlier I tried to keep it as less verbose as possible (and probably
> made it hard to decipher). Does the above titles seems short and
> informative to you? If so then I will use them instead of earlier ones.

Those are better. If I was doing it, I'd probably drop the unnecessary ":", "works with", and "set", so:

    pull --rebase --autostash & rebase.autoStash=true
    pull --rebase --autostash & rebase.autoStash=false
    pull --rebase --no-autostash & rebase.autoStash=true
    pull --rebase --no-autostash & rebase.autoStash=false

or something, but that's a very minor point.

Show 9 quoted lines

>> Finally, shouldn't you also be testing --autostash and --no-autostash
>> when rebase.autostash is not set?
>
> If rebase.autoStash is not set then config.autostash will remain zero
> through out the process. What I want to point out is that rebase.autoStash
> , if not set, is equivalent to being set false. So adding tests regarding
> "--[no-]autostash with rebase.autoStash unset" seems equivalent to tests
> " pull --rebase: --autostash works with rebase.autoStash set false" and
> "pull --rebase: --no-autostash works with rebase.autoStash set false".

Yes, but what you've described is how the current *implementation* works, whereas the tests should be checking expected *behavior*. So, while we both know that under the current implementation, checking:

    pull --rebase --autostash & rebase.autoStash unset

is the same as checking:

    pull --rebase--autostash & rebase.autoStash=false

some future change to the implementation could (accidentally) break this equivalence, and we want to protect against such breakage by checking behavior, not implementation.

Also, I forgot to mention a couple other missing tests you should add:

    * pull --autostash (without --rebase) should error out
    * pull --no-autostash (without --rebase) should error out