threads / discuss / 19873

Please pull gitk master branch

Subject: Please pull gitk master branch

## tl;dr

9 messages between Jun 20, 2009 and Jun 22, 2009.

replies: 8people: 4as markdown or json

Paul Mackerras· Jun 20, 2009, 06:48 UTC · lore
Junio,

Please do a pull from my gitk repository master branch to get the following commits:

Christian Stimming (1):
      gitk: Update German translation.
Dirk Suesserott (1):
      gitk: Add option 'Simple history' to the options menu
Elijah Newren (1):
      gitk: Make more options easily accessible from Edit View dialog
Johannes Sixt (1):
      gitk: Use --textconv to generate diff text
Markus Heidelberg (1):
      gitk: Allow diff view without context lines
Michele Ballabio (1):
      gitk: Add another string to translation
Pat Thoyts (1):
      gitk: Handle msysGit version during version comparisons
Paul Mackerras (1):
      gitk: Check git version before using --textconv flag

Thanks, Paul.

Sitaram Chamarty· Jun 21, 2009, 09:11 UTC · re: Paul Mackerras · lore

[PATCH] gitk: disable checkout of remote branch

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
Junio C Hamano· Jun 21, 2009, 09:22 UTC · re: Sitaram Chamarty · lore

Re: [PATCH] gitk: disable checkout of remote branch

Sitaram Chamarty <sitaramc@gmail.com> writes:
Show 8 quoted lines
> 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.

Show 6 quoted lines
> 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.
Show 12 quoted lines
> @@ -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
Sitaram Chamarty· Jun 21, 2009, 14:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] gitk: disable checkout of remote branch

On Sun, Jun 21, 2009 at 2:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
>
> 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!]

Show 9 quoted lines
>
> 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
Nanako Shiraishi· Jun 21, 2009, 21:34 UTC · re: Junio C Hamano · lore

Re: [PATCH] gitk: disable checkout of remote branch

Quoting Junio C Hamano <gitster@pobox.com>:
Show 15 quoted lines
> 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/
Junio C Hamano· Jun 21, 2009, 23:27 UTC · re: Nanako Shiraishi · lore

Re: [PATCH] gitk: disable checkout of remote branch

Nanako Shiraishi <nanako3@lavabit.com> writes:
Show 22 quoted lines
> 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.

Sitaram Chamarty· Jun 22, 2009, 01:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] gitk: disable checkout of remote branch

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
Sitaram Chamarty· Jun 22, 2009, 03:42 UTC · re: Junio C Hamano · lore

[PATCH] gitk: disable checkout of remote branch

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

← back to recent threads