git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* preventing checking out remote branch in gitk
@ 2009-06-05 15:21 Sitaram Chamarty
  2009-06-06  4:06 ` [PATCH] gitk: disable checkout of remote branch Sitaram Chamarty
  0 siblings, 1 reply; 23+ messages in thread
From: Sitaram Chamarty @ 2009-06-05 15:21 UTC (permalink / raw)
  To: git

Hello,

I have a fair number of new users, mostly refugees from VSS
and SVN, and they do use the GUIs a fair bit.

And they keep doing this :-)  I know what you're thinking:
teach them not to do it...

But doing this at the command line gives you a nice warning,
and a GUI is (like it or not) considered more "safe" (aka
less powerful).  So it should go one better and just refuse
:-)

Is there any way we can disable "Checkout this branch" for
branches that are not local in gitk?  I'm willing to play
around with the code and test it if someone can give me a
couple of pointers at least.

Thanks,

Sitaram

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH] gitk: disable checkout of remote branch
  2009-06-05 15:21 preventing checking out remote branch in gitk Sitaram Chamarty
@ 2009-06-06  4:06 ` Sitaram Chamarty
  0 siblings, 0 replies; 23+ messages in thread
From: Sitaram Chamarty @ 2009-06-06  4:06 UTC (permalink / raw)
  To: git

At the command line, this gives you a detailed warning message, but the
GUI currently allows it without any fuss.

Since the GUI is often used by people much less familiar with git, it
seems reasonable to make the GUI more restrictive than the command line,
not less.

This prevents a lot of detached HEAD commits by new users.

Signed-off-by: Sitaram Chamarty <sitaramc@gmail.com>
---

I have a lot of new users, and I'd really appreciate it if
someone could review (hey it's my first Tcl code in all my
life!) and approve; this will save me a whole boatload of
trouble :-)

If someone disagrees with the logic that the GUI should be
more "safe" than the command line please do let me know.

 gitk-git/gitk |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
 mode change 100644 => 100755 gitk-git/gitk

diff --git a/gitk-git/gitk b/gitk-git/gitk
old mode 100644
new mode 100755
index 8c66d17..411bc52
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -8770,6 +8770,9 @@ proc headmenu {x y id head} {
     set headmenuid $id
     set headmenuhead $head
     set state normal
+    if {[string match "remotes/*" $head]} {
+	set state disabled
+    }
     if {$head eq $mainhead} {
 	set state disabled
     }
-- 
1.6.3.2

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH] gitk: disable checkout of remote branch
  2009-06-20  6:48 Please pull gitk master branch Paul Mackerras
@ 2009-06-21  9:11 ` Sitaram Chamarty
  2009-06-21  9:22   ` Junio C Hamano
  0 siblings, 1 reply; 23+ messages in thread
From: Sitaram Chamarty @ 2009-06-21  9:11 UTC (permalink / raw)
  To: git

At the command line, this gives you a detailed warning message, but the
GUI currently allows it without any fuss.

Since the GUI is often used by people much less familiar with git, it
seems reasonable to make the GUI more restrictive than the command line,
not less.

This prevents a lot of detached HEAD commits by new users.

Signed-off-by: Sitaram Chamarty <sitaramc@gmail.com>
---

Paul,

[Due to a quirk in how I access this list, the "cc" to you
will come from my work address, not my public gmail address
that the list sees.  Sigh...]

I sent this to the list some time ago, and later someone
helpfully suggested I should copy you.  Not wanting to send
it to the list twice, I sent that only to you, but it
probably got lost.

Could you please approve and include this in your repo?
This patch helps me a lot.

Naturally, if you think it's bad, I'd appreciate hearing
criticism.

Thanks and best regards,

Sitaram

 gitk-git/gitk |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
 mode change 100644 => 100755 gitk-git/gitk

diff --git a/gitk-git/gitk b/gitk-git/gitk
old mode 100644
new mode 100755
index 8c66d17..411bc52
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -8770,6 +8770,9 @@ proc headmenu {x y id head} {
     set headmenuid $id
     set headmenuhead $head
     set state normal
+    if {[string match "remotes/*" $head]} {
+	set state disabled
+    }
     if {$head eq $mainhead} {
 	set state disabled
     }
-- 
1.6.3.2

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-06-21  9:11 ` [PATCH] gitk: disable checkout of remote branch Sitaram Chamarty
@ 2009-06-21  9:22   ` Junio C Hamano
  2009-06-21 14:20     ` Sitaram Chamarty
  2009-06-21 21:34     ` Nanako Shiraishi
  0 siblings, 2 replies; 23+ messages in thread
From: Junio C Hamano @ 2009-06-21  9:22 UTC (permalink / raw)
  To: Sitaram Chamarty; +Cc: git, Paul Mackerras

Sitaram Chamarty <sitaramc@gmail.com> writes:

> At the command line, this gives you a detailed warning message, but the
> GUI currently allows it without any fuss.
>
> Since the GUI is often used by people much less familiar with git, it
> seems reasonable to make the GUI more restrictive than the command line,
> not less.
> ...
> This patch helps me a lot.

The patch seems to disable checkout unconditionally, but it at least needs
an "expert mode" switch to bypass the patch's logic, or (better yet) a
"training wheel" switch for you to set in repositories of the people you
manage.

> diff --git a/gitk-git/gitk b/gitk-git/gitk
> old mode 100644
> new mode 100755
> index 8c66d17..411bc52
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk

The above should ideally read:

> diff --git a/gitk b/gitk
> index 8c66d17..411bc52
> --- a/gitk
> +++ b/gitk

if the patch goes to Paulus.

> @@ -8770,6 +8770,9 @@ proc headmenu {x y id head} {
>      set headmenuid $id
>      set headmenuhead $head
>      set state normal
> +    if {[string match "remotes/*" $head]} {
> +	set state disabled
> +    }
>      if {$head eq $mainhead} {
>  	set state disabled
>      }
> -- 
> 1.6.3.2

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-06-21  9:22   ` Junio C Hamano
@ 2009-06-21 14:20     ` Sitaram Chamarty
  2009-06-21 21:34     ` Nanako Shiraishi
  1 sibling, 0 replies; 23+ messages in thread
From: Sitaram Chamarty @ 2009-06-21 14:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Paul Mackerras

On Sun, Jun 21, 2009 at 2:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Sitaram Chamarty <sitaramc@gmail.com> writes:
>
> > At the command line, this gives you a detailed warning message, but the
> > GUI currently allows it without any fuss.
> >
> > Since the GUI is often used by people much less familiar with git, it
> > seems reasonable to make the GUI more restrictive than the command line,
> > not less.
> > ...
> > This patch helps me a lot.
>
> The patch seems to disable checkout unconditionally, but it at least needs
> an "expert mode" switch to bypass the patch's logic, or (better yet) a
> "training wheel" switch for you to set in repositories of the people you
> manage.

Indeed it does disable checkout of a remote/* branch unconditionally.

I'm not just thinking of people *I* teach when I say that the
"training wheel" mode should be the default.

I believe that when someone does this _from the GUI_, it's 100%
certain they intended something else.  My basis for saying so is (1)
even from CLI, it is quite likely, which is why we have a warning, and
(2) people who use GUI are often much less expert than people who use
CLI.

Actually, what are the odds that someone is expert enough to use a
detached HEAD _properly_ (without shooting themselves in the foot),
but is _not_ expert enough to just say "git checkout origin/master" at
the CLI?  I did not think that combination is worth bothering about.

You're welcome to tell me I'm wrong and that there _are_ such people
-- you guys are the gurus here -- but this is what I believe :-)

[Of course, I could just be trying to cover up the fact that those
were literally the first 3 lines of Tcl I ever wrote in all my life,
and the size and scope of gitk is well beyond my comprehension to do
anything non-trivial :-)  I'll let you decide which it is, heh!]

>
> The above should ideally read:
>
> > diff --git a/gitk b/gitk
> > index 8c66d17..411bc52
> > --- a/gitk
> > +++ b/gitk
>
> if the patch goes to Paulus.

Thanks -- I had not realised that subtlety.

Will make that change and re-send after hearing from either of you
about the above.  Because if the decision is that the patch does need
to be conditional etc., it'll take me a long while anyway :-(

Regards,

Sitaram

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-06-21  9:22   ` Junio C Hamano
  2009-06-21 14:20     ` Sitaram Chamarty
@ 2009-06-21 21:34     ` Nanako Shiraishi
  2009-06-21 23:27       ` Junio C Hamano
  1 sibling, 1 reply; 23+ messages in thread
From: Nanako Shiraishi @ 2009-06-21 21:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Sitaram Chamarty, git, Paul Mackerras

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

> Sitaram Chamarty <sitaramc@gmail.com> writes:
>
>> At the command line, this gives you a detailed warning message, but the
>> GUI currently allows it without any fuss.
>>
>> Since the GUI is often used by people much less familiar with git, it
>> seems reasonable to make the GUI more restrictive than the command line,
>> not less.
>> ...
>> This patch helps me a lot.
>
> The patch seems to disable checkout unconditionally, but it at least needs
> an "expert mode" switch to bypass the patch's logic, or (better yet) a
> "training wheel" switch for you to set in repositories of the people you
> manage.

It will be more helpful if it checked out a new local branch that tracks the remote branch, instead of refusing what the user asked to do. It may need a new dialog that asks to confirm (and allows the user to change) the name of the new branch.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-06-21 21:34     ` Nanako Shiraishi
@ 2009-06-21 23:27       ` Junio C Hamano
  2009-06-22  1:59         ` Sitaram Chamarty
  2009-06-22  3:42         ` Sitaram Chamarty
  0 siblings, 2 replies; 23+ messages in thread
From: Junio C Hamano @ 2009-06-21 23:27 UTC (permalink / raw)
  To: Nanako Shiraishi; +Cc: Sitaram Chamarty, git, Paul Mackerras

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Quoting Junio C Hamano <gitster@pobox.com>:
>
>> Sitaram Chamarty <sitaramc@gmail.com> writes:
>>
>>> At the command line, this gives you a detailed warning message, but the
>>> GUI currently allows it without any fuss.
>>>
>>> Since the GUI is often used by people much less familiar with git, it
>>> seems reasonable to make the GUI more restrictive than the command line,
>>> not less.
>>> ...
>>> This patch helps me a lot.
>>
>> The patch seems to disable checkout unconditionally, but it at least needs
>> an "expert mode" switch to bypass the patch's logic, or (better yet) a
>> "training wheel" switch for you to set in repositories of the people you
>> manage.
>
> It will be more helpful if it checked out a new local branch that tracks
> the remote branch, instead of refusing what the user asked to do. It may
> need a new dialog that asks to confirm (and allows the user to change)
> the name of the new branch.

Heh, stop, step back and think a bit.  I admit that it wasn't just you; we
were both blind.

If we unconditionally disable "check out this branch" from the context
sensitive menu for the tip of a remote tracking branch, I do not think we
lose anything.  If one wants to start a new local branch from there, one
can use the context sensitive menu for an arbitrary commit (rowmenu) and
say "Create new branch".

If we wanted users of gitk to use it to detach HEAD, the current UI is not
a good way to do so anyway --- it only allows detaching the tip of remote
tracking branches and not an arbitrary commit.

	Side note.  I am not arguing it is a good idea.  I am only saying
	that if it were a good idea, such an action should be in rowmenu
	that applies to any commit, not headmenu that applies only to the
	tips of refs. 

So I retract my earlier objection entirely.  I do not think the feature
Sitaram is disabling was meant to allow detaching HEAD at all and it can
be safely disabled for remote tracking branches to make the GUI experience
safer.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-06-21 23:27       ` Junio C Hamano
@ 2009-06-22  1:59         ` Sitaram Chamarty
  2009-06-22  3:42         ` Sitaram Chamarty
  1 sibling, 0 replies; 23+ messages in thread
From: Sitaram Chamarty @ 2009-06-22  1:59 UTC (permalink / raw)
  To: git

On 2009-06-21 23:27:13, Junio C Hamano <gitster@pobox.com> wrote:

> If we wanted users of gitk to use it to detach HEAD, the current UI is not
> a good way to do so anyway --- it only allows detaching the tip of remote
> tracking branches and not an arbitrary commit.

Aaah -- excellent; wish I'd thought of it :-)  Thanks

> So I retract my earlier objection entirely.  I do not think the feature
> Sitaram is disabling was meant to allow detaching HEAD at all and it can
> be safely disabled for remote tracking branches to make the GUI experience
> safer.

Thanks.  I will resubmit to you and Paul again with the diff
header changed to suit his tree.

Regards,

Sitaram

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH] gitk: disable checkout of remote branch
  2009-06-21 23:27       ` Junio C Hamano
  2009-06-22  1:59         ` Sitaram Chamarty
@ 2009-06-22  3:42         ` Sitaram Chamarty
  1 sibling, 0 replies; 23+ messages in thread
From: Sitaram Chamarty @ 2009-06-22  3:42 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: Nanako Shiraishi, git, Junio C Hamano

At the command line, this gives you a detailed warning message, but the
GUI currently allows it without any fuss.

Since the GUI is often used by people much less familiar with git, it
seems reasonable to make the GUI more restrictive than the command line,
not less.

This prevents a lot of detached HEAD commits by new users.

Signed-off-by: Sitaram Chamarty <sitaramc@gmail.com>
---

Paul,

I have amended the diff header to fit the gitk repo as opposed to the
git repo.  To summarise the email discussions, the feature that is
being disabled gets you a detached HEAD, but if that were really the
intent we should allow a detached HEAD from any commit, not just a
head commit.  Therefore this particular disablement is useful and
makes the GUI experience safer.

Please accept.  Thanks.

 gitk |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/gitk b/gitk
index 4604c83..70b6abc 100755
--- a/gitk
+++ b/gitk
@@ -8833,6 +8833,9 @@ proc headmenu {x y id head} {
     set headmenuid $id
     set headmenuhead $head
     set state normal
+    if {[string match "remotes/*" $head]} {
+	set state disabled
+    }
     if {$head eq $mainhead} {
 	set state disabled
     }
-- 
1.6.3.2

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH] gitk: disable checkout of remote branch
@ 2009-11-03 16:00 Sitaram Chamarty
  2009-11-03 16:07 ` Sverre Rabbelier
  2009-11-14 11:14 ` Paul Mackerras
  0 siblings, 2 replies; 23+ messages in thread
From: Sitaram Chamarty @ 2009-11-03 16:00 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: Git Mailing List

At the command line, this gives you a detailed warning message, but the
GUI currently allows it without any fuss.

Since the GUI is often used by people much less familiar with git, it
seems reasonable to make the GUI more restrictive than the command line,
not less.

This prevents a lot of detached HEAD commits by new users.

Signed-off-by: Sitaram Chamarty <sitaramc@gmail.com>
---
 gitk |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/gitk b/gitk
index db5ec54..95e27d4 100755
--- a/gitk
+++ b/gitk
@@ -8897,6 +8897,9 @@ proc headmenu {x y id head} {
     set headmenuid $id
     set headmenuhead $head
     set state normal
+    if {[string match "remotes/*" $head]} {
+	set state disabled
+    }
     if {$head eq $mainhead} {
 	set state disabled
     }
-- 
1.6.5

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-11-03 16:00 Sitaram Chamarty
@ 2009-11-03 16:07 ` Sverre Rabbelier
  2009-11-04  0:48   ` Tim Mazid
  2009-11-14 11:14 ` Paul Mackerras
  1 sibling, 1 reply; 23+ messages in thread
From: Sverre Rabbelier @ 2009-11-03 16:07 UTC (permalink / raw)
  To: Sitaram Chamarty; +Cc: Paul Mackerras, Git Mailing List

Heya,

On Tue, Nov 3, 2009 at 17:00, Sitaram Chamarty <sitaramc@gmail.com> wrote:
> At the command line, this gives you a detailed warning message, but the
> GUI currently allows it without any fuss.

This is even better than an annoying popup dialog, as we all know
those are just ignored anyway :).

-- 
Cheers,

Sverre Rabbelier

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-11-03 16:07 ` Sverre Rabbelier
@ 2009-11-04  0:48   ` Tim Mazid
  2009-11-04  1:58     ` Sitaram Chamarty
  0 siblings, 1 reply; 23+ messages in thread
From: Tim Mazid @ 2009-11-04  0:48 UTC (permalink / raw)
  To: git



Sverre Rabbelier-2 wrote:
> 
> On Tue, Nov 3, 2009 at 17:00, Sitaram Chamarty <sitaramc@gmail.com> wrote:
>> At the command line, this gives you a detailed warning message, but the
>> GUI currently allows it without any fuss.
> 
> This is even better than an annoying popup dialog, as we all know
> those are just ignored anyway :).
> 

Might be better to include a configuration option to allow this, for those
that know what they're doing. Most of the people that know what they're
doing will use the command line, anyway, but it may irritate some people.
-- 
View this message in context: http://n2.nabble.com/PATCH-gitk-disable-checkout-of-remote-branch-tp3939363p3942366.html
Sent from the git mailing list archive at Nabble.com.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-11-04  0:48   ` Tim Mazid
@ 2009-11-04  1:58     ` Sitaram Chamarty
  2009-11-04  6:17       ` Tim Mazid
  0 siblings, 1 reply; 23+ messages in thread
From: Sitaram Chamarty @ 2009-11-04  1:58 UTC (permalink / raw)
  To: Tim Mazid; +Cc: git

On Wed, Nov 4, 2009 at 6:18 AM, Tim Mazid <timmazid@hotmail.com> wrote:

> Might be better to include a configuration option to allow this, for those
> that know what they're doing. Most of the people that know what they're
> doing will use the command line, anyway, but it may irritate some people.

I considered that but found my tcl fu was seriously lacking.  These
are literally the first 3 lines of tcl I ever wrote in my life, and
this program is one huge 11,000+ line monolith, so I'm naturally
scared to make more than very, very, small changes :)

In any case, as you said, most people who know what they're doing can
use the CLI to get there anyway...

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-11-04  1:58     ` Sitaram Chamarty
@ 2009-11-04  6:17       ` Tim Mazid
  2009-11-04  6:41         ` Sverre Rabbelier
  0 siblings, 1 reply; 23+ messages in thread
From: Tim Mazid @ 2009-11-04  6:17 UTC (permalink / raw)
  To: git



Sitaram Chamarty wrote:
> 
> On Wed, Nov 4, 2009 at 6:18 AM, Tim Mazid <timmazid@hotmail.com> wrote:
> 
>> Might be better to include a configuration option to allow this, for
>> those
>> that know what they're doing. Most of the people that know what they're
>> doing will use the command line, anyway, but it may irritate some people.
> 
> I considered that but found my tcl fu was seriously lacking.  These
> are literally the first 3 lines of tcl I ever wrote in my life, and
> this program is one huge 11,000+ line monolith, so I'm naturally
> scared to make more than very, very, small changes :)
> 
> In any case, as you said, most people who know what they're doing can
> use the CLI to get there anyway...
> 

Hm, now that I think about it, what might be better is to just do what
should be done for them. :P

So instead of invoking 'git checkout REMOTE/BRANCH', do 'git checkout -b
BRANCH REMOTE/BRANCH'.

I know you said, you don't know, tcl, but just throwing it out there for
anyone else.

Cheers,
Tim.
-- 
View this message in context: http://n2.nabble.com/PATCH-gitk-disable-checkout-of-remote-branch-tp3939363p3943388.html
Sent from the git mailing list archive at Nabble.com.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-11-04  6:17       ` Tim Mazid
@ 2009-11-04  6:41         ` Sverre Rabbelier
  2009-11-04  7:27           ` Jeff King
  0 siblings, 1 reply; 23+ messages in thread
From: Sverre Rabbelier @ 2009-11-04  6:41 UTC (permalink / raw)
  To: Tim Mazid; +Cc: git

Heya,

On Wed, Nov 4, 2009 at 07:17, Tim Mazid <timmazid@hotmail.com> wrote:
> So instead of invoking 'git checkout REMOTE/BRANCH', do 'git checkout -b
> BRANCH REMOTE/BRANCH'.

Automagically doing 'git checkout -t remote/branch' when asked to do
'git checkout remote/branch' was suggested earlier on the list and I
think there was even a patch that implemented it, not sure what the
outcome of the series was. I do remember that Peff was annoyed by it
at the GitTogether though so it might be a bad idea.

-- 
Cheers,

Sverre Rabbelier

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-11-04  6:41         ` Sverre Rabbelier
@ 2009-11-04  7:27           ` Jeff King
  2009-11-04  9:08             ` Tim Mazid
  2009-11-04 18:03             ` Junio C Hamano
  0 siblings, 2 replies; 23+ messages in thread
From: Jeff King @ 2009-11-04  7:27 UTC (permalink / raw)
  To: Sverre Rabbelier; +Cc: Tim Mazid, git

On Wed, Nov 04, 2009 at 07:41:28AM +0100, Sverre Rabbelier wrote:

> On Wed, Nov 4, 2009 at 07:17, Tim Mazid <timmazid@hotmail.com> wrote:
> > So instead of invoking 'git checkout REMOTE/BRANCH', do 'git checkout -b
> > BRANCH REMOTE/BRANCH'.
> 
> Automagically doing 'git checkout -t remote/branch' when asked to do
> 'git checkout remote/branch' was suggested earlier on the list and I
> think there was even a patch that implemented it, not sure what the
> outcome of the series was. I do remember that Peff was annoyed by it
> at the GitTogether though so it might be a bad idea.

It's in 'next' now. And for the record, my complaint about its behavior
turned out to be partially because I was an idiot. I am still not
convinced that we won't later regret leaving the stale local branch
sitting around, or that users won't find it confusing to see:

  $ git checkout foo
  Branch foo set up to track remote branch foo from origin.
  Switched to a new branch 'foo'

  ... time passes ...

  $ git checkout foo
  Switched to branch 'foo'
  Your branch is behind 'origin/foo' by 1 commit, and can be fast-forwarded.

(i.e., you do the same thing, but get two very different results, and
you have to know how to do the fast-forward. Trivial if you are used to
working with branches, but perhaps not if you are just sightseeing).

But I am no longer planning on writing a long-winded rant about the
feature. ;)

-Peff

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-11-04  7:27           ` Jeff King
@ 2009-11-04  9:08             ` Tim Mazid
  2009-11-04 16:46               ` Sitaram Chamarty
  2009-11-04 18:03             ` Junio C Hamano
  1 sibling, 1 reply; 23+ messages in thread
From: Tim Mazid @ 2009-11-04  9:08 UTC (permalink / raw)
  To: git




Jeff King wrote:
> 
> On Wed, Nov 04, 2009 at 07:41:28AM +0100, Sverre Rabbelier wrote:
> 
>> On Wed, Nov 4, 2009 at 07:17, Tim Mazid <timmazid@hotmail.com> wrote:
>> > So instead of invoking 'git checkout REMOTE/BRANCH', do 'git checkout
>> -b
>> > BRANCH REMOTE/BRANCH'.
>> 
>> Automagically doing 'git checkout -t remote/branch' when asked to do
>> 'git checkout remote/branch' was suggested earlier on the list and I
>> think there was even a patch that implemented it, not sure what the
>> outcome of the series was. I do remember that Peff was annoyed by it
>> at the GitTogether though so it might be a bad idea.
> 
> It's in 'next' now. And for the record, my complaint about its behavior
> turned out to be partially because I was an idiot. I am still not
> convinced that we won't later regret leaving the stale local branch
> sitting around, or that users won't find it confusing to see:
> 
>   $ git checkout foo
>   Branch foo set up to track remote branch foo from origin.
>   Switched to a new branch 'foo'
> 
>   ... time passes ...
> 
>   $ git checkout foo
>   Switched to branch 'foo'
>   Your branch is behind 'origin/foo' by 1 commit, and can be
> fast-forwarded.
> 

Hm. I actually meant inside gitk, not git itself. As in, when you click
inside gitk and try to checkout a remote, it automatically creates a
tracking branch and checks THAT out instead, whereas command-line git works
the same way.
Does that even make sense? :P
-- 
View this message in context: http://n2.nabble.com/PATCH-gitk-disable-checkout-of-remote-branch-tp3939363p3943894.html
Sent from the git mailing list archive at Nabble.com.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-11-04  9:08             ` Tim Mazid
@ 2009-11-04 16:46               ` Sitaram Chamarty
  0 siblings, 0 replies; 23+ messages in thread
From: Sitaram Chamarty @ 2009-11-04 16:46 UTC (permalink / raw)
  To: Tim Mazid; +Cc: git

On Wed, Nov 4, 2009 at 2:38 PM, Tim Mazid <timmazid@hotmail.com> wrote:

> Hm. I actually meant inside gitk, not git itself. As in, when you click
> inside gitk and try to checkout a remote, it automatically creates a
> tracking branch and checks THAT out instead, whereas command-line git works
> the same way.
> Does that even make sense? :P

It certainly does.  And if gitk is internally running the same
"checkout" command that may happen automatically (in 1.7 or whatever).

I will "watch this space" as they say :)  I stated only my minimum
needs (prevent detached HEAD with even less warning [i.e., zero
warning] than in CLI) and managed minimum code for it.  But any other
solution that achives the same effect is also fine!

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-11-04  7:27           ` Jeff King
  2009-11-04  9:08             ` Tim Mazid
@ 2009-11-04 18:03             ` Junio C Hamano
  2009-11-05  7:48               ` Jeff King
  1 sibling, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2009-11-04 18:03 UTC (permalink / raw)
  To: Jeff King; +Cc: Sverre Rabbelier, Tim Mazid, git

Jeff King <peff@peff.net> writes:

> On Wed, Nov 04, 2009 at 07:41:28AM +0100, Sverre Rabbelier wrote:
>
>> On Wed, Nov 4, 2009 at 07:17, Tim Mazid <timmazid@hotmail.com> wrote:
>> > So instead of invoking 'git checkout REMOTE/BRANCH', do 'git checkout -b
>> > BRANCH REMOTE/BRANCH'.
>> 
>> Automagically doing 'git checkout -t remote/branch' when asked to do
>> 'git checkout remote/branch' was suggested earlier on the list and I
>> think there was even a patch that implemented it, not sure what the
>> outcome of the series was. I do remember that Peff was annoyed by it
>> at the GitTogether though so it might be a bad idea.
>
> It's in 'next' now.

Isn't it quite different?  What's in 'next' for 1.7.0 is to guess the
user's intention when:

 - he says 'git checkout BRANCH'; and

 - BRANCH does not yet exist; and

 - BRANCH does not name a commit so the request cannot be to detach HEAD
   at some commit (like REMOTE/BRANCH); and

 - there is a unique REMOTE that has BRANCH.  

The user wants to check out his own BRANCH (the request lacks REMOTE to
start with) but such a branch does not exist yet, and there is only one
sensible commit to start that new branch, hence we DWIM it and helpfully
run "git branch -t BRANCH REMOTE/BRANCH" automatically before performing
"git checkout BRANCH" that was asked.

We never claim to allow checking out the remote tracking branch itself.
The new guessing is only about a local branch that does not exist yet.

> ... I am still not convinced that we won't later regret leaving the
> stale local branch sitting around, or that users won't find it confusing
> to see:
>
>   $ git checkout foo
>   Branch foo set up to track remote branch foo from origin.
>   Switched to a new branch 'foo'
>
>   ... time passes ...
>
>   $ git checkout foo
>   Switched to branch 'foo'
>   Your branch is behind 'origin/foo' by 1 commit, and can be fast-forwarded.
>
> (i.e., you do the same thing, but get two very different results,...

I think this is primarily because the way this DWIM is totally silent in
the transcript is misleading.  If you explain it the way I outlined above,
I do not think there is any confusion.  That is, there is no way for the
user to get confused if the command sequence were like so:

   $ git branch -t foo origin/foo
   Branch foo set up to track remote branch foo from origin.
   $ git checkout foo
   Switched to a new branch 'foo'

   ... time passes ...

   $ git checkout foo
   Switched to branch 'foo'
   Your branch is behind 'origin/foo' by 1 commit, and can be fast-forwarded.

It could just be a matter of telling what we are doing a bit more
explicitly when this DWIM kicks in.  How about this?

   $ git checkout foo
   (first forking your own 'foo' from 'origin/foo')
   Branch foo set up to track remote branch foo from origin.
   Switched to a new branch 'foo'

In any case, I do not think the DWIM would kick in when you try to detach
at remote branch head.  I did not check gitk code to find out the exact
command line it uses, but I do not think it runs "checkout BRANCH".  The
command needs to be at least "checkout REMOTE/BRANCH" to work the way it
does now with any released version of git, and I would not be surprised if
paulus was cautious enough to have spelled it as "refs/REMOTE/BRANCH" to
avoid any potential ambiguity issues.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-11-04 18:03             ` Junio C Hamano
@ 2009-11-05  7:48               ` Jeff King
  2009-11-06  0:45                 ` Junio C Hamano
  0 siblings, 1 reply; 23+ messages in thread
From: Jeff King @ 2009-11-05  7:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Sverre Rabbelier, Tim Mazid, git

On Wed, Nov 04, 2009 at 10:03:49AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Nov 04, 2009 at 07:41:28AM +0100, Sverre Rabbelier wrote:
> >
> >> On Wed, Nov 4, 2009 at 07:17, Tim Mazid <timmazid@hotmail.com> wrote:
> >> > So instead of invoking 'git checkout REMOTE/BRANCH', do 'git checkout -b
> >> > BRANCH REMOTE/BRANCH'.
> >> 
> >> Automagically doing 'git checkout -t remote/branch' when asked to do
> >> 'git checkout remote/branch' was suggested earlier on the list and I
> >> think there was even a patch that implemented it, not sure what the
> >> outcome of the series was. I do remember that Peff was annoyed by it
> >> at the GitTogether though so it might be a bad idea.
> >
> > It's in 'next' now.
> 
> Isn't it quite different?  What's in 'next' for 1.7.0 is to guess the
> user's intention when:

Sorry, yes, I just saw Sverre's comment and misread the original
proposal.  Checking out "$remote/$branch" will still detach the HEAD,
and I don't think anybody has a previous proposal to change that.

> I think this is primarily because the way this DWIM is totally silent in
> the transcript is misleading.  If you explain it the way I outlined above,
> I do not think there is any confusion.  That is, there is no way for the
> user to get confused if the command sequence were like so:
> 
>    $ git branch -t foo origin/foo
>    Branch foo set up to track remote branch foo from origin.
>    $ git checkout foo
>    Switched to a new branch 'foo'
> 
>    ... time passes ...
> 
>    $ git checkout foo
>    Switched to branch 'foo'
>    Your branch is behind 'origin/foo' by 1 commit, and can be fast-forwarded.
> 
> It could just be a matter of telling what we are doing a bit more
> explicitly when this DWIM kicks in.  How about this?
>
>    $ git checkout foo
>    (first forking your own 'foo' from 'origin/foo')
>    Branch foo set up to track remote branch foo from origin.
>    Switched to a new branch 'foo'

This is much better than the current behavior, IMHO. It at least says
what is going on, so a user who actually reads the message will have a
chance of knowing what happened.

The devil's advocate argument is that the difference between the "branch
-t" and the DWIM is that in the former, the user intentionally asks for
a new branch, whereas in the latter, they must realize (by reading and
understanding) that a new branch has been created.

Maybe that difference isn't relevant, and people actually read and
understand everything git says. Maybe not. I dunno. I don't think we
have any real data yet on how people will perceive the feature over
time, and I suspect the only way to get it is to release with it and see
what happens.

> In any case, I do not think the DWIM would kick in when you try to detach
> at remote branch head.  I did not check gitk code to find out the exact
> command line it uses, but I do not think it runs "checkout BRANCH".  The
> command needs to be at least "checkout REMOTE/BRANCH" to work the way it
> does now with any released version of git, and I would not be surprised if
> paulus was cautious enough to have spelled it as "refs/REMOTE/BRANCH" to
> avoid any potential ambiguity issues.

Yes, I was confused when I wrote the original. I agree that "checkout
REMOTE/BRANCH" from the command line should still detach. If gitk wants
to prevent people accidentally detaching HEAD, the context menu for
remote branch boxes should probably detect remote branches and say
something like "Create local branch 'foo' from 'origin/foo'".

-Peff

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-11-05  7:48               ` Jeff King
@ 2009-11-06  0:45                 ` Junio C Hamano
  2009-11-06  8:30                   ` Sverre Rabbelier
  0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2009-11-06  0:45 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Sverre Rabbelier, Tim Mazid, git

Jeff King <peff@peff.net> writes:

>> Isn't it quite different?  What's in 'next' for 1.7.0 is to guess the
>> user's intention when:
>
> Sorry, yes, I just saw Sverre's comment and misread the original
> proposal.  Checking out "$remote/$branch" will still detach the HEAD,
> and I don't think anybody has a previous proposal to change that.

Heh, I think both of us forgot that we decided it is safe enough not to
wait for 1.7.0 already, because the situation this kicks in has always
resulted in an error.  We have it in master since e3de372 (Merge branch
'jc/checkout-auto-track', 2009-10-30).

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-11-06  0:45                 ` Junio C Hamano
@ 2009-11-06  8:30                   ` Sverre Rabbelier
  0 siblings, 0 replies; 23+ messages in thread
From: Sverre Rabbelier @ 2009-11-06  8:30 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Tim Mazid, git

Heya,

On Fri, Nov 6, 2009 at 01:45, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>> Sorry, yes, I just saw Sverre's comment and misread the original
>> proposal.  Checking out "$remote/$branch" will still detach the HEAD,
>> and I don't think anybody has a previous proposal to change that.
>
> Heh, I think both of us forgot that we decided it is safe enough not to
> wait for 1.7.0 already, because the situation this kicks in has always
> resulted in an error.  We have it in master since e3de372 (Merge branch
> 'jc/checkout-auto-track', 2009-10-30).

Sorry for causing so much confusion, I misremembered what the patch
was about ("git checkout <branch>" vs "git checkout
<remote>/<branch>"). Apologies.

-- 
Cheers,

Sverre Rabbelier

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] gitk: disable checkout of remote branch
  2009-11-03 16:00 Sitaram Chamarty
  2009-11-03 16:07 ` Sverre Rabbelier
@ 2009-11-14 11:14 ` Paul Mackerras
  1 sibling, 0 replies; 23+ messages in thread
From: Paul Mackerras @ 2009-11-14 11:14 UTC (permalink / raw)
  To: Sitaram Chamarty; +Cc: Git Mailing List

Sitaram Chamarty writes:

> This prevents a lot of detached HEAD commits by new users.
> 
> Signed-off-by: Sitaram Chamarty <sitaramc@gmail.com>

Thanks, applied.

Paul.

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2009-11-14 11:20 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-05 15:21 preventing checking out remote branch in gitk Sitaram Chamarty
2009-06-06  4:06 ` [PATCH] gitk: disable checkout of remote branch Sitaram Chamarty
  -- strict thread matches above, loose matches on Subject: below --
2009-06-20  6:48 Please pull gitk master branch Paul Mackerras
2009-06-21  9:11 ` [PATCH] gitk: disable checkout of remote branch Sitaram Chamarty
2009-06-21  9:22   ` Junio C Hamano
2009-06-21 14:20     ` Sitaram Chamarty
2009-06-21 21:34     ` Nanako Shiraishi
2009-06-21 23:27       ` Junio C Hamano
2009-06-22  1:59         ` Sitaram Chamarty
2009-06-22  3:42         ` Sitaram Chamarty
2009-11-03 16:00 Sitaram Chamarty
2009-11-03 16:07 ` Sverre Rabbelier
2009-11-04  0:48   ` Tim Mazid
2009-11-04  1:58     ` Sitaram Chamarty
2009-11-04  6:17       ` Tim Mazid
2009-11-04  6:41         ` Sverre Rabbelier
2009-11-04  7:27           ` Jeff King
2009-11-04  9:08             ` Tim Mazid
2009-11-04 16:46               ` Sitaram Chamarty
2009-11-04 18:03             ` Junio C Hamano
2009-11-05  7:48               ` Jeff King
2009-11-06  0:45                 ` Junio C Hamano
2009-11-06  8:30                   ` Sverre Rabbelier
2009-11-14 11:14 ` Paul Mackerras

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).