[PATCH v2 00/11] Improve git gui operation without a worktree

[Mark Levedahl <mlevedahl@gmail.com>]

git gui has a number of inter-related problems that result in problems
during startup from anything but a checked out worktree pointing at a
valid git repository. Some of the symptoms are:
- blame / browser subcommands, and launching gitk, are intended to be
  useful without a worktree, but fail to work.
- unlike git, git-gui is supposed to use the parent directory as a
  worktree if started from the .git subdirectory in the very common
  single worktree + embedded git repository format. This does not
  work.
- git-gui includes a repository picker allowing a user to select a
  worktree from a list and/or start a new repo+worktree: this dialog can
  appear at unexpected times, masking useful error feedback on
  configuration problems.

This patch series addresses the above issues, substantially rewriting
the initial repository/worktree process to rely upon git rev-parse so
that git's knowledge of access rules, repository configuration, and use
of GIT_DIR / GIT_WORK_TREE (or git --gitdir / --work-tree) is used
throughout, replacing code largely based upon what git did in 2008. This
also means that git gui will naturally gain any new rules implmented in
git-core.

With this, git-gui only exports GIT_WORK_TREE when non-empty.
GIT_WORK_TREE is needed, and must be exported, if the user is overriding
core.worktree in the git repository. But, GIT_WORK_TREE cannot be used
to specify the lack of a worktree, so exporting an empty GIT_WORK_TREE
is one of the problems fixed by this series.

v2 of this series is a very substantial rewrite driven by j6t's review,
with patches reoranized and squashed, interfaces to the repository
chooser changed, a different code structure to allow user control of the
repository picker, a different approach to fixing the command line
parser for blame / browser, and other more minor changes. Patches
for fixing blame / browser are now after all discovery refactoring as
they cannot be tested without some of those fixes.

Many subtle things are fixed beyond the list at the top, including
better compatibility with git blame and repeatable browser / blame
operation for specific revs not in the worktree, regardless of the
worktree state. j6t indicated that in the git-gui project, the following
fails in the current release:

cd lib
GIT_DIR=$PWD/../.git GIT_WORK_TREE=$PWD/.. ../git-gui.sh browser origin/master .

This is due to a _prefix issue, and is fixed as of the patch
     git-gui: use git rev-parse for worktree discovery

Mark Levedahl (11):
  git-gui: guard set/unset of GIT_DIR and GIT_WORK_TREE
  git-gui: return status from choose_repository::pick
  git-gui: use --absolute-git-dir
  git-gui: use rev-parse exclusively to find a repository
  git-gui: simplify [is_bare] to report if a worktree is known
  git-gui: use git rev-parse for worktree discovery
  git-gui: try harder to find worktree from gitdir
  git-gui: use HEAD as current branch when detached (bug fix)
  git-gui: allow specifying path '.' to the browser
  git-gui: adapt blame/browser parsing for bare operation
  git-gui: add gui and pick as explicit subcommands

 git-gui.sh                | 412 +++++++++++++++++++++++---------------
 lib/choose_repository.tcl |  21 +-
 2 files changed, 257 insertions(+), 176 deletions(-)

Interdiff against v1:
diff --git a/git-gui.sh b/git-gui.sh
index c56aeeff88..299c1a0292 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -648,6 +648,9 @@ proc load_current_branch {} {
 
 	set current_branch [git branch --show-current]
 	set is_detached [expr [string length $current_branch] == 0]
+	if {$is_detached} {
+		set current_branch {HEAD}
+	}
 }
 
 auto_load tk_optionMenu
@@ -1021,7 +1024,8 @@ proc load_config {include_global} {
 ##
 ## feature option selection
 
-set run_picker_on_error 1
+enable_option picker
+enable_option gitdir_discovery
 if {[regexp {^git-(.+)$} [file tail $argv0] _junk subcommand]} {
 	unset _junk
 } else {
@@ -1031,9 +1035,11 @@ if {$subcommand eq {gui.sh}} {
 	set subcommand gui
 }
 if {$subcommand eq {gui} && [llength $argv] > 0} {
-	set run_picker_on_error 0
 	set subcommand [lindex $argv 0]
 	set argv [lrange $argv 1 end]
+	if {$subcommand eq {gui}} {
+		disable_option picker
+	}
 }
 
 enable_option multicommit
@@ -1049,7 +1055,7 @@ blame {
 	disable_option multicommit
 	disable_option branch
 	disable_option transport
-	set run_picker_on_error 0
+	disable_option picker
 }
 citool {
 	enable_option singlecommit
@@ -1058,7 +1064,7 @@ citool {
 	disable_option multicommit
 	disable_option branch
 	disable_option transport
-	set run_picker_on_error 0
+	disable_option picker
 
 	while {[llength $argv] > 0} {
 		set a [lindex $argv 0]
@@ -1081,6 +1087,9 @@ citool {
 		set argv [lrange $argv 1 end]
 	}
 }
+pick {
+	disable_option gitdir_discovery
+}
 }
 
 ######################################################################
@@ -1104,21 +1113,39 @@ unset argv0dir
 ##
 ## repository setup
 
-proc is_parent_worktree {} {
-	# Directory 'parent' of a repository named 'parent/.git' might be the worktree
-	set ok 0
+proc find_worktree_from_gitdir {} {
+	# Directory 'parent' of a repository named 'parent/.git' might be the worktree.
+	# Assure parent is a worktree and using the git repository already discovered.
+	# Also, handle case of being in a worktree's gitdir, where file "gitdir" points to
+	# gitlink file .git in the real worktree.
+	set worktree {}
 	if {[file tail $::_gitdir] eq {.git}} {
-		set gitdir_parent [file join $::_gitdir {..}]
-		set expected_worktree [file normalize $gitdir_parent]
-		catch {set git_worktree [git -C $gitdir_parent rev-parse --show-toplevel]}
-		if {[string compare $expected_worktree $git_worktree] == 0} {
-			set ::_prefix {}
-			set ::_gitworktree $git_worktree
-			cd $git_worktree
-			set ok 1
+		if {[catch {
+			set gitdir_parent [file dirname $::_gitdir]
+			set worktree [git -C $gitdir_parent rev-parse --show-toplevel]
+			set parent_gitdir [git -C $worktree rev-parse --absolute-git-dir]
+			if {$::_gitdir ne $parent_gitdir} {
+				set worktree {}
+			}
+		}]} {
+			set worktree {}
+		}
+	} elseif [file exists {gitdir}] {
+		if {[catch {
+			set fd_gitdir [open {gitdir} {r}]
+			set gitlink_parent [file dirname [read $fd_gitdir]]
+			catch {close $fd_gitdir}
+			set worktree [git -C $gitlink_parent rev-parse --show-toplevel]
+			set parent_gitdir [git -C $worktree rev-parse --absolute-git-dir]
+			if {$::_gitdir ne $parent_gitdir} {
+				set worktree {}
+			}
+		}]} {
+			catch {close $fd_gitdir}
+			set worktree {}
 		}
 	}
-	return $ok
+	return $worktree
 }
 
 proc is_gitvars_error {err} {
@@ -1155,62 +1182,76 @@ proc unset_gitdir_vars {} {
 	catch {unset env(GIT_WORK_TREE)}
 }
 
+# find repository.
+set _gitdir {}
+if {[is_enabled gitdir_discovery]} {
+	if {[catch {
+		set _gitdir [git rev-parse --absolute-git-dir]
+	} err]} {
+		if {[is_gitvars_error $err]} {
+			exit 1
+		}
+		set _gitdir {}
+	}
+}
+
 set picked 0
-proc pick_repo {} {
+if {$_gitdir eq {} && [is_enabled picker]} {
 	unset_gitdir_vars
 	load_config 1
 	apply_config
-	choose_repository::pick
-	set _gitdir [git rev-parse --absolute-git-dir]
-	set _prefix {}
+	if {![choose_repository::pick]} {
+		exit 1
+	}
+	if {[catch {
+		set _gitdir [git rev-parse --absolute-git-dir]
+	} err]} {
+		catch {wm withdraw .}
+		error_popup [strcat [mc "Unusable repo/worktree:"] " [pwd] "\n\n$err"]
+	}
 	set picked 1
 }
 
-# run repository picker if explicitly requested
-switch -- $subcommand {
-	pick {
-		pick_repo
-		set subcommand gui
-		set run_picker_on_error 0
-	}
-}
-
-# find repository.
-if {[catch {
-	set _gitdir [git rev-parse --absolute-git-dir]
-} err]} {
-	if {[is_gitvars_error $err]} {
-		exit 1
-	}
-	if {$run_picker_on_error} {
-		pick_repo
-	} else {
-		catch {wm withdraw .}
-		error_popup [strcat [mc "Git directory not found:"] "\n\n$err"]
-		exit 1
-	}
+if {$_gitdir eq {}} {
+	catch {wm withdraw .}
+	error_popup [strcat [mc "Git directory not found:"] "\n\n$err"]
+	exit 1
 }
 
 # find worktree, continue without if not required
 if {[catch {
 	set _gitworktree [git rev-parse --show-toplevel]
 	set _prefix [git rev-parse --show-prefix]
-	cd $_gitworktree
 } err]} {
 	if {[is_gitvars_error $err]} {
 		exit 1
 	}
 	set _gitworktree {}
 	set _prefix {}
-	if {[is_enabled bare]} {
-		cd $_gitdir
-	} elseif {![is_parent_worktree]} {
-		catch {wm withdraw .}
-		error_popup [strcat [mc "Cannot use bare repository:"] "\n\n" $_gitdir]
-		exit 1
+}
+
+if {[is_bare]} {
+	# Maybe we are in an embedded or worktree specific gitdir
+	if {[set _gitworktree [find_worktree_from_gitdir]] ne {}} {
+		set _prefix {}
 	}
 }
 
+if {![is_bare]} {
+	if {[catch {
+		cd $_gitworktree
+	} err]} {
+		catch {wm withdraw .}
+		error_popup [strcat [mc "Cannot change to discovered worktree: "] \
+			"$_gitworktree" "\n\n$err"]
+		exit 1;
+	}
+} elseif {![is_enabled bare]} {
+	catch {wm withdraw .}
+	error_popup [strcat [mc "Cannot use bare repository:"] "\n\n" $_gitdir]
+	exit 1
+}
+
 # repository and worktree config are complete, export them
 set_gitdir_vars
 
@@ -1229,8 +1270,6 @@ if {$hashalgorithm eq "sha1"} {
 load_config 0
 apply_config
 
-
-# Derive a human-readable repository name
 set _reponame [file split [file normalize $_gitdir]]
 if {[lindex $_reponame end] eq {.git}} {
 	set _reponame [lindex $_reponame end-1]
@@ -2035,7 +2074,7 @@ proc incr_font_size {font {amt 1}} {
 
 proc do_gitk {revs {is_submodule false}} {
 	global current_diff_path file_states current_diff_side ui_index
-	global _gitdir _gitworktree
+	global _gitworktree
 
 	# -- Always start gitk through whatever we were loaded with.  This
 	#    lets us bypass using shell process on Windows systems.
@@ -2045,8 +2084,6 @@ proc do_gitk {revs {is_submodule false}} {
 	if {$exe eq {}} {
 		error_popup [mc "Couldn't find gitk in PATH"]
 	} else {
-		global env
-
 		set pwd [pwd]
 
 		if {!$is_submodule} {
@@ -2105,9 +2142,6 @@ proc do_git_gui {} {
 	if {$exe eq {}} {
 		error_popup [mc "Couldn't find git gui in PATH"]
 	} else {
-		global env
-		global _gitdir _gitworktree
-
 		# see note in do_gitk about unsetting these vars when
 		# running tools in a submodule
 		unset_gitdir_vars
@@ -2992,77 +3026,135 @@ proc normalize_relpath {path} {
 	if {$elements ne {}} {
 		return [eval file join $elements]
 	} else {
-		return {}
+		return {./}
 	}
 }
 
+proc find_path_type {head path} {
+	if {$path eq {./}} {
+		# the root-tree exists in every rev, ls-tree gives data on the contents,
+		# not the type of tree itself. So, if the rev exists, return {tree}
+		if {[catch {set objtype [git ls-tree $head]}]} {
+			set objtype {}
+		} else {
+			set objtype {tree}
+		}
+	} else {
+		# test that the path exists in head, ls-tree gives info on the path only
+		if {[catch {set objtype [git ls-tree {--format=%(objecttype)} $head $path]}]} {
+			set objtype {}
+		}
+	}
+	return $objtype
+}
+
 # -- Not a normal commit type invocation?  Do that instead!
 #
 switch -- $subcommand {
 browser -
 blame {
 	if {$subcommand eq "blame"} {
-		set subcommand_args {[--line=<num>] rev? path}
+		set subcommand_args {[--line=<num>] <[rev] [--] filename | [--] filename rev>}
+		set required_objtype blob
 	} else {
-		set subcommand_args {rev? path}
+		set subcommand_args {<[rev] [--] directory | [--] directory rev>}
+		set required_objtype tree
 	}
-	if {$argv eq {}} usage
-	set head {}
-	set path {}
-	set jump_spec {}
+	set maxargs [llength $subcommand_args]
 	set nargs [llength $argv]
-	if {$nargs < 1} {
-		usage
-	}
-	set argn 0
-	foreach a $argv {
-		set argn [expr {$argn + 1}]
+	if {$nargs < 1 || $nargs > $maxargs} usage
+	set head {}
+	set althead {}
+	set path {}
+	set altpath {}
+	set canswap 1
+	set jump_spec {}
 
-		if {$argn < $nargs} {
-			# revision or line number
-			if {[regexp {^--line=(\d+)$} $a a lnum]} {
-				set jump_spec [list $lnum]
+	# assume: [--line=num] [head] [--] path as the possible arguments, in order.
+	# head and path may need a swap later.
+	for {set iarg 0} {$iarg < $nargs} {incr iarg} {
+		set arg [lindex $argv $iarg]
+		if {$arg eq {--}} {
+			# next arg is the path, prevent or FORCE swap?
+			if {$iarg == $nargs - 2} {
+				set canswap 0
+			} elseif {$iarg == $nargs - 3} {
+				set canswap 2
 			} else {
-				set head $a
+				usage
 			}
+		} elseif {[regexp {^--line=(\d+)$} $arg arg lnum]} {
+			# --line can only be the first arg
+			if {$iarg != 0 || $maxargs < 4} usage
+			set jump_spec [list $lnum]
+		} elseif {$iarg == $nargs - 1} {
+			# assume final argument is path
+			set path [normalize_relpath [file join $_prefix $arg]]
+			set althead $arg
+		} elseif {$head eq {}} {
+			# assume the other argument is head
+			set head $arg
+			set altpath [normalize_relpath [file join $_prefix $arg]]
 		} else {
-			set path [normalize_relpath $a]
+			usage
 		}
 	}
 
+	# no swapping allowed if head not given, use current branch (HEAD)
 	if {$head eq {}} {
 		load_current_branch
 		set head $current_branch
-	} else {
-		if {[regexp [string map "@@ [expr $hashlength - 1]" {^[0-9a-f]{1,@@}$}] $head]} {
-			if {[catch {
-				set head [git rev-parse --verify $head]
-			} err]} {
-				if {[tk windowingsystem] eq "win32"} {
-					tk_messageBox -icon error -title [mc Error] -message $err
-				} else {
-					puts stderr $err
-				}
-				exit 1
-			}
+		set canswap 0
+	}
+
+	# -- before "rev" arg means we got -- path head
+	if {$canswap == 2} {
+		set head $althead
+		set path $altpath
+		set canswap 0
+	}
+
+	set objtype [find_path_type $head $path]
+	if {$objtype eq {} && $canswap} {
+		set objtype [find_path_type $althead $altpath]
+		if {$objtype ne {}} {
+			set head $althead
+			set path $altpath
 		}
-		set current_branch $head
+	}
+	set current_branch $head
+
+	# check that path exists in head, and objtype matches need
+	if {$objtype ne $required_objtype} {
+		switch -- $required_objtype {
+			tree {set err [strcat \
+				[mc "'%s' is not a directory in rev '%s'" $path $head]]}
+			blob {set err [strcat \
+				[mc "'%s' is not a filename in rev '%s'" $path $head]]}
+		}
+		if {[tk windowingsystem] eq "win32"} {
+			catch {wm withdraw .}
+			error_popup $err
+		} else {
+			puts stderr $err
+		}
+		exit 1
 	}
 
 	wm deiconify .
 	switch -- $subcommand {
 	browser {
-		if {$jump_spec ne {}} usage
 		browser::new $head $path
 	}
-	blame   {
+	blame {
 		blame::new $head $path $jump_spec
 	}
 	}
 	return
 }
 citool -
-gui {
+gui -
+pick {
 	if {[llength $argv] != 0} {
 		usage
 	}
diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index 7e1462a20c..4b06afee93 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -15,7 +15,7 @@ field w_recentlist ; # Listbox containing recent repositories
 field w_localpath  ; # Entry widget bound to local_path
 
 field done              0 ; # Finished picking the repository?
-field clone_ok      false ; # clone succeeeded
+field pick_ok           0 ; # true if repo pick/clone succeeded
 field local_path       {} ; # Where this repository is locally
 field origin_url       {} ; # Where we are cloning from
 field origin_name  origin ; # What we shall call 'origin'
@@ -220,6 +220,8 @@ constructor pick {} {
 	if {$top eq {.}} {
 		eval destroy [winfo children $top]
 	}
+
+	return $pick_ok
 }
 
 method _center {} {
@@ -327,8 +329,7 @@ method _git_init {} {
 	}
 
 	_append_recentrepos [pwd]
-	set ::_gitdir .git
-	set ::_prefix {}
+	set pick_ok 1
 	return 1
 }
 
@@ -409,6 +410,7 @@ method _do_new2 {} {
 	if {![_git_init $this]} {
 		return
 	}
+	set pick_ok 1
 	set done 1
 }
 
@@ -621,7 +623,7 @@ method _do_clone2 {} {
 	}
 
 	tkwait variable @done
-	if {!$clone_ok} {
+	if {!$pick_ok} {
 		error_popup [mc "Clone failed."]
 		return
 	}
@@ -632,18 +634,12 @@ method _do_clone2_done {ok} {
 	if {$ok} {
 		if {[catch {
 			cd $local_path
-			set ::_gitdir .git
-			set ::_prefix {}
 			_append_recentrepos [pwd]
 		} err]} {
 			set ok 0
 		}
 	}
-	if {!$ok} {
-		set ::_gitdir {}
-		set ::_prefix {}
-	}
-	set clone_ok $ok
+	set pick_ok $ok
 	set done 1
 }
 
@@ -721,8 +717,7 @@ method _do_open2 {} {
 	}
 
 	_append_recentrepos [pwd]
-	set ::_gitdir $actualgit
-	set ::_prefix {}
+	set pick_ok 1
 	set done 1
 }
 
-- 
2.54.0.99.14