Re: [PATCH v1 02/11] git-gui: refactor browser / blame argument parsing

[Johannes Sixt <j6t@kdbg.org>]

Am 14.05.26 um 16:33 schrieb Mark Levedahl:
> git-gui has subcommands blame and browser, both of which accept a
> pathname, possibly preceded by a commit-ish item to specify a revision.
> Also, blame can take a first argument that gives a line number to focus.
> 
> The command line parser for the above is more complex than needed, and
> cannot work without a worktree as the pathname objects are checked
> against the current worktree for existence. This also precludes naming a
> directory or file that does not exist on the currently checked out
> branch.

While the old browser isn't simple, it implements the strategy "revs
before paths, no revs after the first path or '--'" that is applied by
every git command. The rewritten parser is only slightly simpler. It
should not ignore "--". Furthermore, the old parser ignored excessive
trailing arguments, while the new parser ignores excessive leading
arguments. Neither is desirable, and we should report an incorrect
argument list (if we don't, then I prefer the old behavior).

> 
> So, replace this with a simpler parser that looks at argument number and
> number of arguments to know what value to expect. The blame and browser
> backends already have error checking with diagnostic information, so
> defer most error checking to those. Also, allow a line-number selection
> to be given and silently ignored for the browser, further simplifying
> this code.

The line number selection isn't ignored by the browser, but reported as
an incorrect usage before and after this patch.

> 
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
>  git-gui.sh | 66 +++++++++++++-----------------------------------------
>  1 file changed, 16 insertions(+), 50 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 6048f92..a951fcd 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -2986,51 +2986,34 @@ blame {
>  	set head {}
>  	set path {}
>  	set jump_spec {}
> -	set is_path 0
> +	set nargs [llength $argv]
> +	if {$nargs < 1} {
> +		usage
> +	}
> +	set argn 0
>  	foreach a $argv {
> -		set p [file join $_prefix $a]
> +		set argn [expr {$argn + 1}]
>  
> -		if {$is_path || [file exists $p]} {
> -			if {$path ne {}} usage
> -			set path [normalize_relpath $p]
> -			break
> -		} elseif {$a eq {--}} {
> -			if {$path ne {}} {
> -				if {$head ne {}} usage
> -				set head $path
> -				set path {}
> +		if {$argn < $nargs} {
> +			# revision or line number
> +			if {[regexp {^--line=(\d+)$} $a a lnum]} {
> +				set jump_spec [list $lnum]
> +			} else {
> +				set head $a
>  			}
> -			set is_path 1
> -		} elseif {[regexp {^--line=(\d+)$} $a a lnum]} {
> -			if {$jump_spec ne {} || $head ne {}} 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 {}
>  		} else {
> -			set path [normalize_relpath $_prefix$head]
> -			set head {}
> +			set path [normalize_relpath $a]

This loses the capability to request a browser relative to a
subdirectory of the working tree. For example, "git gui browser main ."
now shows the top-level directory instead of the subdirectory when
invoked from a subdirectory.

>  		}
>  	}
>  
>  	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]} {
> +				set head [git rev-parse --verify $head]
> +			} err]} {

Please leave the indentation unchanged.

>  				if {[tk windowingsystem] eq "win32"} {
>  					tk_messageBox -icon error -title [mc Error] -message $err
>  				} else {
> @@ -3046,26 +3029,9 @@ blame {
>  	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
>  	}
>  	}

The check for the existence of files is actually necessary to
disambiguate the meaning of the argument. If a file "maint" exists, then
the argument is to be interpreted as path, not as the ref "maint", even
if that exists, too.

I suggest to protect the "file exists" calls with ($_gitworktree ne {}
&& ...) or (![is_bare] && ...) to handle being invoked from a bare
repository. That is, in a bare repository we treat arguments the same as
files that do not exist in the currently checked-out branch.

-- Hannes