Naughty, Evil git-gui patches

Naughty, Evil git-gui patches

[Barry Roberts]

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

I have no hope of these patches being accepted, this is just for 
discussion (and maybe education) purposes, so I attached them all here 
instead of sending individual messages and having to put the disclaimer 
on all of them.

The combination of compulsive command-line aversion among Windows users 
and git-gui not being oriented toward CVS-like usage almost killed our 
cvs->git transition.  These patches are emergency measures to appease 
point-and-clickers transitioning from CVS, but  written by someone who 
doesn't know TCL (or even git).  Written isn't even the appropriate 
word, most are cut&paste from existing git-gui code.

Disclaimers and excuses:
 0001 - All the CVS transition docs talk about pull, but not having it 
in the gui means it doesn't really exist for some users

0002 - I have read the reasons for having merge in the gui be more 
strict than 'git merge' from the command line, but 'cvs update' never 
gave a clean way to back out, so nobody here expects that anyway.

0003 -  Yeah, I want to list the stashes and select from available 
stashes to apply.  But this does the 80% of what we need (reducing 
command line usage), and my tcl sk1llz aren't that l33t.

0004 - This is just a concession to (I think) Tortoise.  Just before you 
commit, you notice that you left in a debug message.  This gives us an 
easy way to fix by diff before commit'ing.  This requires setting 
GIT_EXTERNAL_DIFF, or it's not very interesting.

My intention in sending these is to hopefully generate some discussion 
on how something like these changes can be done properly, with the 
exception of patch 0002, which I fully intend to maintain as a local patch.

Our developers are happy with git now.  Speed, topic branches, speed, 
easy merges from release branches, speed, and my bastard git-gui have 
won over the developers I work with AFAIK.  Thanks for a great suite of 
tools.

Barry Roberts


[-- Attachment #2: 0001-Add-a-simple-git-pull-to-the-menu.patch --]
[-- Type: text/x-patch, Size: 1228 bytes --]

>From 1829aa454f8aad89fca460b0f3f86ecda6ca8556 Mon Sep 17 00:00:00 2001
From: Barry Roberts <blr@robertsr.us>
Date: Tue, 6 May 2008 11:31:52 -0600
Subject: [PATCH] Add a simple "git pull" to the menu.

---
 git-gui/git-gui.sh        |    3 +++
 git-gui/lib/transport.tcl |    9 +++++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 7c25bb9..49c8580 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -2110,6 +2110,9 @@ if {[is_enabled transport]} {
 	.mbar.remote add command \
 		-label [mc "Delete..."] \
 		-command remote_branch_delete::dialog
+        .mbar.remote add command \
+	    -label [mc "Pull..."] \
+	    -command do_pull
 }
 
 if {[is_MacOSX]} {
diff --git a/git-gui/lib/transport.tcl b/git-gui/lib/transport.tcl
index 8e6a9d0..30f1015 100644
--- a/git-gui/lib/transport.tcl
+++ b/git-gui/lib/transport.tcl
@@ -182,3 +182,12 @@ proc do_push_anywhere {} {
 	wm title $w [append "[appname] ([reponame]): " [mc "Push"]]
 	tkwait window $w
 }
+
+proc do_pull {} {
+	set w [console::new \
+		[mc "pull "] \
+		[mc "Pulling changes from default"]]
+	set cmd [list git pull]
+	lappend cmd -v
+	console::exec $w $cmd
+}
-- 
1.5.5.1.116.ge4b9c


[-- Attachment #3: 0002-Removed-strictness-of-merge.patch --]
[-- Type: text/x-patch, Size: 2043 bytes --]

>From 5bad6baa742fb719ada8c0bdcee959d36f2914f2 Mon Sep 17 00:00:00 2001
From: Barry Roberts <blr@robertsr.us>
Date: Tue, 6 May 2008 14:46:35 -0600
Subject: [PATCH] Removed strictness of merge

Started on stash, but that's a bigger project.
---
 git-gui/git-gui.sh    |   13 +++++++++++++
 git-gui/lib/merge.tcl |   10 ----------
 git-gui/lib/stash.tcl |    6 ++++++
 3 files changed, 19 insertions(+), 10 deletions(-)
 create mode 100644 git-gui/lib/stash.tcl

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 49c8580..63c9e09 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -874,6 +874,7 @@ if {$subcommand eq {gui} && [llength $argv] > 0} {
 enable_option multicommit
 enable_option branch
 enable_option transport
+disable_option stash
 disable_option bare
 
 switch -- $subcommand {
@@ -2137,6 +2138,18 @@ if {[is_MacOSX]} {
 		-command do_options
 }
 
+# -- Stash Menu
+#
+
+if {[is_enabled stash]} {
+	.mbar add cascade -label [mc Stash] -menu .mbar.stash
+	menu .mbar.stash
+
+	.mbar.stash add command \
+		-label [mc "List"] \
+		-command do_stash_list
+}
+
 # -- Help Menu
 #
 .mbar add cascade -label [mc Help] -menu .mbar.help
diff --git a/git-gui/lib/merge.tcl b/git-gui/lib/merge.tcl
index cc26b07..d4c44c2 100644
--- a/git-gui/lib/merge.tcl
+++ b/git-gui/lib/merge.tcl
@@ -50,16 +50,6 @@ You must resolve them, stage the file, and commit to complete the current merge.
 			unlock_index
 			return 0
 		}
-		?? {
-			error_popup [mc "You are in the middle of a change.
-
-File %s is modified.
-
-You should complete the current commit before starting a merge.  Doing so will help you abort a failed merge, should the need arise.
-" [short_path $path]]
-			unlock_index
-			return 0
-		}
 		}
 	}
 
diff --git a/git-gui/lib/stash.tcl b/git-gui/lib/stash.tcl
new file mode 100644
index 0000000..4ff2d20
--- /dev/null
+++ b/git-gui/lib/stash.tcl
@@ -0,0 +1,6 @@
+# git stash support
+#
+
+proc do_stash_list {} {
+	error_popup [mc "do_stash_list called."]
+}
\ No newline at end of file
-- 
1.5.5.1.116.ge4b9c


[-- Attachment #4: 0003-Added-VERY-simple-stash.patch --]
[-- Type: text/x-patch, Size: 2103 bytes --]

>From ae77b093e71335d2c2941677e5c366dc31732773 Mon Sep 17 00:00:00 2001
From: Barry Roberts <blr@robertsr.us>
Date: Wed, 7 May 2008 09:20:50 -0600
Subject: [PATCH] Added VERY simple stash

Don't forget to manually re-scan after saving or applyling
---
 git-gui/git-gui.sh    |   11 ++++++++++-
 git-gui/lib/stash.tcl |   36 ++++++++++++++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 63c9e09..0b201ec 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -874,7 +874,7 @@ if {$subcommand eq {gui} && [llength $argv] > 0} {
 enable_option multicommit
 enable_option branch
 enable_option transport
-disable_option stash
+enable_option stash
 disable_option bare
 
 switch -- $subcommand {
@@ -2148,6 +2148,15 @@ if {[is_enabled stash]} {
 	.mbar.stash add command \
 		-label [mc "List"] \
 		-command do_stash_list
+	.mbar.stash add command \
+		-label [mc "Apply"] \
+		-command do_stash_apply
+	.mbar.stash add command \
+		-label [mc "Save"] \
+		-command do_stash_save
+	.mbar.stash add command \
+		-label [mc "Drop"] \
+		-command do_stash_drop
 }
 
 # -- Help Menu
diff --git a/git-gui/lib/stash.tcl b/git-gui/lib/stash.tcl
index 4ff2d20..c2e40db 100644
--- a/git-gui/lib/stash.tcl
+++ b/git-gui/lib/stash.tcl
@@ -2,5 +2,37 @@
 #
 
 proc do_stash_list {} {
-	error_popup [mc "do_stash_list called."]
-}
\ No newline at end of file
+	set w [console::new \
+		[mc "stash"] \
+		[mc "Listing stashes"]]
+	set cmd [list git stash list]
+	lappend cmd -v
+	console::exec $w $cmd
+}
+
+proc do_stash_apply { } {
+	set w [console::new \
+		[mc "stash"] \
+		[mc "Applying stash"]]
+	set cmd [list git stash apply]
+	lappend cmd -v
+	console::exec $w $cmd
+	
+}
+
+proc do_stash_save { } {
+	set w [console::new \
+		[mc "stash"] \
+		[mc "Saving stash"]]
+	set cmd [list git stash save]
+	lappend cmd -v
+	console::exec $w $cmd
+}
+
+proc do_stash_drop { } {
+	set w [console::new \
+		[mc "stash"] \
+		[mc "Dropping stash"]]
+	set cmd [list git stash drop]
+	console::exec $w $cmd
+}
-- 
1.5.5.1.116.ge4b9c


[-- Attachment #5: 0004-Added-cheap-call-to-git-diff-for-ext-diff.patch --]
[-- Type: text/x-patch, Size: 1481 bytes --]

>From 53d911208cb867eab4a2a6d71bfa3d95bc9f2671 Mon Sep 17 00:00:00 2001
From: Barry Roberts <blr@robertsr.us>
Date: Thu, 8 May 2008 11:37:53 -0600
Subject: [PATCH] Added cheap call to git-diff for ext diff

---
 git-gui/git-gui.sh    |    3 +++
 git-gui/lib/index.tcl |   14 ++++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 0b201ec..3b77e68 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -2061,6 +2061,9 @@ if {[is_enabled multicommit] || [is_enabled singlecommit]} {
 	lappend disable_on_lock \
 		[list .mbar.commit entryconf [.mbar.commit index last] -state]
 
+	.mbar.commit add command -label [mc "External Diff"] \
+		-command do_ext_diff_selection
+
 	.mbar.commit add separator
 
 	.mbar.commit add command -label [mc "Show Less Context"] \
diff --git a/git-gui/lib/index.tcl b/git-gui/lib/index.tcl
index 3c1fce7..1e08664 100644
--- a/git-gui/lib/index.tcl
+++ b/git-gui/lib/index.tcl
@@ -435,3 +435,17 @@ proc do_select_commit_type {} {
 		}
 	}
 }
+
+proc do_ext_diff_selection { } {
+	global current_diff_path selected_paths
+
+	if {[array size selected_paths] > 0} {
+		error_popup [mc "Please select one file at a time for diffs"]
+	} elseif {$current_diff_path ne {}} {
+		set w [console::new \
+				   [mc "diff"] \
+				   [mc "Differencing"]]
+		set cmd [list git diff $current_diff_path]
+		console::exec $w $cmd
+	}
+}
\ No newline at end of file
-- 
1.5.5.1.116.ge4b9c

Re: Naughty, Evil git-gui patches

[Shawn O. Pearce]

Barry Roberts <blr@robertsr.us> wrote:
> The combination of compulsive command-line aversion among Windows users 
> and git-gui not being oriented toward CVS-like usage almost killed our 
> cvs->git transition.  These patches are emergency measures to appease 
> point-and-clickers transitioning from CVS, but  written by someone who 
> doesn't know TCL (or even git).  Written isn't even the appropriate 
> word, most are cut&paste from existing git-gui code.

Hah!  Do you know why git-gui came along?  Because it was written as
an emergency measure to allow a transition from no version control
at all to git at my day-job.  :-)

> Disclaimers and excuses:
> 0001 - All the CVS transition docs talk about pull, but not having it 
> in the gui means it doesn't really exist for some users

Yea, that has been a glaring difference between git-gui and command
line for a long time.  There is also the inability to add a remote,
or to do "git pull git://some/where for-linus" or some such one
shot pull.

> 0002 - I have read the reasons for having merge in the gui be more 
> strict than 'git merge' from the command line, but 'cvs update' never 
> gave a clean way to back out, so nobody here expects that anyway.

Maybe its just the way my brain works these days with git, but I
have never found the refusal to merge with a dirty working directory
to be a limitation.  I usually either don't have a dirty working
directory, or I stash into a temporary branch and switch back to
do the merge, then rebase or cherry-pick.  Yea, that does mean I
fall back to the command line in such cases.  Git Gui only users
don't have that option.

Once you get used to the idea of being able to recover your old
state after a merge has started (or even finished!) though the
idea of a dirty merge just sends chills down my spine.  Its a
really bad idea.  IMHO its like holding a loaded gun to your foot
and pulling the trigger every time you do a merge.  After a while
you run out of toes and have lost something you cared about.

Of course there is the school of thought that users should be
given that gun, with extra shells to boot.  I'd rather refuse to
do a dirty merge and let the user work with stashes in Git Gui.
Then the dirty merge error dialog can offer an option to stash
(and try to apply after merge) the dirty changes.  I think that
is the direction you were starting to go here.
 
> 0003 -  Yeah, I want to list the stashes and select from available 
> stashes to apply.  But this does the 80% of what we need (reducing 
> command line usage), and my tcl sk1llz aren't that l33t.

They are apparently l33t enough to come up with this set of changes.
Which is pretty good if you ask me.

Building a good looking list of stashes would probably require using
several columns of text widgets with a single scrollbar.  This is
how the blame viewer and gitk are put together.  Its ugly as s**t.
 
> 0004 - This is just a concession to (I think) Tortoise.  Just before you 
> commit, you notice that you left in a debug message.  This gives us an 
> easy way to fix by diff before commit'ing.  This requires setting 
> GIT_EXTERNAL_DIFF, or it's not very interesting.

I'm not sure I understand this.  Are you trying to get a diff for
the entire working directory against the staged files in the index?
As opposed to looking at each file individually?  What is your
external diff program able to show that git-gui's internal diff
viewer does not?

I'm not objecting to supporting GIT_EXTERNAL_DIFF, I just want to
better understand what you are trying to accomplish here so we can
make sure its the _right_ support.
 
> My intention in sending these is to hopefully generate some discussion 
> on how something like these changes can be done properly, with the 
> exception of patch 0002, which I fully intend to maintain as a local patch.

I definately see some value in your bastard patches and would like to
work with you to get them into a shape that we can include them.  :-)

-- 
Shawn.

Re: Naughty, Evil git-gui patches

[Barry Roberts]

On Thu, 2008-05-22 at 19:24 -0400, Shawn O. Pearce wrote:
> Hah!  Do you know why git-gui came along?  Because it was written as
> an emergency measure to allow a transition from no version control
> at all to git at my day-job.  :-)

Necessity, invention.

> > 0002 - I have read the reasons for having merge in the gui be more 
> > strict than 'git merge' from the command line, but 'cvs update' never 
> > gave a clean way to back out, so nobody here expects that anyway.
> 
> Maybe its just the way my brain works these days with git, but I
> have never found the refusal to merge with a dirty working directory
> to be a limitation.  I usually either don't have a dirty working
> directory, or I stash into a temporary branch and switch back to
> do the merge, then rebase or cherry-pick.  Yea, that does mean I
> fall back to the command line in such cases.  Git Gui only users
> don't have that option.
> 
> Once you get used to the idea of being able to recover your old
> state after a merge has started (or even finished!) though the
> idea of a dirty merge just sends chills down my spine.  Its a
> really bad idea.  IMHO its like holding a loaded gun to your foot
> and pulling the trigger every time you do a merge.  After a while
> you run out of toes and have lost something you cared about.

I agree.  I regard this one as transitional.  People used to thinking in
CVS have a hard time letting go.  My plan for this was to maintain a
local patch until the developers I work with are properly trained, and
then drop it.  But, it's Free Software, so if other folks have the same
pain, it's in the mail archive now.

> > 0003 -  Yeah, I want to list the stashes and select from available 
> > stashes to apply.  But this does the 80% of what we need (reducing 
> > command line usage), and my tcl sk1llz aren't that l33t.
> 
> Building a good looking list of stashes would probably require using
> several columns of text widgets with a single scrollbar.  This is
> how the blame viewer and gitk are put together.  Its ugly as s**t.

When I get time, I still plan to look at it.  But if my Windows using
colleagues don't complain, it may not happen.

A simpler option I considered  is just a simple numeric text field in
the stash commands that are allowed to specify a stash (show, apply,
drop, pop).  Choosing any of these would just open a dialog with a list
of stashes and a text field that defaults to 0.  That text field value
gets put into the stash@{$number}.  Is that too lame to bother
implementing?  Clearly a list to choose from is more standard UI design,
I just don't know if it justifies the cost for me.

>  
> > 0004 - This is just a concession to (I think) Tortoise.  Just before you 
> > commit, you notice that you left in a debug message.  This gives us an 
> > easy way to fix by diff before commit'ing.  This requires setting 
> > GIT_EXTERNAL_DIFF, or it's not very interesting.
> 
> I'm not sure I understand this.  Are you trying to get a diff for
> the entire working directory against the staged files in the index?
> As opposed to looking at each file individually?  What is your
> external diff program able to show that git-gui's internal diff
> viewer does not?

The intent is to edit one file at time to clean up before committing.
The external diff doesn't show anything different, it just lets you
edit.  I use emacs/ediff, win users use winmerge or kdiff3.  So it's
poorly named, but that was the git feature I used. The Tortoise (or
maybe WinCVS) scenario that I'm trying to enable is this:

As I'm staging files for commit, I notice I left in a debug message or
test code that shouldn't be committed, and I want to get rid of in the
file in the working directory.  With that file selected in the "Unstaged
Changes" box, I select "External Diff" from "Commit" menu (at least
that's where it is now).  For me that opens an ediff with my modified
file and the previous version so I can easily remove things I don't want
to commit.  Save, exit, Rescan (necessary?), stage, commit.

Another option I considered was adding to the context menu in the diff
box to revert a hunk in the file.  But sometimes the changes aren't
exactly hunks and you want an editor to, say, just change one line in a
hunk.

What I don't like about my patch now is that it is synchronous.  git-gui
is locked up waiting for the ediff/winmerge/kdiff3 window to close.
Plus, then I have to click OK to close the console window.  Hmm. Maybe I
should look at how you launch gitk.  That seems more appropriate here.


> I'm not objecting to supporting GIT_EXTERNAL_DIFF, I just want to
> better understand what you are trying to accomplish here so we can
> make sure its the _right_ support.

I'm positive there's room for improvement.
 

> I definately see some value in your bastard patches and would like to
> work with you to get them into a shape that we can include them.  :-)

For now, I think the external diff and pull functions would be the
easiest to clean up.  My stash menu is ugly, and if git-gui's merge is
forever safer than cmd line git-merge, that's fine with me.  I would be
interested in your opinion on that prioritization.

Now I get to go play with git and figure out how to disentangle the
mixed stash and merge stuff in patch #2.  That'll be fun.

Thanks,
Barry

Re: Naughty, Evil git-gui patches

[Shawn O. Pearce]

Barry Roberts <blr@robertsr.us> wrote:
> On Thu, 2008-05-22 at 19:24 -0400, Shawn O. Pearce wrote:
> > 
> > Building a good looking list of stashes would probably require using
> > several columns of text widgets with a single scrollbar.  This is
> > how the blame viewer and gitk are put together.  Its ugly as s**t.
> 
> A simpler option I considered  is just a simple numeric text field in
> the stash commands that are allowed to specify a stash (show, apply,
> drop, pop).  Choosing any of these would just open a dialog with a list
> of stashes and a text field that defaults to 0.  That text field value
> gets put into the stash@{$number}.  Is that too lame to bother
> implementing?  Clearly a list to choose from is more standard UI design,
> I just don't know if it justifies the cost for me.

Good point.  A text field would at least let you select a stash,
but I'm not sure there is much of a point without also having a
way to view the stash's diff.  But that could also just be a text
field to enter a stash.  ;-)
 
> > > 0004 - This is just a concession to (I think) Tortoise.  Just before you 
> > > commit, you notice that you left in a debug message.  This gives us an 
> > > easy way to fix by diff before commit'ing.  This requires setting 
> > > GIT_EXTERNAL_DIFF, or it's not very interesting.

OK.  Your (clipped) explanation makes sense.  I'm not sure
GIT_EXTERNAL_DIFF is the best way to select that program; perhaps
it should be a git-gui specific setting in ~/.gitconfig or something.
 
> What I don't like about my patch now is that it is synchronous.  git-gui
> is locked up waiting for the ediff/winmerge/kdiff3 window to close.
> Plus, then I have to click OK to close the console window.  Hmm. Maybe I
> should look at how you launch gitk.  That seems more appropriate here.

I think the procedure is called do_gitk, but it invokes another Tcl/Tk
process with the path of gitk.  The trick to making it run asynchronous
is to append "&" to the end of the argument list when you call exec:

	exec [list $env(GIT_EXTERNAL_DIFF) $file &]

> > I definately see some value in your bastard patches and would like to
> > work with you to get them into a shape that we can include them.  :-)
> 
> For now, I think the external diff and pull functions would be the
> easiest to clean up.  My stash menu is ugly, and if git-gui's merge is
> forever safer than cmd line git-merge, that's fine with me.  I would be
> interested in your opinion on that prioritization.

Both are useful.  Whatever order you want to work on them in.
I think you are right that the diff and pull work is smaller and
easier to clean up.

-- 
Shawn.