@matheusbsbs / matheus-bagliano / commits / 3fbef193c7

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>

Mark Levedahl committed May 31, 2026 at 19:02 UTC 3fbef193c7e49e8edb5a4421c465df67de944da6

1 file changed +63 -59

git-gui.sh

+63 -59

index 40a83487d6..c72d453586 100755 --- 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