Re: [PATCH 2/3] branch: add --unset-upstream option

> cmn@elego.de (Carlos Martín Nieto) writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Carlos Martín Nieto <cmn@elego.de> writes:
>>>
>>>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>>>> index e9019ac..93e5d6e 100755
>>>> --- a/t/t3200-branch.sh
>>>> +++ b/t/t3200-branch.sh
>>>> @@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a particular branch' \
>>>>       test "$(git config branch.my13.remote)" = "." &&
>>>>       test "$(git config branch.my13.merge)" = "refs/heads/master"'
>>>>  
>>>> +test_expect_success 'test --unset-upstream on HEAD' \
>>>> +    'git branch my14
>>>> +     test_config branch.master.remote foo &&
>>>> +     test_config branch.master.merge foo &&
>>>> +     git branch --set-upstream-to my14 &&
>>>> +     git branch --unset-upstream &&
>>>> +     test_must_fail git config branch.master.remote &&
>>>> +     test_must_fail git config branch.master.merge'
>>>> +
>>>> +test_expect_success 'test --unset-upstream on a particular branch' \
>>>> +    'git branch my15
>>>> +     git branch --set-upstream-to master my14 &&
>>>> +     git branch --unset-upstream my14 &&
>>>> +     test_must_fail git config branch.my14.remote &&
>>>> +     test_must_fail git config branch.my14.merge'
>>>> +
>>>
>>> What should happen when you say "--unset-upstream" on a branch B
>>> that does not have B@{upstream}?  Should it fail?  Should it be
>>> silently ignored?  Is it undefined that we do not want to test?
>>
>> I'd say it should be ignored, as the end result we want is for there to
>> be no upstream information. What we do underneath is remove a couple of
>> config options, wich doesn't fail if they didn't insist in the first
>> place.
>
> I am not sure about that reasoning.
>
>     $ git config --unset core.nosuchvariable ; echo $?
>     5
>
> looks like a failure to me.
>
> More importantly, wouldn't we want to catch typo in
>
> 	git branch --unset-upstream mext
>
> which admittedly should come from a different codepath (I do not
> think your patch checks if "mext" branch exists before going ahead
> to the config--unset dance)?