On Fri, Mar 18, 2016 at 11:17 AM, Mehul Jain <mehul.jain2029@xxxxxxxxx> wrote:
> On Fri, Mar 18, 2016 at 9:54 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html