From: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
Date: Tue, 15 Aug 2017 10:23:57 GMT
Subject: Re: [PATCH v3 1/2 / RFC] builtin/branch: stop supporting the use of --set-upstream option
Message-ID: <b12f73f0-b4de-e96e-f102-2ec919ca8db5@gmail.com>
In-Reply-To: <CAN0heSr1FjayzX-SnNVcgQuh+Cc-f=AjY8H=pGG6uvf8rrJM=A@mail.gmail.com>

On Tuesday 15 August 2017 12:44 AM, Martin Ågren wrote:
>>   --set-upstream::
>> -       If specified branch does not exist yet or if `--force` has been
>> -       given, acts exactly like `--track`. Otherwise sets up configuration
>> -       like `--track` would when creating the branch, except that where
>> -       branch points to is not changed.
>> +       As this option has confusing syntax it's no longer supported. Please use
> "has" or "had"? (I guess when someone reads this, it "has" no syntax at
> all. ;) )
Got it.
>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>> index dd37ac47c..249be4b1a 100755
>> --- a/t/t3200-branch.sh
>> +++ b/t/t3200-branch.sh
>> @@ -561,7 +561,8 @@ test_expect_success 'use --set-upstream-to modify a particular branch' '
>>          git branch my13 &&
>>          git branch --set-upstream-to master my13 &&
>>          test "$(git config branch.my13.remote)" = "." &&
>> -       test "$(git config branch.my13.merge)" = "refs/heads/master"
>> +       test "$(git config branch.my13.merge)" = "refs/heads/master" &&
>> +       git branch --unset-upstream my13
> I think it would be safer to use test_when_finished like on line 625.
> Out of curiosity: are you adding this out of caution, or did some later
> test fail without this?
One test seems to fail without this. I guess it's better to keep this 
change as a separate
commit.

>> diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
>> index 97a07655a..4b522f456 100755
>> --- a/t/t6040-tracking-info.sh
>> +++ b/t/t6040-tracking-info.sh
>> @@ -188,35 +188,35 @@ test_expect_success 'fail to track annotated tags' '
>>          test_must_fail git checkout heavytrack
>>   '
>>
>> -test_expect_success 'setup tracking with branch --set-upstream on existing branch' '
>> +test_expect_success 'setup tracking with branch --set-upstream-to on existing branch' '
>>          git branch from-master master &&
>>          test_must_fail git config branch.from-master.merge > actual &&
>> -       git branch --set-upstream from-master master &&
>> +       git branch --set-upstream-to master from-master &&
>>          git config branch.from-master.merge > actual &&
>>          grep -q "^refs/heads/master$" actual
>>   '
>>
>> -test_expect_success '--set-upstream does not change branch' '
>> +test_expect_success '--set-upstream-to does not change branch' '
>>          git branch from-master2 master &&
>>          test_must_fail git config branch.from-master2.merge > actual &&
>>          git rev-list from-master2 &&
>>          git update-ref refs/heads/from-master2 from-master2^ &&
>>          git rev-parse from-master2 >expect2 &&
>> -       git branch --set-upstream from-master2 master &&
>> +       git branch --set-upstream-to master from-master2 &&
>>          git config branch.from-master.merge > actual &&
>>          git rev-parse from-master2 >actual2 &&
>>          grep -q "^refs/heads/master$" actual &&
>>          cmp expect2 actual2
>>   '
> The two tests above were added when --set-upstream was originally added.
> Now that you're converting them to use --set-upstream-to, to what extent
> do they just test the same thing as the tests in t3200?
The first seems useless, I'll remove it. Regarding the second one, as 
far as I could see
there's no test in t3200 that does something similar so I guess it could 
be kept back.

---
Kaartic