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

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

[Joergen Edelbo]

Problem: It is not possible to push for Gerrit review as you will
always try to push to "refs/heads/..." on the remote.

Changes done:

Add an option to select "Gerrit review" and a corresponding entry
for a branch name. If this option is selected, push the changes to
"refs/for/<gerrit-branch>/<local branch>". In this way the local branch
names will be used as topic branches on Gerrit.
---
Hi all,

This is a second attempt to support Gerrit review. It is fully backwards
compatible as the Gerrit option is an addition only. It is also better
than the first approach as it supports pushing more local branches to
Gerrit - each having their own topic branch there.

Further improvement could be to make the Gerrit branch specification a
drop down list, but I would like to have this first simple approach 
evaluated first.

BR Joergen

 lib/transport.tcl |   30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/lib/transport.tcl b/lib/transport.tcl
index e5d211e..adf2bbb 100644
--- a/lib/transport.tcl
+++ b/lib/transport.tcl
@@ -61,6 +61,7 @@ proc push_to {remote} {
 
 proc start_push_anywhere_action {w} {
 	global push_urltype push_remote push_url push_thin push_tags
+	global gerrit_review gerrit_branch
 	global push_force
 	global repo_config
 
@@ -95,7 +96,15 @@ proc start_push_anywhere_action {w} {
 		set cnt 0
 		foreach i [$w.source.l curselection] {
 			set b [$w.source.l get $i]
-			lappend cmd "refs/heads/$b:refs/heads/$b"
+			if {$gerrit_review && $gerrit_branch ne {}} {
+				if {$gerrit_branch eq $b} {
+					lappend cmd "refs/heads/$b:refs/for/$b"
+				} else {
+					lappend cmd "refs/heads/$b:refs/for/$gerrit_branch/$b"
+				}
+			} else {
+				lappend cmd "refs/heads/$b:refs/heads/$b"
+			}
 			incr cnt
 		}
 		if {$cnt == 0} {
@@ -120,6 +129,7 @@ trace add variable push_remote write \
 proc do_push_anywhere {} {
 	global all_remotes current_branch
 	global push_urltype push_remote push_url push_thin push_tags
+	global gerrit_review gerrit_branch
 	global push_force use_ttk NS
 
 	set w .push_setup
@@ -215,6 +225,24 @@ proc do_push_anywhere {} {
 		-text [mc "Include tags"] \
 		-variable push_tags
 	grid $w.options.tags -columnspan 2 -sticky w
+	${NS}::checkbutton $w.options.gerrit \
+		-text [mc "Gerrit review.  Branch: "] \
+		-variable gerrit_review
+	${NS}::entry $w.options.gerrit_br \
+		-width 50 \
+		-textvariable gerrit_branch \
+		-validate key \
+		-validatecommand {
+			if {%d == 1 && [regexp {\s} %S]} {return 0}
+			if {%d == 1 && [string length %S] > 0} {
+				set gerrit_review 1
+			}
+			if {[string length %P] == 0} {
+				set gerrit_review 0
+			}
+			return 1
+		}
+	grid $w.options.gerrit $w.options.gerrit_br -sticky we -padx {0 5}
 	grid columnconfigure $w.options 1 -weight 1
 	pack $w.options -anchor nw -fill x -pady 5 -padx 5
 
-- 
1.7.9.5

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

[Phil Hord]

On Fri, Sep 6, 2013 at 6:30 AM, Joergen Edelbo <jed@napatech.com> wrote:
> Problem: It is not possible to push for Gerrit review as you will
> always try to push to "refs/heads/..." on the remote.
>
> Changes done:
>
> Add an option to select "Gerrit review" and a corresponding entry
> for a branch name. If this option is selected, push the changes to
> "refs/for/<gerrit-branch>/<local branch>". In this way the local branch
> names will be used as topic branches on Gerrit.


In my gerrit repos, I have this configuration

  $  git config remote.origin.push HEAD:refs/for/master

And so I can simply 'git push' and git does what I mean.

My main complaint with git-gui's push is that it ignores my
configuration. Can you teach git-gui to honor this setting, instead?

I would like for this remote."name".push option to be smarter and figure
out which $branch I mean when I am not on master, but that is a different
discussion. Having said that, I see that your change to git-gui attempts to
make that automatic.  Kudos for that, but I don't think this will work for
me as I am often on a detached branch and so "refs/heads/$b" is meaningless.

Can you think of a sane way to separate the "from" and the "to" branches in
the GUI?  I mean, I would like to push "HEAD" and I would like it to
go to "refs/for/frotz-2.0".

Phil

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

[Jørgen Edelbo]

On 06-09-2013 23:49, Phil Hord wrote:
> Can you think of a sane way to separate the "from" and the "to" branches in
> the GUI?  I mean, I would like to push "HEAD" and I would like it to
> go to "refs/for/frotz-2.0".

My first attemt at this change was to do do exactly that: always push 
HEAD, and being able to specify the destination branches. If it was a 
Gerrit review server, it would replace refs/heads/... with refs/for/...

However, it is now clear, that we have to support specifying the 
branches to push.

My next thought was: could we add an entry to the list of branches to 
push, meaning "omit the branch specs in the command". Then it should 
just perform a "git push <remote>". What was then pushed would depend
on the push configuration for the remote. In the Gerrit case we could 
then have:
   push = HEAD:refs/for/master, or whatever.

The drawback of this solution is that the UI is not so intuitive and it 
would not support the case where you would want to push your branch 
frotz to refs/for/master/frotz, which is the way you specify a topic in 
Gerrit. The current solution supports this.

What remains to support is the case where you work on a detached HEAD. 
Most of it should be straight forward. The gui knows we are in detached 
state, so in this case, a "HEAD" entry could be added to the list of 
branches. The question is just: to which branch to push to in the non 
Gerrit case? Perhaps the ref specs could just be left out in this case.

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

[Joergen Edelbo]

Problem: It is not possible to push for Gerrit review as you will
always try to push to "refs/heads/..." on the remote.

Changes done:

Add an option in the Push dialog to select "Gerrit review" and a
corresponding entry for a branch name. If this option is selected,
push the changes to "refs/for/<gerrit-branch>/<local branch>". In
this way the local branch names will be used as topic branches on
Gerrit.

If you are on a detached HEAD, then add a "HEAD" entry in the branch
selection list. If this is selected, push HEAD:HEAD in the normal
case and HEAD:refs/for/<gerrit_branch> in the Gerrit case.

The Gerrit branch to push to is controlled by gerrit.reviewbranch
configuration option.
---
Hi again,

Seems like this discussion has died out. Is there no perspective in
changing git-gui to support Gerrit better?

Anyway here is what I consider my final shot at a solution. Compared
to the last one, this commit can handle the case when you work on a 
detached HEAD, and the Gerrit branch to push to is handled by a
configuration option.

BR Jørgen Edelbo

 git-gui.sh        |    1 +
 lib/option.tcl    |    1 +
 lib/transport.tcl |   48 +++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index b62ae4a..3228654 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -901,6 +901,7 @@ set default_config(gui.diffcontext) 5
 set default_config(gui.diffopts) {}
 set default_config(gui.commitmsgwidth) 75
 set default_config(gui.newbranchtemplate) {}
+set default_config(gerrit.reviewbranch) {}
 set default_config(gui.spellingdictionary) {}
 set default_config(gui.fontui) [font configure font_ui]
 set default_config(gui.fontdiff) [font configure font_diff]
diff --git a/lib/option.tcl b/lib/option.tcl
index 23c9ae7..ffa4226 100644
--- a/lib/option.tcl
+++ b/lib/option.tcl
@@ -157,6 +157,7 @@ proc do_options {} {
 		{t gui.diffopts {mc "Additional Diff Parameters"}}
 		{i-0..99 gui.commitmsgwidth {mc "Commit Message Text Width"}}
 		{t gui.newbranchtemplate {mc "New Branch Name Template"}}
+		{t gerrit.reviewbranch {mc "Gerrit Review Branch"}}
 		{c gui.encoding {mc "Default File Contents Encoding"}}
 		{b gui.warndetachedcommit {mc "Warn before committing to a detached head"}}
 		{s gui.stageuntracked {mc "Staging of untracked files"} {list "yes" "no" "ask"}}
diff --git a/lib/transport.tcl b/lib/transport.tcl
index e5d211e..9b1cfc5 100644
--- a/lib/transport.tcl
+++ b/lib/transport.tcl
@@ -61,6 +61,7 @@ proc push_to {remote} {
 
 proc start_push_anywhere_action {w} {
 	global push_urltype push_remote push_url push_thin push_tags
+	global gerrit_review gerrit_branch
 	global push_force
 	global repo_config
 
@@ -95,7 +96,19 @@ proc start_push_anywhere_action {w} {
 		set cnt 0
 		foreach i [$w.source.l curselection] {
 			set b [$w.source.l get $i]
-			lappend cmd "refs/heads/$b:refs/heads/$b"
+			if {$gerrit_review && $gerrit_branch ne {}} {
+				switch $b {
+				$gerrit_branch	{ lappend cmd "refs/heads/$b:refs/for/$gerrit_branch" }
+				HEAD		{ lappend cmd "HEAD:refs/for/$gerrit_branch" }
+				default		{ lappend cmd "refs/heads/$b:refs/for/$gerrit_branch/$b" }
+				}
+			} else {
+				if {$b eq HEAD} {
+					lappend cmd "HEAD:HEAD"
+				} else {
+					lappend cmd "refs/heads/$b:refs/heads/$b"
+				}
+			}
 			incr cnt
 		}
 		if {$cnt == 0} {
@@ -118,9 +131,10 @@ trace add variable push_remote write \
 	[list radio_selector push_urltype remote]
 
 proc do_push_anywhere {} {
-	global all_remotes current_branch
+	global all_remotes current_branch is_detached
 	global push_urltype push_remote push_url push_thin push_tags
-	global push_force use_ttk NS
+	global gerrit_review gerrit_branch
+	global push_force use_ttk NS repo_config
 
 	set w .push_setup
 	toplevel $w
@@ -149,6 +163,11 @@ proc do_push_anywhere {} {
 		-height 10 \
 		-width 70 \
 		-selectmode extended
+	if {$is_detached} {
+		$w.source.l insert end HEAD
+		$w.source.l select set end
+		$w.source.l yview end
+	}
 	foreach h [load_all_heads] {
 		$w.source.l insert end $h
 		if {$h eq $current_branch} {
@@ -215,13 +234,36 @@ proc do_push_anywhere {} {
 		-text [mc "Include tags"] \
 		-variable push_tags
 	grid $w.options.tags -columnspan 2 -sticky w
+	${NS}::checkbutton $w.options.gerrit \
+		-text [mc "Gerrit review.  Branch: "] \
+		-variable gerrit_review
+	${NS}::entry $w.options.gerrit_br \
+		-width 50 \
+		-textvariable gerrit_branch \
+		-validate key \
+		-validatecommand {
+			if {%d == 1 && [regexp {\s} %S]} {return 0}
+			if {%d == 1 && [string length %S] > 0} {
+				set gerrit_review 1
+			}
+			if {[string length %P] == 0} {
+				set gerrit_review 0
+			}
+			return 1
+		}
+	grid $w.options.gerrit $w.options.gerrit_br -sticky we -padx {0 5}
 	grid columnconfigure $w.options 1 -weight 1
 	pack $w.options -anchor nw -fill x -pady 5 -padx 5
 
+	if ![info exists gerrit_branch] { 
+		set gerrit_branch {}
+		catch {set gerrit_branch $repo_config(gerrit.reviewbranch)}
+	}
 	set push_url {}
 	set push_force 0
 	set push_thin 0
 	set push_tags 0
+	set gerrit_review [expr {$gerrit_branch ne {}}]
 
 	bind $w <Visibility> "grab $w; focus $w.buttons.create"
 	bind $w <Key-Escape> "destroy $w"
-- 
1.7.9.5

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

[Joergen Edelbo]

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 <jed@napatech.com>
---
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

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

[Heiko Voigt]

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 <jed@napatech.com>
> ---
> 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

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

[Johannes Sixt]

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

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

[Jørgen Edelbo]

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

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

[Johannes Sixt]

Please do not top-post.

Am 9/5/2013 10:29, schrieb Jørgen Edelbo:
> -----Original Message----- From: Johannes Sixt
>> 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?
> 
> 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

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

[Jørgen Edelbo]

> -----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:
> > -----Original Message----- From: Johannes Sixt
> >> 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?
> >
> > 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

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

[Johannes Sixt]

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

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

[Junio C Hamano]

Jørgen Edelbo <jed@napatech.com> writes:

>> 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.

People work in very different ways, and mine and yours are extreme
opposites.  I almost never push out a commit that is just off the
press, I use topic branches heavily and work on multiple topics
(either related or unrelated) in parallel all the time, so it is
very usual for me to push out more than one branches with a single
push---by definition, if we support pushing only the current branch
out, "working on more than one topics and after perfecting these,
push them together" cannot be done.

If one sets push.default to something other than the matching, and
does not have any remote.*.push refspec in the configuration, J6t's
"I forgot to push while I was on that branch" and my "I deliberately
did not push those branches out while I was on them, but now I know
I am ready to push them out" cannot be done without explicit
refspecs on the command line.

But stepping back a bit, I think this "I commit because I want to
get it out the door" is coming from the same desire that led to a
possible design mistake that made various push.default settings push
only the current branch out.

The possible design mistake that has been disturbing me is the
following.

The "push.default" setting controls two unrelated things, and that
is one too many.  Between the matching modes and the rest, it tells
what branch(es) are pushed out (either "all the branches with the
same name" or "the current branch").  That is one thing, and I tend
to agree that "push only the current branch" would be a sane default
many people would prefer (and that is the reason we made it the
default for Git 2.0).

Among the various non matching modes, it also determines where a
branch that is pushed out would go ("current" pushes to the same
name, "upstream" pushes to @{upstream}, etc.).  But this "once I
choose what branch to push out, the branch being pushed out knows
where it wants to go", does not take effect if one explicitly names
what to push, e.g. in this sequence where one forgets to push
'frotz' out:

    j6t$ git checkout frotz
    ... work work work ...
    j6t$ git checkout xyzzy
    ... work work work ...
    ... realize 'frotz' is done
    j6t$ git push origin frotz

The last push may work differently from the push in this sequence:

    j6t$ git checkout frotz
    ... work work work ...
    j6t$ git push ;# or "git push origin"

In the latter sequence, the niceties of push.upstream to push
'frotz' out to the frotz@{upstream} (and your "git push origin
refs/heads/frotz:refs/for/frotz" mapping done in git-gui) will take
effect, but in the former, the refspec "frotz" on the command line
is taken as a short-hand for "frotz:frotz".

We may want to teach "git push" that the command line refspec that
names a local branch may not be pushing to the same name but wants
to go through the same "mapping" used when "git push" is done while
the branch is checked out, with some mechanism.

It is a separate but very related topic, I think.

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

[Jørgen Edelbo]

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

> Isn't the right way to improve the situation to let the command line tools
> know how the user wants to push things out and just have Git-GUI delegate
> the choice to the underlying "git push"?

Thank you for all the constructive feedback. I realize that it is not the way forward to remove the selection of branches to push.

What I consider now, is to pursue the idea that Junio presents above: just let the gui tool use whatever is configured for the selected remote. I have, however not been able to come up with a solution that looks nice. What I have been trying now is the following little modification:

diff --git a/git-gui/lib/transport.tcl b/git-gui/lib/transport.tcl
index e5d211e..1709464 100644
--- a/git-gui/lib/transport.tcl
+++ b/git-gui/lib/transport.tcl
@@ -95,7 +95,9 @@ proc start_push_anywhere_action {w} {
                set cnt 0
                foreach i [$w.source.l curselection] {
                        set b [$w.source.l get $i]
-                       lappend cmd "refs/heads/$b:refs/heads/$b"
+                       if {$b nw {<default>}}{
+                               lappend cmd "refs/heads/$b:refs/heads/$b"
+                       }
                        incr cnt
                }
                if {$cnt == 0} {
@@ -149,6 +151,7 @@ proc do_push_anywhere {} {
                -height 10 \
                -width 70 \
                -selectmode extended
+       $w.source.l insert end <default>
        foreach h [load_all_heads] {
                $w.source.l insert end $h
                if {$h eq $current_branch} {

The idea is to insert a "<default>" entry in the branch selection list, and then skip that when building the command. Then you can avoid having refs on the command line and just let git act according to configuration.

How about that? Any smarter way to do it?

BR Jørgen

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

[Junio C Hamano]

Joergen Edelbo <jed@napatech.com> writes:

> +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"
>  	}
> +}

I am puzzled.  This may be fine for those who use Git-GUI and
nothing else to push, but will not help whose who use both Git-GUI
and the command line.

Isn't the right way to improve the situation to let the command line
tools know how the user wants to push things out and just have
Git-GUI delegate the choice to the underlying "git push"?

For example, if you are working on a topic 'frotz', and if the
location you push is managed by Gerrit, isn't it the case that you
always want to push it to "refs/for/frotz", whether you are pushing
via Git-GUI or from the command line?

I think we discussed during 1.8.4 cycle a configuration like this:

	[branch "frotz"]
        	push = refs/heads/frotz:refs/for/frotz

as part of the "triangular workflow" topic that allows you to
specify that "when 'frotz' is pushed out, it goes to
refs/for/frotz", or something like that, but I do not recall what
came out of that.