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

Junio C Hamano [off-list ref] writes:

Junio C Hamano [off-list ref] writes:

quoted

cmn@elego.de (Carlos Martín Nieto) writes:

quoted

Junio C Hamano [off-list ref] writes:

quoted

Carlos Martín Nieto [off-list ref] writes:

quoted

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").

OK, that's a good point, I'll update the patch.

   cmn