[PATCH] git-gui: Modify push dialog to support Gerrit review

Problem: It is not possible to push for Gerrit review
as you will always try to push to /refs/heads/... on
the remote. As you should not be forced to work on a
branch with the same name as some branch on the remote,
some more flexibility in the selection of destination
branch is also needed.

Changes done:

Remove selection of branches to push - push always HEAD.
This can be justified by the fact that this far the most
common thing to do.
Specify both the remote repository to push to as well as
the specific branch on that remote. This gives the flexibility.

Add option to specify "Gerrit review". If selected, replace
the traditional "heads" with the artificial "for" in the
destination ref spec. This is what actually solved the trigger
problem.

Limit the branches to select from to the known branches
for currently selected remote. This is motivated in better
usability. Works only when "usettk" is true - it is left for
further study how to change the values in tk_optionMenu on the
fly.

Signed-off-by: Joergen Edelbo <redacted>
---
Hi there,

We are at Napatech A/S just about to roll out a Git/Gerrit/Jenkins
solution. It will really help the gui oriented people in pushing
commits if this can be done directly in git-gui.

BR
Joergen Edelbo
Napatech A/S

 lib/transport.tcl |  184 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 142 insertions(+), 42 deletions(-)

diff --git a/lib/transport.tcl b/lib/transport.tcl
index e5d211e..4c20ef7 100644
--- a/lib/transport.tcl
+++ b/lib/transport.tcl

@@ -59,20 +59,42 @@ proc push_to {remote} {
 	console::exec $w $cmd
 }
 
-proc start_push_anywhere_action {w} {
-	global push_urltype push_remote push_url push_thin push_tags
-	global push_force
-	global repo_config
-
-	set is_mirror 0
-	set r_url {}
+proc get_remote_rep {} {
+	global push_urltype push_remote push_url
+	set rep {}
 	switch -- $push_urltype {
-	remote {
-		set r_url $push_remote
-		catch {set is_mirror $repo_config(remote.$push_remote.mirror)}
+	remote { set rep $push_remote }
+	url    { set rep $push_url }
 	}
-	url {set r_url $push_url}
+	return $rep
+}
+
+proc get_remote_branch {} {
+	global push_branchtype push_branch push_new
+	set branch {}
+	switch -- $push_branchtype {
+	existing { set branch $push_branch }
+	create   { set branch $push_new }
+	}
+   return $branch
+}
+
+proc get_remote_ref_spec {} {
+	global gerrit_review
+	set push_branch [get_remote_branch]
+	if {$gerrit_review} {
+		return "refs/for/$push_branch"
+	} else {
+		return "refs/heads/$push_branch"
 	}
+}
+
+proc start_push_anywhere_action {w} {
+	global push_thin push_tags push_force
+	global repo_config current_branch is_detached
+
+	set is_mirror 0
+	set r_url [get_remote_rep]
 	if {$r_url eq {}} return
 
 	set cmd [list git push]

@@ -87,28 +109,25 @@ proc start_push_anywhere_action {w} {
 		lappend cmd --tags
 	}
 	lappend cmd $r_url
+
+	catch {set is_mirror $repo_config(remote.$r_url.mirror)}
 	if {$is_mirror} {
 		set cons [console::new \
 			[mc "push %s" $r_url] \
 			[mc "Mirroring to %s" $r_url]]
 	} else {
-		set cnt 0
-		foreach i [$w.source.l curselection] {
-			set b [$w.source.l get $i]
-			lappend cmd "refs/heads/$b:refs/heads/$b"
-			incr cnt
-		}
-		if {$cnt == 0} {
-			return
-		} elseif {$cnt == 1} {
-			set unit branch
+		if {$is_detached} {
+			set src HEAD
 		} else {
-			set unit branches
+			set src $current_branch
 		}
+		set dest [get_remote_ref_spec]
+
+		lappend cmd "$src:$dest"
 
 		set cons [console::new \
 			[mc "push %s" $r_url] \
-			[mc "Pushing %s %s to %s" $cnt $unit $r_url]]
+			[mc "Pushing %s to %s" $src $dest]]
 	}
 	console::exec $cons $cmd
 	destroy $w

@@ -117,10 +136,58 @@ proc start_push_anywhere_action {w} {
 trace add variable push_remote write \
 	[list radio_selector push_urltype remote]
 
+proc update_branchtype {br} {
+	global current_branch push_branch push_branchtype
+	if {$br eq {}} {
+		set push_branchtype create
+		set push_branch {}
+	} else {
+		set push_branchtype existing
+		if {[lsearch -sorted -exact $br $current_branch] != -1} {
+			set push_branch $current_branch
+		} elseif {[lsearch -sorted -exact $br master] != -1} {
+			set push_branch master
+		} else {
+			set push_branch [lindex $br 0]
+		}
+	}
+}
+
+proc all_branches_combined {} {
+	set branches [list]
+	foreach spec [all_tracking_branches] {
+		set refn [lindex $spec 2]
+		regsub ^refs/heads/ $refn {} name
+		if { $name ne {HEAD} && [lsearch $branches $name] eq -1} {
+			lappend branches $name
+		}
+	}
+	update_branchtype  $branches
+	return $branches
+}
+
+proc update_branches {} {
+	global push_remote branch_combo
+	set branches [list]
+	foreach spec [all_tracking_branches] {
+		if {[lindex $spec 1] eq $push_remote} {
+			set refn [lindex $spec 0]
+			regsub ^refs/(heads|remotes)/$push_remote/ $refn {} name
+			if {$name ne {HEAD}} {
+				lappend branches $name
+			}
+		}
+	}
+	update_branchtype  $branches
+	$branch_combo configure -values $branches
+	return $branches
+}
+
 proc do_push_anywhere {} {
-	global all_remotes current_branch
-	global push_urltype push_remote push_url push_thin push_tags
-	global push_force use_ttk NS
+	global all_remotes use_ttk branch_combo
+	global push_urltype push_remote push_url
+	global push_branchtype push_branch push_new
+	global push_thin push_tags push_force NS gerrit_review
 
 	set w .push_setup
 	toplevel $w

@@ -129,7 +196,7 @@ proc do_push_anywhere {} {
 	wm geometry $w "+[winfo rootx .]+[winfo rooty .]"
 	pave_toplevel $w
 
-	${NS}::label $w.header -text [mc "Push Branches"] \
+	${NS}::label $w.header -text [mc "Push current HEAD"] \
 		-font font_uibold -anchor center
 	pack $w.header -side top -fill x
 

@@ -144,21 +211,6 @@ proc do_push_anywhere {} {
 	pack $w.buttons.cancel -side right -padx 5
 	pack $w.buttons -side bottom -fill x -pady 10 -padx 10
 
-	${NS}::labelframe $w.source -text [mc "Source Branches"]
-	slistbox $w.source.l \
-		-height 10 \
-		-width 70 \
-		-selectmode extended
-	foreach h [load_all_heads] {
-		$w.source.l insert end $h
-		if {$h eq $current_branch} {
-			$w.source.l select set end
-			$w.source.l yview end
-		}
-	}
-	pack $w.source.l -side left -fill both -expand 1
-	pack $w.source -fill both -expand 1 -pady 5 -padx 5
-
 	${NS}::labelframe $w.dest -text [mc "Destination Repository"]
 	if {$all_remotes ne {}} {
 		${NS}::radiobutton $w.dest.remote_r \

@@ -202,7 +254,51 @@ proc do_push_anywhere {} {
 	grid columnconfigure $w.dest 1 -weight 1
 	pack $w.dest -anchor nw -fill x -pady 5 -padx 5
 
+	${NS}::labelframe $w.destbr -text [mc "Destination Branches"]
+	set all_branches [all_branches_combined]
+	if {$all_branches ne {}} {
+		${NS}::radiobutton $w.destbr.remote_b \
+			-text [mc "Known Branch:        "] \
+			-value existing \
+			-variable push_branchtype
+		if {$use_ttk} {
+			ttk::combobox $w.destbr.remote_n -state readonly \
+				-exportselection false \
+				-textvariable push_branch
+			set branch_combo $w.destbr.remote_n
+			update_branches
+		} else {
+			eval tk_optionMenu $w.destbr.remote_n push_branch $all_branches
+		}
+		grid $w.destbr.remote_b $w.destbr.remote_n -sticky w
+	}
+
+	${NS}::radiobutton $w.destbr.branch_r \
+		-text [mc "Arbitrary Branch:"] \
+		-value create \
+		-variable push_branchtype
+	${NS}::entry $w.destbr.branch_t \
+		-width 50 \
+		-textvariable push_new \
+		-validate key \
+		-validatecommand {
+			if {%d == 1 && [regexp {\s} %S]} {return 0}
+			if {%d == 1 && [string length %S] > 0} {
+				set push_branchtype create
+			}
+			return 1
+		}
+	grid $w.destbr.branch_r $w.destbr.branch_t -sticky we -padx {0 5}
+	${NS}::checkbutton $w.destbr.gerrit \
+		-text [mc "Push for Gerrit review (refs/for/...)"] \
+		-variable gerrit_review
+	grid $w.destbr.gerrit -columnspan 2 -sticky w
+
+	grid columnconfigure $w.destbr 1 -weight 1
+	pack $w.destbr -anchor nw -fill x -pady 5 -padx 5
+
 	${NS}::labelframe $w.options -text [mc "Transfer Options"]
+
 	${NS}::checkbutton $w.options.force \
 		-text [mc "Force overwrite existing branch (may discard changes)"] \
 		-variable push_force

@@ -222,10 +318,14 @@ proc do_push_anywhere {} {
 	set push_force 0
 	set push_thin 0
 	set push_tags 0
+	set gerrit_review 0
 
 	bind $w <Visibility> "grab $w; focus $w.buttons.create"
 	bind $w <Key-Escape> "destroy $w"
 	bind $w <Key-Return> [list start_push_anywhere_action $w]
+	if {$all_remotes ne {}} {
+		bind $w.dest.remote_m <<ComboboxSelected>> { update_branches }
+	}
 	wm title $w [append "[appname] ([reponame]): " [mc "Push"]]
 	wm deiconify $w
 	tkwait window $w

-- 
1.7.9.5

Hi,

On Mon, Sep 02, 2013 at 10:54:19AM +0200, Joergen Edelbo wrote:

Problem: It is not possible to push for Gerrit review
as you will always try to push to /refs/heads/... on
the remote. As you should not be forced to work on a
branch with the same name as some branch on the remote,
some more flexibility in the selection of destination
branch is also needed.

Changes done:

Remove selection of branches to push - push always HEAD.
This can be justified by the fact that this far the most
common thing to do.
Specify both the remote repository to push to as well as
the specific branch on that remote. This gives the flexibility.

Add option to specify "Gerrit review". If selected, replace
the traditional "heads" with the artificial "for" in the
destination ref spec. This is what actually solved the trigger
problem.

Limit the branches to select from to the known branches
for currently selected remote. This is motivated in better
usability. Works only when "usettk" is true - it is left for
further study how to change the values in tk_optionMenu on the
fly.

Signed-off-by: Joergen Edelbo <redacted>
---
Hi there,

We are at Napatech A/S just about to roll out a Git/Gerrit/Jenkins
solution. It will really help the gui oriented people in pushing
commits if this can be done directly in git-gui.

I like where this is heading. I myself was thinking about this some time
ago. Lets include Pat in the CC, since he is the current maintainer of
git-gui.

Give me some time to review/testdrive the patch.

Cheers Heiko

Am 9/2/2013 10:54, schrieb Joergen Edelbo:

Changes done:

Remove selection of branches to push - push always HEAD.
This can be justified by the fact that this far the most
common thing to do.

What are your plans to support a topic-based workflow? "Far the most
common thing to happen" is that someone forgets to push completed topics.
With this change, aren't those people forced to relinguish their current
work because they have to checkout the completed topics to push them?

-- Hannes

Hi,

I am not quite sure what your concern is.

Anyway - I don't think you are any more restricted with this change than before. If you currently work on a topic branch, and the same branch exists on the chosen remote, then this branch will be the default one to push to. 

BR Jørgen Edelbo

-----Original Message-----
From: Johannes Sixt [mailto:j.sixt@viscovery.net] 
Sent: 5. september 2013 08:42
To: Jørgen Edelbo
Cc: git@vger.kernel.org; spearce@spearce.org; hvoigt@hvoigt.net
Subject: Re: [PATCH] git-gui: Modify push dialog to support Gerrit review

Am 9/2/2013 10:54, schrieb Joergen Edelbo:

Changes done:

Remove selection of branches to push - push always HEAD.
This can be justified by the fact that this far the most common thing 
to do.

What are your plans to support a topic-based workflow? "Far the most common thing to happen" is that someone forgets to push completed topics.
With this change, aren't those people forced to relinguish their current work because they have to checkout the completed topics to push them?

-- Hannes

Please do not top-post.

Am 9/5/2013 10:29, schrieb Jørgen Edelbo:

-----Original Message----- From: Johannes Sixt

quoted

Am 9/2/2013 10:54, schrieb Joergen Edelbo:

quoted

Changes done:

Remove selection of branches to push - push always HEAD. This can be
justified by the fact that this far the most common thing to do.

What are your plans to support a topic-based workflow? "Far the most
common thing to happen" is that someone forgets to push completed
topics. With this change, aren't those people forced to relinguish
their current work because they have to checkout the completed topics
to push them?

I am not quite sure what your concern is.

When I have completed topics A and B, but forgot to push them, and now I
am working on topic C, how do I push topics A and B?

You say I can only push HEAD. I understand this that I have to stop work
on C (perhaps commit or stash any unfinished work), then checkout A, push
it, checkout B, push it, checkout C and unstash the unfinished work. If my
understanding is correct, the new restriction is a no-go.

-- Hannes

-----Original Message-----
From: Johannes Sixt [mailto:j.sixt@viscovery.net]
Sent: 5. september 2013 10:57

Please do not top-post.

Am 9/5/2013 10:29, schrieb Jørgen Edelbo:

quoted

-----Original Message----- From: Johannes Sixt

quoted

Am 9/2/2013 10:54, schrieb Joergen Edelbo:

quoted

Changes done:

Remove selection of branches to push - push always HEAD. This can be
justified by the fact that this far the most common thing to do.

What are your plans to support a topic-based workflow? "Far the most
common thing to happen" is that someone forgets to push completed
topics. With this change, aren't those people forced to relinguish
their current work because they have to checkout the completed topics
to push them?

I am not quite sure what your concern is.

When I have completed topics A and B, but forgot to push them, and now I
am working on topic C, how do I push topics A and B?

You say I can only push HEAD. I understand this that I have to stop work on C
(perhaps commit or stash any unfinished work), then checkout A, push it,
checkout B, push it, checkout C and unstash the unfinished work. If my
understanding is correct, the new restriction is a no-go.

Ok, this way of working is not supported. It just never occurred to me that
you would work this way. Forgetting to push something that you have just 
completed is very far from what I am used to. I think it comes most natural
to push what you have done before changing topic. The reason I make a commit
is to get it out of the door.

-- Hannes

- Jørgen

Am 9/5/2013 11:18, schrieb Jørgen Edelbo:

Forgetting to push something that you have just 
completed is very far from what I am used to.

"Forgetting to push" is just one of many reasons why a branch that is not
equal to HEAD is not yet pushed... The new restriction is just too tight.

-- Hannes

On Thu, Sep 05, 2013 at 09:18:25AM +0000, Jørgen Edelbo wrote:

quoted

-----Original Message-----
From: Johannes Sixt [mailto:j.sixt@viscovery.net]
Sent: 5. september 2013 10:57

Please do not top-post.

Am 9/5/2013 10:29, schrieb Jørgen Edelbo:

quoted

-----Original Message----- From: Johannes Sixt

quoted

Am 9/2/2013 10:54, schrieb Joergen Edelbo:

quoted

Changes done:

Remove selection of branches to push - push always HEAD. This can be
justified by the fact that this far the most common thing to do.

What are your plans to support a topic-based workflow? "Far the most
common thing to happen" is that someone forgets to push completed
topics. With this change, aren't those people forced to relinguish
their current work because they have to checkout the completed topics
to push them?

I am not quite sure what your concern is.

When I have completed topics A and B, but forgot to push them, and now I
am working on topic C, how do I push topics A and B?

You say I can only push HEAD. I understand this that I have to stop work on C
(perhaps commit or stash any unfinished work), then checkout A, push it,
checkout B, push it, checkout C and unstash the unfinished work. If my
understanding is correct, the new restriction is a no-go.

Ok, this way of working is not supported. It just never occurred to me that
you would work this way. Forgetting to push something that you have just 
completed is very far from what I am used to. I think it comes most natural
to push what you have done before changing topic. The reason I make a commit
is to get it out of the door.

FWIW, I also think that we should keep the box which allows you to
select the branch to push. I did not realize that you were removing it
when I first glanced at your patch.

Even if your reasoning that pushing the currently checked out branch is
correct: This box has been around for too long, so it will annoy people
that got used to the fact that they can select the branch to push.

Another problem: It is not very intuitive to only select the branch to
push to. You can do that on the command line but IMO using

	git push origin HEAD:refs/heads/<branchname>

is way less common than

	git push origin <branchname>

and I think that should also be reflected in the gui. It might be more
common for a gerrit user but for the typical git user without gerrit it
is not.

So to make it easy for the user to transition from gui to commandline
and back with your patch I would expect: The user selects a branch
to push. The new "Destination Branches" section automatically selects/shows
the same name for the default case as destination (like the cli). So
if I only select the branch to push it behaves the same as before.

If you detect (I assume that is possible somehow) that the remote is a
gerrit remote: "Push for Gerrit review" would automatically be ticked and
the branch a git pull would merge (e.g. the one from branch.<name>.merge)
is selected as the destination branch under refs/for/... . If there is
no config for that, fallback to "master".

This is what I would expect with no further extension of the current git
command line behavior and config options. So that way your patch will be
an *extension* and not a change of behavior.

Another unrelated thing that is currently left out: You can transport
the local branchname when pushing to the magical gerrit refs/for/... . I
would like to see that appended as well. But opposed to the branch
selection that is not a show stopper for the patch more a side note.

Cheers Heiko