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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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)?
>
> Sorry, the last paragraph was incomplete.
>
> In general, we should detect errors and allow the user to choose to
> ignore.
>
> A script that wants to make sure that B does not have an upstream,
> without knowing if it already has one, can say "--unset-upstream B"
> and choose to ignore the error if B does not have an upstream.  
>
> If the script is carefully written, it would try to check if B has
> one and call "--unset-upstream B" only when it doesn't.  A casually
> written loose script would say "--unset-upstream B 2>/dev/null"
> and keep going (it would not notice other kinds of errors, but that
> is what makes it "casual and loose").