[PATCH 1/2] t3200-branch: test setting branch as own upstream

[PATCH 1/2] t3200-branch: test setting branch as own upstream

[Brian Gesiak]

From: modocache <modocache@gmail.com>

No test asserts that "git branch -u refs/heads/my-branch my-branch"
emits a warning. Add a test that does so.

Signed-off-by: Brian Gesiak <modocache@gmail.com>
---
 t/t3200-branch.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index fcdb867..f70b96f 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -507,6 +507,14 @@ EOF
 	test_cmp expected actual
 '
 
+test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' '
+	git branch --set-upstream-to refs/heads/my13 my13 2>actual &&
+	cat >expected <<EOF &&
+warning: Not setting branch my13 as its own upstream.
+EOF
+	test_cmp expected actual
+'
+
 # Keep this test last, as it changes the current branch
 cat >expect <<EOF
 $_z40 $HEAD $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	branch: Created from master
-- 
1.8.3.4 (Apple Git-47)

[PATCH 2/2] branch: use skip_prefix

[Brian Gesiak]

From: modocache <modocache@gmail.com>

The install_branch_config function reimplemented the skip_prefix
function inline. Use skip_prefix function instead for brevity.

Signed-off-by: Brian Gesiak <modocache@gmail.com>
Reported-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 branch.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/branch.c b/branch.c
index 723a36b..e163f3c 100644
--- a/branch.c
+++ b/branch.c
@@ -1,3 +1,4 @@
+#include "git-compat-util.h"
 #include "cache.h"
 #include "branch.h"
 #include "refs.h"
@@ -49,12 +50,11 @@ static int should_setup_rebase(const char *origin)
 
 void install_branch_config(int flag, const char *local, const char *origin, const char *remote)
 {
-	const char *shortname = remote + 11;
-	int remote_is_branch = starts_with(remote, "refs/heads/");
+	const char *shortname = skip_prefix(remote, "refs/heads/");
 	struct strbuf key = STRBUF_INIT;
 	int rebasing = should_setup_rebase(origin);
 
-	if (remote_is_branch
+	if (shortname
 	    && !strcmp(local, shortname)
 	    && !origin) {
 		warning(_("Not setting branch %s as its own upstream."),
@@ -77,29 +77,29 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 	strbuf_release(&key);
 
 	if (flag & BRANCH_CONFIG_VERBOSE) {
-		if (remote_is_branch && origin)
+		if (shortname && origin)
 			printf_ln(rebasing ?
 				  _("Branch %s set up to track remote branch %s from %s by rebasing.") :
 				  _("Branch %s set up to track remote branch %s from %s."),
 				  local, shortname, origin);
-		else if (remote_is_branch && !origin)
+		else if (shortname && !origin)
 			printf_ln(rebasing ?
 				  _("Branch %s set up to track local branch %s by rebasing.") :
 				  _("Branch %s set up to track local branch %s."),
 				  local, shortname);
-		else if (!remote_is_branch && origin)
+		else if (!shortname && origin)
 			printf_ln(rebasing ?
 				  _("Branch %s set up to track remote ref %s by rebasing.") :
 				  _("Branch %s set up to track remote ref %s."),
 				  local, remote);
-		else if (!remote_is_branch && !origin)
+		else if (!shortname && !origin)
 			printf_ln(rebasing ?
 				  _("Branch %s set up to track local ref %s by rebasing.") :
 				  _("Branch %s set up to track local ref %s."),
 				  local, remote);
 		else
-			die("BUG: impossible combination of %d and %p",
-			    remote_is_branch, origin);
+			die("BUG: impossible combination of %p and %p",
+			    shortname, origin);
 	}
 }
 
-- 
1.8.3.4 (Apple Git-47)

Re: [PATCH 2/2] branch: use skip_prefix

[Jeff King]

On Fri, Feb 28, 2014 at 12:04:19PM +0900, Brian Gesiak wrote:

> From: modocache <modocache@gmail.com>

Both your emailed patches have this, which is due to your author name
not matching your sending identity. You probably want to set user.name,
or if you already have (which it looks like you might have from your
Signed-off-by), use "git commit --amend --reset-author" to update the
author information.

> The install_branch_config function reimplemented the skip_prefix
> function inline. Use skip_prefix function instead for brevity.
> 
> Signed-off-by: Brian Gesiak <modocache@gmail.com>
> Reported-by: Michael Haggerty <mhagger@alum.mit.edu>

It's a minor thing, but usually these footer lines try to follow a
chronological order. So the report would come before the signoff (and a
further signoff from the maintainer would go after yours).

> diff --git a/branch.c b/branch.c
> index 723a36b..e163f3c 100644
> --- a/branch.c
> +++ b/branch.c
> [...]

The patch itself looks OK to me.

-Peff

Re: [PATCH 2/2] branch: use skip_prefix

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Feb 28, 2014 at 12:04:19PM +0900, Brian Gesiak wrote:
>
>> From: modocache <modocache@gmail.com>
>
> Both your emailed patches have this, which is due to your author name
> not matching your sending identity. You probably want to set user.name,
> or if you already have (which it looks like you might have from your
> Signed-off-by), use "git commit --amend --reset-author" to update the
> author information.
>
>> The install_branch_config function reimplemented the skip_prefix
>> function inline. Use skip_prefix function instead for brevity.
>> 
>> Signed-off-by: Brian Gesiak <modocache@gmail.com>
>> Reported-by: Michael Haggerty <mhagger@alum.mit.edu>
>
> It's a minor thing, but usually these footer lines try to follow a
> chronological order. So the report would come before the signoff (and a
> further signoff from the maintainer would go after yours).
>
>> diff --git a/branch.c b/branch.c
>> index 723a36b..e163f3c 100644
>> --- a/branch.c
>> +++ b/branch.c
>> [...]
>
> The patch itself looks OK to me.
>
> -Peff

Thanks.  Queued and pushed out on 'pu' with fixups already.

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Jeff King]

On Fri, Feb 28, 2014 at 12:04:18PM +0900, Brian Gesiak wrote:

> No test asserts that "git branch -u refs/heads/my-branch my-branch"
> emits a warning. Add a test that does so.

For an operation like "git branch foo origin" where setting up the
tracking is a side effect, a warning makes sense. But the sole purpose
of the command above is to set the upstream, and we didn't do it; should
this warning actually be upgraded to an error?

> +test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' '
> +	git branch --set-upstream-to refs/heads/my13 my13 2>actual &&
> +	cat >expected <<EOF &&
> +warning: Not setting branch my13 as its own upstream.
> +EOF
> +	test_cmp expected actual
> +'

This should use test_i18ncmp, as the string you are matching is
internationalized.

-Peff

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Brian Gesiak]

> For an operation like "git branch foo origin" where setting up the
> tracking is a side effect, a warning makes sense. But the sole purpose
> of the command above is to set the upstream, and we didn't do it; should
> this warning actually be upgraded to an error?

I agree. I originally wrote the test using test_expect_failure--imagine my
surprise when the exit status was 0, despite the fact that the upstream wasn't
set!

> This should use test_i18ncmp, as the string you are matching is
> internationalized.

Patch is on the way, just waiting for the tests to complete. Thanks for pointing
that out! Also, sorry if it's in the Makefile somewhere, but is there
an easy way
to run just a single test file in the t directory?

- Brian Gesiak

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Jeff King]

On Fri, Feb 28, 2014 at 03:17:28PM +0900, Brian Gesiak wrote:

> > For an operation like "git branch foo origin" where setting up the
> > tracking is a side effect, a warning makes sense. But the sole purpose
> > of the command above is to set the upstream, and we didn't do it; should
> > this warning actually be upgraded to an error?
> 
> I agree. I originally wrote the test using test_expect_failure--imagine my
> surprise when the exit status was 0, despite the fact that the upstream wasn't
> set!

For reference, you don't want test_expect_failure here; it is about "we
want this to succeed, but git is currently buggy and we know it, so mark
it as a failing test". You want test_must_fail to indicate a command
inside a test that must exit non-zero:

  test_expect_success 'pointing upstream to itself fails' '
          test_must_fail git branch -u ...
  '

I notice that the warning comes from install_branch_config, which gets
used both for "branch -u", but also in the "side effect" case I
mentioned above. Is it possible to trigger this as part of such a case?
I think maybe "git branch -f --track foo foo" would do it. If so, we
should perhaps include a test that it does not break if we upgrade the
"-u" case to an error.

> Patch is on the way, just waiting for the tests to complete. Thanks
> for pointing that out! Also, sorry if it's in the Makefile somewhere,
> but is there an easy way to run just a single test file in the t
> directory?

You can run "./tXXXX-....sh" explicitly.

-Peff

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

On Fri, Feb 28, 2014 at 01:27:15AM -0500, Jeff King wrote:
> On Fri, Feb 28, 2014 at 03:17:28PM +0900, Brian Gesiak wrote:
> > Patch is on the way, just waiting for the tests to complete. Thanks
> > for pointing that out! Also, sorry if it's in the Makefile somewhere,
> > but is there an easy way to run just a single test file in the t
> > directory?
> 
> You can run "./tXXXX-....sh" explicitly.

You can also use Perl's prove command, which provides some nice-to-have
features, such as exiting abnormally if your tests abort prematurely.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Johannes Sixt]

Am 2/28/2014 6:37, schrieb Jeff King:
> On Fri, Feb 28, 2014 at 12:04:18PM +0900, Brian Gesiak wrote:
> 
>> No test asserts that "git branch -u refs/heads/my-branch my-branch"
>> emits a warning. Add a test that does so.
> 
> For an operation like "git branch foo origin" where setting up the
> tracking is a side effect, a warning makes sense. But the sole purpose
> of the command above is to set the upstream, and we didn't do it; should
> this warning actually be upgraded to an error?
> 
>> +test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' '
>> +	git branch --set-upstream-to refs/heads/my13 my13 2>actual &&
>> +	cat >expected <<EOF &&
>> +warning: Not setting branch my13 as its own upstream.
>> +EOF
>> +	test_cmp expected actual
>> +'
> 
> This should use test_i18ncmp, as the string you are matching is
> internationalized.

More generally, stderr output shouldn't be tested with test_cmp or
test_i18ncmp at all, but with grep and test_i18ngrep. The reason is that
when you run the test with 'sh -x t3200* -v -i', the trace output is also
in stderr, and the test_cmp/test_i18ncmp fails due to the unexpected extra
text.

-- Hannes

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Jeff King]

On Fri, Feb 28, 2014 at 07:55:25AM +0100, Johannes Sixt wrote:

> > This should use test_i18ncmp, as the string you are matching is
> > internationalized.
> 
> More generally, stderr output shouldn't be tested with test_cmp or
> test_i18ncmp at all, but with grep and test_i18ngrep. The reason is that
> when you run the test with 'sh -x t3200* -v -i', the trace output is also
> in stderr, and the test_cmp/test_i18ncmp fails due to the unexpected extra
> text.

I didn't think we bothered to make "sh -x" work robustly. I don't mind
if we do, but "git grep -E 'test_(i18n)?cmp .*err" shows many potential
problem spots.

Hmm. Looks like it is only a problem if you are calling a shell function
(since it is the shell function's trace output you are seeing). So this
test would be OK as-is, but testing for an error, like:

  test_must_fail git branch -u foo foo 2>stderr

would not be, because we see the trace from test_must_fail. So some of
the callsites found by my grep are actually probably fine.

-Peff

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Jeff King]

On Fri, Feb 28, 2014 at 02:14:01AM -0500, Jeff King wrote:

> I didn't think we bothered to make "sh -x" work robustly. I don't mind
> if we do, but "git grep -E 'test_(i18n)?cmp .*err" shows many potential
> problem spots.

Just for fun:

  cd t
  make SHELL_PATH="sh -x" prove

causes 326 test failures across 43 scripts. That's slightly misleading,
because 200 of the failures are all in t0008, and updating one function
would probably clear up all of them.

-Peff

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Brian Gesiak]

> Hmm. Looks like it is only a problem if you are calling a shell function
> (since it is the shell function's trace output you are seeing). So this
> test would be OK as-is

Indeed, this test passes when run locally, even using "sh -x".

I would be in favor of using test_i18ngrep, but it seems like this
test file overwhelmingly uses test_(i18n)cmp, even when inspecting
stderr output. Making double-sure that all tests pass when run with
"sh -x" seems like a larger endeavor.

Of course, I'd be happy to submit several patches if there's support
for such a change. But as Peff points out it will be a large diff.

- Brian Gesiak

On Fri, Feb 28, 2014 at 4:26 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Feb 28, 2014 at 02:14:01AM -0500, Jeff King wrote:
>
>> I didn't think we bothered to make "sh -x" work robustly. I don't mind
>> if we do, but "git grep -E 'test_(i18n)?cmp .*err" shows many potential
>> problem spots.
>
> Just for fun:
>
>   cd t
>   make SHELL_PATH="sh -x" prove
>
> causes 326 test failures across 43 scripts. That's slightly misleading,
> because 200 of the failures are all in t0008, and updating one function
> would probably clear up all of them.
>
> -Peff

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Jeff King]

On Fri, Feb 28, 2014 at 04:28:38PM +0900, Brian Gesiak wrote:

> I would be in favor of using test_i18ngrep, but it seems like this
> test file overwhelmingly uses test_(i18n)cmp, even when inspecting
> stderr output.

We generally prefer "cmp" checks to "grep" checks, because they are more
rigorous. However, when testing human-readable output which may change,
sometimes being too specific can simply make the tests brittle and
annoying. Using a forgiving regex that matches keywords can be helpful.
So there's definitely some room for judgement.

I think what you posted as v2 looks OK.

> Making double-sure that all tests pass when run with "sh -x" seems
> like a larger endeavor.
> 
> Of course, I'd be happy to submit several patches if there's support
> for such a change. But as Peff points out it will be a large diff.

Yeah, I don't think it's worth the effort.

If you feel like continuing on this series, converting the warning()
into a die() would be a much more productive use of time (but if you
don't, I do not see any reason not to take the patches you've posted).

-Peff

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Brian Gesiak]

> If you feel like continuing on this series, converting the warning()
> into a die() would be a much more productive use of time (but if you
> don't, I do not see any reason not to take the patches you've posted).

I'd be happy to keep working on this. In fact, I think I have a patch
for this ready. But just to clarify:

> I notice that the warning comes from install_branch_config, which gets
> used both for "branch -u", but also in the "side effect" case I
> mentioned above. Is it possible to trigger this as part of such a case?
> I think maybe "git branch -f --track foo foo" would do it. If so, we
> should perhaps include a test that it does not break if we upgrade the
> "-u" case to an error.

Do you mean that install_branch_config should continue to emit a
warning in the "side effect" case? I'm not sure I agree--how is "git
branch -f --track foo foo" less erroneous than "git branch -u foo
refs/heads/foo"? Perhaps I'm missing some insight on how "--track" is
used.

The tests appear to already cover all instances in which
install_branch_config is called, and bumping the warning to an error
does not cause any test failures.

- Brian Gesiak

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Jeff King]

On Fri, Feb 28, 2014 at 07:44:10PM +0900, Brian Gesiak wrote:

> > I notice that the warning comes from install_branch_config, which gets
> > used both for "branch -u", but also in the "side effect" case I
> > mentioned above. Is it possible to trigger this as part of such a case?
> > I think maybe "git branch -f --track foo foo" would do it. If so, we
> > should perhaps include a test that it does not break if we upgrade the
> > "-u" case to an error.
> 
> Do you mean that install_branch_config should continue to emit a
> warning in the "side effect" case? I'm not sure I agree--how is "git
> branch -f --track foo foo" less erroneous than "git branch -u foo
> refs/heads/foo"? Perhaps I'm missing some insight on how "--track" is
> used.

I'd be more worried about triggering it via the config. E.g.:

  git config branch.autosetupmerge always
  git branch -f foo foo

Should the second command die? I admit I'm having a hard time coming up
with a feasible reason why anyone would do "branch -f foo foo" in the
first place. I just don't want to regress somebody else's workflow due
to my lack of imagination.

-Peff

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Brian Gesiak]

> I just don't want to regress somebody else's workflow due
> to my lack of imagination.

This makes a lot of sense to me, although as-is the function emits a
warning and returns immediately (although with a successful status
code), so I'm also stumped as to what kind of workflow this would be
included in.

In any case, if the jury's out on this one, I suppose the two patches
I submitted are good to merge? Part of me thinks the bump from warning
to error belongs in its own patch anyway.

- Brian Gesiak

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Jeff King]

On Fri, Feb 28, 2014 at 08:16:13PM +0900, Brian Gesiak wrote:

> > I just don't want to regress somebody else's workflow due
> > to my lack of imagination.
> 
> This makes a lot of sense to me, although as-is the function emits a
> warning and returns immediately (although with a successful status
> code), so I'm also stumped as to what kind of workflow this would be
> included in.

I'm cc-ing Matthieu, who wrote 85e2233, which introduces the check. Its
commit message says:

  branch: warn and refuse to set a branch as a tracking branch of
  itself.

  Previous patch allows commands like "git branch --set-upstream foo
  foo", which doesn't make much sense. Warn the user and don't change
  the configuration in this case. Don't die to let the caller finish its
  job in such case.

For those just joining us, we are focused on the final statement, and
deciding whether we should die() in this case. But I am not clear what
job it would want to be finishing (creating the branch, I guess, but you
cannot be doing anything useful, since by definition the branch already
exists and you are not changing its tip). There wasn't any relevant
discussion on the list[1]. Matthieu, can you remember anything else that
led to that decision?

> In any case, if the jury's out on this one, I suppose the two patches
> I submitted are good to merge? Part of me thinks the bump from warning
> to error belongs in its own patch anyway.

Yeah, it should definitely be a separate patch on top.

-Peff

[1] Threads are:

      http://thread.gmane.org/gmane.comp.version-control.git/137297/focus=137299

    and

      http://thread.gmane.org/gmane.comp.version-control.git/137401/focus=137403

    but the discussion focused on the first part of the series.

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

>   Don't die to let the caller finish its
>   job in such case.

[...]

> Matthieu, can you remember anything else that
> led to that decision?

Not at all, unfortunately. I don't remember if I did that "in case
there's something like some cleanup to do" or because I had something
more precise in mind.

A case to be carefull about is if you're using the same "git branch"
command for multiple actions (trying --set-upstream in combination with
other options). But I do not see a case where this would be possible.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH v2] branch: die when setting branch as own upstream

[Brian Gesiak]

From: modocache <modocache@gmail.com>

Branch set as own upstream using one of the following commands returns
immediately with an exit code of 0:

- `git branch --set-upstream-to foo refs/heads/foo`
- `git branch --force --track foo foo`

Since neither of these actions currently set the upstream, an exit code
of 0 is misleading. Instead, exit with a status code indicating failure
by using the die function.

Signed-off-by: Brian Gesiak <modocache@gmail.com>
---
 branch.c          | 9 ++-------
 t/t3200-branch.sh | 6 +++---
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/branch.c b/branch.c
index e163f3c..9bac8b5 100644
--- a/branch.c
+++ b/branch.c
@@ -54,13 +54,8 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 	struct strbuf key = STRBUF_INIT;
 	int rebasing = should_setup_rebase(origin);
 
-	if (shortname
-	    && !strcmp(local, shortname)
-	    && !origin) {
-		warning(_("Not setting branch %s as its own upstream."),
-			local);
-		return;
-	}
+	if (shortname && !strcmp(local, shortname) && !origin)
+		die(_("Not setting branch %s as its own upstream."), local);
 
 	strbuf_addf(&key, "branch.%s.remote", local);
 	git_config_set(key.buf, origin ? origin : ".");
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 6164126..3ac493f 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -507,10 +507,10 @@ EOF
 	test_cmp expected actual
 '
 
-test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' '
-	git branch --set-upstream-to refs/heads/my13 my13 2>actual &&
+test_expect_success '--set-upstream-to fails if used to set branch as own upstream' '
+	test_must_fail git branch --set-upstream-to refs/heads/my13 my13 2>actual &&
 	cat >expected <<EOF &&
-warning: Not setting branch my13 as its own upstream.
+fatal: Not setting branch my13 as its own upstream.
 EOF
 	test_i18ncmp expected actual
 '
-- 
1.8.3.4 (Apple Git-47)

[PATCH 3/3] branch: die when setting branch as own upstream

[Brian Gesiak]

Branch set as own upstream using one of the following commands returns
immediately with an exit code of 0:

- `git branch --set-upstream-to foo refs/heads/foo`
- `git branch --force --track foo foo`

Since neither of these actions currently set the upstream, an exit code
of 0 is misleading. Instead, exit with a status code indicating failure
by using the die function.

Signed-off-by: Brian Gesiak <modocache@gmail.com>
---
 branch.c          | 9 ++-------
 t/t3200-branch.sh | 6 +++---
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/branch.c b/branch.c
index e163f3c..9bac8b5 100644
--- a/branch.c
+++ b/branch.c
@@ -54,13 +54,8 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 	struct strbuf key = STRBUF_INIT;
 	int rebasing = should_setup_rebase(origin);
 
-	if (shortname
-	    && !strcmp(local, shortname)
-	    && !origin) {
-		warning(_("Not setting branch %s as its own upstream."),
-			local);
-		return;
-	}
+	if (shortname && !strcmp(local, shortname) && !origin)
+		die(_("Not setting branch %s as its own upstream."), local);
 
 	strbuf_addf(&key, "branch.%s.remote", local);
 	git_config_set(key.buf, origin ? origin : ".");
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 6164126..3ac493f 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -507,10 +507,10 @@ EOF
 	test_cmp expected actual
 '
 
-test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' '
-	git branch --set-upstream-to refs/heads/my13 my13 2>actual &&
+test_expect_success '--set-upstream-to fails if used to set branch as own upstream' '
+	test_must_fail git branch --set-upstream-to refs/heads/my13 my13 2>actual &&
 	cat >expected <<EOF &&
-warning: Not setting branch my13 as its own upstream.
+fatal: Not setting branch my13 as its own upstream.
 EOF
 	test_i18ncmp expected actual
 '
-- 
1.8.3.4 (Apple Git-47)

Re: [PATCH 3/3] branch: die when setting branch as own upstream

[Brian Gesiak]

Sorry for the multiple patches--I noticed the commit author was off in
the first one.

This patch converts the warning to an error, should it be decided that
it's prudent to do so (I'm in favor of doing so). If not, I think the
other two patches I submitted are good to merge.

Thanks for all the feedback so far!

- Brian Gesiak


On Sat, Mar 1, 2014 at 9:23 PM, Brian Gesiak <modocache@gmail.com> wrote:
> Branch set as own upstream using one of the following commands returns
> immediately with an exit code of 0:
>
> - `git branch --set-upstream-to foo refs/heads/foo`
> - `git branch --force --track foo foo`
>
> Since neither of these actions currently set the upstream, an exit code
> of 0 is misleading. Instead, exit with a status code indicating failure
> by using the die function.
>
> Signed-off-by: Brian Gesiak <modocache@gmail.com>
> ---
>  branch.c          | 9 ++-------
>  t/t3200-branch.sh | 6 +++---
>  2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index e163f3c..9bac8b5 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -54,13 +54,8 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
>         struct strbuf key = STRBUF_INIT;
>         int rebasing = should_setup_rebase(origin);
>
> -       if (shortname
> -           && !strcmp(local, shortname)
> -           && !origin) {
> -               warning(_("Not setting branch %s as its own upstream."),
> -                       local);
> -               return;
> -       }
> +       if (shortname && !strcmp(local, shortname) && !origin)
> +               die(_("Not setting branch %s as its own upstream."), local);
>
>         strbuf_addf(&key, "branch.%s.remote", local);
>         git_config_set(key.buf, origin ? origin : ".");
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 6164126..3ac493f 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -507,10 +507,10 @@ EOF
>         test_cmp expected actual
>  '
>
> -test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' '
> -       git branch --set-upstream-to refs/heads/my13 my13 2>actual &&
> +test_expect_success '--set-upstream-to fails if used to set branch as own upstream' '
> +       test_must_fail git branch --set-upstream-to refs/heads/my13 my13 2>actual &&
>         cat >expected <<EOF &&
> -warning: Not setting branch my13 as its own upstream.
> +fatal: Not setting branch my13 as its own upstream.
>  EOF
>         test_i18ncmp expected actual
>  '
> --
> 1.8.3.4 (Apple Git-47)
>

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Johannes Sixt]

Am 2/28/2014 8:14, schrieb Jeff King:
> I didn't think we bothered to make "sh -x" work robustly. I don't mind
> if we do, but "git grep -E 'test_(i18n)?cmp .*err" shows many potential
> problem spots.
> 
> Hmm. Looks like it is only a problem if you are calling a shell function
> (since it is the shell function's trace output you are seeing). So this
> test would be OK as-is, but testing for an error, like:
> 
>   test_must_fail git branch -u foo foo 2>stderr
> 
> would not be, because we see the trace from test_must_fail. So some of
> the callsites found by my grep are actually probably fine.

Yeah, your assessment is correct: only shell function output is affected.

Some time (years?) ago, I used to run the tests on Windows with sh -x,
redirected to a log file to be able to trace intermittent failures. It was
distracting to find many false positives. Today, I don't do that anymore,
and it is not a big deal for me, just like for anybody else ;-)

Consider the topic settled.

-- Hannes