git-gui: check browser/blame arguments carefully

git gui offers two related commands, browser and blame, that provide
graphical interfaces driven by git ls-tree and git blame. As such, the
arguments to git-gui need to satisfy those two git commands. But,
git-gui does not assure this leading to confusing or incorrect results.
For instance 'git browser <non-existent path>' shows a blank browser
window rather than error message.

Also, commit 3e45ee1ef2 ("git-gui: Smarter command line parsing for
browser, blame", 2007-05-08) implemented code to allow giving path
before rev on the command line, and unconditionally uses the worktree to
disambiguate. As a result, the following command run in a current
git-gui checkout of the master branch shows the master branch version of
blame.tcl, when none should be shown as that file does not exist in
gitgui-0.6.0.

  git gui blame lib/blame.tcl gitgui-0.6.0

This 'file before rev' feature in git-gui mirrors ideas considered when
git's user interface was very young, but no such feature is documented
for any git command.  Rather than try to fix an idea git itself
rejected, let's just remove this broken and hopefully unused feature.

git-gui browser|blame both accept 'rev' and 'path' as command line
arguments.  rev defaults to 'HEAD' if not given, while path must be
given. path names a directory tree to ls-tree or a file to blame. path
must exist in rev for ls-tree and for blame.  In addition git blame will
include uncommitted changes from the worktree file at 'path' if rev is
not given (thus defaulting to HEAD), but still requires that the file
exists in HEAD.

So, let's clean up the parser to check that the arguments are usable.
- give a full synopsis, including '--' that may be used to separate rev and
  path. (as path is the required final arg, -- gives no extra info)
- explicitly check the number of arguments
- use rev-parse to assure a user supplied rev is valid
- use ls-tree to assure that path exists in rev
- for blame only, with no rev given and a worktree existing, also assure
  that path points to a file in the worktree

With these changes, error messages are thrown by the parser if the path
or rev are not known: no blank or erroneous displays are created. Also,
this avoids accessing the worktree except in the specific use case
supported by blame / git-blame, meaning browser|blame now also work
without a worktree.

Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>

diff --git a/git-gui.sh b/git-gui.sh
index 40a83487d64a147f7a38cac4014f5e1bccd42d35..c72d453586c099270e57d758b023310bd1ed2cc0 100755 (executable)
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -2999,101 +2999,105 @@ proc normalize_relpath {path} {
        }
 }
 
+proc show_parse_err {err} {
+       if {[tk windowingsystem] eq "win32"} {
+               catch {wm withdraw .}
+               error_popup $err
+       } else {
+               puts stderr $err
+       }
+       exit 1
+}
+
 # -- 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>}
+               set required_pathtype blob
        } else {
-               set subcommand_args {rev? path}
+               set subcommand_args {[rev] [--] <dirname>}
+               set required_pathtype tree
        }
-       if {$argv eq {}} usage
+       set maxargs [llength $subcommand_args]
+       set nargs [llength $argv]
+       if {$nargs < 1 || $nargs > $maxargs} usage
        set head {}
        set path {}
        set jump_spec {}
-       set is_path 0
-       foreach a $argv {
-               set p [file join $_prefix $a]
 
-               if {$is_path || [file exists $p]} {
-                       if {$path ne {}} usage
-                       set path [normalize_relpath $p]
-                       break
+       set iarg 0
+       foreach a $argv {
+               incr iarg
+               if {$iarg == $nargs} {
+                       # final argument is path
+                       set path [normalize_relpath [file join $_prefix $a]]
                } elseif {$a eq {--}} {
-                       if {$path ne {}} {
-                               if {$head ne {}} usage
-                               set head $path
-                               set path {}
+                       # allow before required final arg that must be path
+                       if {$iarg != $nargs - 1} {
+                               usage
                        }
-                       set is_path 1
                } elseif {[regexp {^--line=(\d+)$} $a a lnum]} {
-                       if {$jump_spec ne {} || $head ne {}} usage
+                       # --line can only be the first arg
+                       if {$iarg != 1 || $subcommand ne {blame}} usage
                        set jump_spec [list $lnum]
                } elseif {$head eq {}} {
-                       if {$head ne {}} usage
                        set head $a
-                       set is_path 1
                } else {
                        usage
                }
        }
-       unset is_path
 
-       if {$head ne {} && $path eq {}} {
-               if {[string index $head 0] eq {/}} {
-                       set path [normalize_relpath $head]
-                       set head {}
+       # If head not given, use current branch (HEAD),
+       # and blame will use worktree if there is one.
+       set use_worktree 0
+       if {$head eq {}} {
+               load_current_branch
+               set head $current_branch
+               if {$subcommand eq {blame} && ![is_bare]} {
+                       if {![file isfile $path]} {
+                               show_parse_err [mc "fatal: no such file '%s' in worktree" $path]
+                       }
+                       set use_worktree 1
+               }
+       } else {
+               if {[catch {
+                               set commitid \
+                                       [git rev-parse --verify --end-of-options \
+                                       [strcat $head "^{commit}"]]
+                       }]} {
+                       show_parse_err [mc "fatal: '%s' is not a valid rev'" $head]
                } else {
-                       set path [normalize_relpath $_prefix$head]
-                       set head {}
+                       set current_branch $head
                }
        }
 
-       if {$head eq {}} {
-               load_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
-                       }
+       # check path is known in head, and is file / directory as required
+       set pathtype {}
+       catch {set pathtype [git ls-tree {--format=%(objecttype)} $head $path]}
+       if {$pathtype ne {} && $path eq {.}} {
+               # ls-tree gives contents of root-dir, we need root-dir itself
+               set pathtype {tree}
+       }
+
+       if {$pathtype ne $required_pathtype} {
+               switch -- $required_pathtype {
+                       tree {show_parse_err \
+                               [mc "'%s' is not a directory in rev '%s'" $path $head]}
+                       blob {show_parse_err \
+                               [mc "'%s' is not a filename in rev '%s'" $path $head]}
                }
-               set current_branch $head
        }
 
        wm deiconify .
        switch -- $subcommand {
        browser {
-               if {$jump_spec ne {}} usage
-               if {$head eq {}} {
-                       if {$path ne {} && [file isdirectory $path]} {
-                               set head $current_branch
-                       } else {
-                               set head $path
-                               set path {}
-                       }
-               }
                browser::new $head $path
        }
        blame   {
-               if {$head eq {} && ![file exists $path]} {
-                       catch {wm withdraw .}
-                       tk_messageBox \
-                               -icon error \
-                               -type ok \
-                               -title [mc "git-gui: fatal error"] \
-                               -message [mc "fatal: cannot stat path %s: No such file or directory" $path]
-                       exit 1
-               }
-               blame::new $head $path $jump_spec
+               blame::new [expr {$use_worktree ? {} : $head}] $path $jump_spec
        }
        }
        return