[PATCH] Do not display 'Switched to a new branch' when the branch existed

[PATCH] Do not display 'Switched to a new branch' when the branch existed

[Knittl]

From cc6410b89b85822aadc5a7843b7398209957e549 Mon Sep 17 00:00:00 2001
From: Tay Ray Chuan <rctay89@gmail.com>
Date: Thu, 24 Jun 2010 03:29:00 +0800
Subject: [PATCH] builtin/checkout: fix info message for `git checkout <branch>`

Since 02ac98374eefbe4a46d4b53a8a78057ad8ad39b7 `git checkout` would
always display 'Switched to a new branch <branch>` even if the branch
had already existed.

Signed-off-by: Daniel Knittl-Frank <knittl89+git@googlemail.com>
---

git checkout should only display 'Switched to a new branch <branch>'
when it creates a new branch, not when it simply switches branches.

ps. I'm not sure about the style used in git for nested ternary
statements (if they should even be used …)

 builtin/checkout.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4ad7427..ed7cde1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -536,7 +536,9 @@ static void update_refs_for_switch(struct
checkout_opts *opts,
 					new->name);
 			else
 				fprintf(stderr, "Switched to%s branch '%s'\n",
-					opts->branch_exists ? " and reset" : " a new",
+					opts->branch_exists
+						? " and reset"
+						: opts->new_branch ? " a new" : "",
 					new->name);
 		}
 		if (old->path && old->name) {
-- 
1.7.1.574.g421e3

[PATCH re-roll] Do not display 'Switched to a new branch' when the branch existed

[Knittl]

From 16f540c87f8c7b87692dfd488d507802ae975312 Mon Sep 17 00:00:00 2001
From: Daniel Knittl-Frank <knittl89+git@googlemail.com>
Date: Wed, 18 Aug 2010 10:35:42 +0200
Subject: [PATCH] builtin/checkout: fix info message for `git checkout <branch>`

Since 02ac98374eefbe4a46d4b53a8a78057ad8ad39b7 `git checkout` would
always display 'Switched to a new branch <branch>` even if the branch
had already existed.

Signed-off-by: Daniel Knittl-Frank <knittl89+git@googlemail.com>
---

stupid me, i forgot to reset author in re-used commit …

 builtin/checkout.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4ad7427..ed7cde1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -536,7 +536,9 @@ static void update_refs_for_switch(struct
checkout_opts *opts,
 					new->name);
 			else
 				fprintf(stderr, "Switched to%s branch '%s'\n",
-					opts->branch_exists ? " and reset" : " a new",
+					opts->branch_exists
+						? " and reset"
+						: opts->new_branch ? " a new" : "",
 					new->name);
 		}
 		if (old->path && old->name) {
-- 
1.7.1.574.g421e3


-- 
typed with http://neo-layout.org
myFtPhp -- visit http://myftphp.sf.net -- v. 0.4.7 released!

Re: [PATCH re-roll] Do not display 'Switched to a new branch' when the branch existed

[Jonathan Nieder]

Hi,

Warning: nitpicks coming.

Knittl wrote:

> From 16f540c87f8c7b87692dfd488d507802ae975312 Mon Sep 17 00:00:00 2001
> From: Daniel Knittl-Frank <knittl89+git@googlemail.com>
> Date: Wed, 18 Aug 2010 10:35:42 +0200
> Subject: [PATCH] builtin/checkout: fix info message for `git checkout <branch>`

On the git list, there are two formats often used for patches (see
Documentation/SubmittingPatches for details): whole-message patches,
which look like this:

	git checkout should only display 'Switched to a new branch <branch>'
	when it creates a new branch, not when it simply switches branches.

	This fixes a bug introduced by 02ac9837 (builtin/checkout:
	learn -B, 2010-06-24).

	Signed-off-by: Daniel Knittl-Frank <knittl89+git@googlemail.com>
	---
	comments of the moment

	 diffstat
	...

and "inline" patches, which look like this:

	comments of the moment
	-- 8< --
	Subject: patch subject

	patch rationale
	---
	 diffstat
	...

and sometimes get used when it is more natural for discussion.

The "From " line and so on output by "git format-patch" are for your
mailer.  Clarifying From:, Date:, and Subject: lines at the start of
your message are allowed, though, and can be useful when forwarding
patches from someone else.

> +++ b/builtin/checkout.c
> @@ -536,7 +536,9 @@ static void update_refs_for_switch(struct
> checkout_opts *opts,
>  					new->name);
>  			else
>  				fprintf(stderr, "Switched to%s branch '%s'\n",
> -					opts->branch_exists ? " and reset" : " a new",
> +					opts->branch_exists
> +						? " and reset"
> +						: opts->new_branch ? " a new" : "",

Maybe it would be clearer to write

	opts->new_branch ? " a new"
		: opts->branch_exists ? " and reset"
		: "",

to emphasize that this is a list of condition/result pairs?

The functionality of your patch is obviously good.  Thanks.

Jonathan

Re: [PATCH re-roll] Do not display 'Switched to a new branch' when the branch existed

[Tay Ray Chuan]

Hi,

On Wed, Aug 18, 2010 at 5:16 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,

Johnathan, thanks for the heads up.

> [snip]
>
> The "From " line and so on output by "git format-patch" are for your
> mailer.  Clarifying From:, Date:, and Subject: lines at the start of
> your message are allowed, though, and can be useful when forwarding
> patches from someone else.

Knittl, I wonder how you generated this patch? Were you working on top
of the "bad" commit?

>> +++ b/builtin/checkout.c
>> @@ -536,7 +536,9 @@ static void update_refs_for_switch(struct
>> checkout_opts *opts,
>>                                       new->name);
>>                       else
>>                               fprintf(stderr, "Switched to%s branch '%s'\n",
>> -                                     opts->branch_exists ? " and reset" : " a new",
>> +                                     opts->branch_exists
>> +                                             ? " and reset"
>> +                                             : opts->new_branch ? " a new" : "",

Strange - I thought I had this sorted out. Thanks for spotting this.

> Maybe it would be clearer to write
>
>        opts->new_branch ? " a new"
>                : opts->branch_exists ? " and reset"
>                : "",
>
> to emphasize that this is a list of condition/result pairs?

We could do with some parentheses - here's my take:

	fprintf(stderr, "Switched to%s branch '%s'\n",
		(opts->branch_exists ? " and reset" :
			(opts->new_branch ? " a new" : "")),
		new->name);

-- 
Cheers,
Ray Chuan

Re: [PATCH re-roll] Do not display 'Switched to a new branch' when the branch existed

[Tay Ray Chuan]

Hi,

oops, seems like you dropped everyone from the Cc list, including the
mailing list. Try using the "Reply to all" next time.

On Wed, Aug 18, 2010 at 9:56 PM, Knittl <knittl89@googlemail.com> wrote:
> [snip
> yes, i branched off of your bad commit (or rather the commit after
> your bad commit "fix detached head usage") and created the commit with
> git commit -c HEAD^ to have the same heading and similar wording
> without opening a second terminal to copy it over. so i accidentally
> sent the patch with your name as author, which i then fixed with git
> amend --reset-author

Why copy over the old commit message? You should be writing one that
fits what you're did, not what *I* did.

-- 
Cheers,
Ray Chuan

Re: [PATCH re-roll] Do not display 'Switched to a new branch' when the branch existed

[Knittl]

sorry for the late reply, i hadn't had access to internet for the last
week and as it turns i sent my response only to tay

On Wed, Aug 18, 2010 at 3:39 PM, Tay Ray Chuan <rctay89@gmail.com> wrote:
> Hi,
>
> On Wed, Aug 18, 2010 at 5:16 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> [snip]
>
>> The "From " line and so on output by "git format-patch" are for your
>> mailer.  Clarifying From:, Date:, and Subject: lines at the start of
>> your message are allowed, though, and can be useful when forwarding
>> patches from someone else.
>
> Knittl, I wonder how you generated this patch? Were you working on top
> of the "bad" commit?

yes, i branched off of your bad commit (or rather the commit after
your bad commit "fix detached head usage") and created the commit with
git commit -c HEAD^ to have the same heading and similar wording
without opening a second terminal to copy it over. so i accidentally
sent the patch with your name as author, which i then fixed with git
amend --reset-author

>>> +++ b/builtin/checkout.c
>>> @@ -536,7 +536,9 @@ static void update_refs_for_switch(struct
>>> checkout_opts *opts,
>>>                                       new->name);
>>>                       else
>>>                               fprintf(stderr, "Switched to%s branch '%s'\n",
>>> -                                     opts->branch_exists ? " and reset" : " a new",
>>> +                                     opts->branch_exists
>>> +                                             ? " and reset"
>>> +                                             : opts->new_branch ? " a new" : "",
>
> Strange - I thought I had this sorted out. Thanks for spotting this.

i tested with next and pu and both tips had the same (confusing) message.

>> Maybe it would be clearer to write
>>
>>        opts->new_branch ? " a new"
>>                : opts->branch_exists ? " and reset"
>>                : "",
>>
>> to emphasize that this is a list of condition/result pairs?
>
> We could do with some parentheses - here's my take:
>
>        fprintf(stderr, "Switched to%s branch '%s'\n",
>                (opts->branch_exists ? " and reset" :
>                        (opts->new_branch ? " a new" : "")),
>                new->name);

that's not really for me to decide, but i'm fine with either version

cheers

-- 
typed with http://neo-layout.org
myFtPhp -- visit http://myftphp.sf.net -- v. 0.4.7 released!

Re: [PATCH re-roll] Do not display 'Switched to a new branch' when the branch existed

[Tay Ray Chuan]

Hi,

On Tue, Aug 24, 2010 at 2:50 PM, Knittl <knittl89@googlemail.com> wrote:
> sorry for the late reply, i hadn't had access to internet for the last
> week and as it turns i sent my response only to tay

just a heads-up - this has already been fixed since 09a0ec5 in master.

-- 
Cheers,
Ray Chuan

Re: [PATCH re-roll] Do not display 'Switched to a new branch' when the branch existed

[Knittl]

On Tue, Aug 24, 2010 at 3:06 PM, Tay Ray Chuan <rctay89@gmail.com> wrote:
> Hi,
>
> On Tue, Aug 24, 2010 at 2:50 PM, Knittl <knittl89@googlemail.com> wrote:
>> sorry for the late reply, i hadn't had access to internet for the last
>> week and as it turns i sent my response only to tay
>
> just a heads-up - this has already been fixed since 09a0ec5 in master.


oh. good :)


-- 
typed with http://neo-layout.org
myFtPhp -- visit http://myftphp.sf.net -- v. 0.4.7 released!

Re: [PATCH re-roll] Do not display 'Switched to a new branch' when the branch existed

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The functionality of your patch is obviously good.  Thanks.

In what way is it good?  I am especially worried about the word "reset"
being confusing.

You are switching to a new context to work on something else, so I don't
necessarily think it is confusing that the word "new branch" in this
message does not mean "a branch that did not exist before this operation
(i.e. a newly created branch)."

Re: [PATCH re-roll] Do not display 'Switched to a new branch' when the branch existed

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> The functionality of your patch is obviously good.  Thanks.
>
> In what way is it good?  I am especially worried about the word "reset"
> being confusing.
>
> You are switching to a new context to work on something else, so I don't
> necessarily think it is confusing that the word "new branch" in this
> message does not mean "a branch that did not exist before this operation
> (i.e. a newly created branch)."

Ahh, please disregard the above; I somehow failed to see that this is only
in the "-b/-B" codepath.  Sorry for the noise.