Re: [PATCH v1 11/11] git-gui: add gui and pick as explicit subcommands

[Mark Levedahl <mlevedahl@gmail.com>]

On 5/16/26 4:18 AM, Johannes Sixt wrote:
> Am 14.05.26 um 16:33 schrieb Mark Levedahl:
>> git-gui accepts subcommands blame | browser | citool, and assumes the
>> subcommand is 'gui' if none is actually given, But, git gui also has a
>> repository picker (choose_repository::pick) that can create a new
>> repository + worktree, or choose an existing one, switch to that, and
>> the run the gui. The user has no direct control over invoking the
>> picker, instead the picker is triggered by failure in the repository /
>> worktree discover process: this includes being started in a directory
>> not controlled by git, which is probably the intended use case.
>>
>> The picker can appear when the user has no intention of creating a new
>> worktree, and the user cannot use the picker to create a new worktree
>> inside another.
>>
>> So, add two new explicit subcommands:
>>     gui  - Run the gui if repository/worktree discovery succeeds, or die
>>            with an error message, but never run the picker.
>>     pick - First run the picker, regardless, then start the gui in
>>            the chosen worktree.
>>
>> Nothing in this changes the prior behavior, the alternates above must be
>> explicitly selected to see any change.
> OK.
>
>> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
>> ---
>>  git-gui.sh | 22 ++++++++++++++++++++--
>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/git-gui.sh b/git-gui.sh
>> index 3a83dd5..c56aeef 100755
>> --- a/git-gui.sh
>> +++ b/git-gui.sh
>> @@ -1021,6 +1021,7 @@ proc load_config {include_global} {
>>  ##
>>  ## feature option selection
>>  
>> +set run_picker_on_error 1
>>  if {[regexp {^git-(.+)$} [file tail $argv0] _junk subcommand]} {
>>  	unset _junk
>>  } else {
>> @@ -1030,6 +1031,7 @@ 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]
>>  }
>> @@ -1047,6 +1049,7 @@ blame {
>>  	disable_option multicommit
>>  	disable_option branch
>>  	disable_option transport
>> +	set run_picker_on_error 0
>>  }
>>  citool {
>>  	enable_option singlecommit
>> @@ -1055,6 +1058,7 @@ citool {
>>  	disable_option multicommit
>>  	disable_option branch
>>  	disable_option transport
>> +	set run_picker_on_error 0
>>  
>>  	while {[llength $argv] > 0} {
>>  		set a [lindex $argv 0]
> Can we please use the available disable_option and enable_option feature
> instead of a new variable. Just for consistency around repository discovery.
>
>> @@ -1162,14 +1166,28 @@ proc pick_repo {} {
>>  	set picked 1
>>  }
>>  
>> +# run repository picker if explicitly requested
>> +switch -- $subcommand {
>> +	pick {
>> +		pick_repo
>> +		set subcommand gui
>> +		set run_picker_on_error 0
>> +	}
>> +}
>> +
> It just feels wrong to have a new pick_repo call before repository
> discovery. Can we not treat this case below as if regular repository
> discovery failed and then end up in the existing call of pick_repo?

So, your suggestion is to create an error inside the catch clause, assure GIT_VAR and
GIT_WORK_TREE are unset so we don't throw and error message and abort, and then fall
through to the existing pick_repo clause?

I think this makes code less understandable and more complicated. As written, the user
selects the repo, and now the standard discovery runs except that the picker cannot be re-run.

I like the current structure better.

>>  # find repository.
>>  if {[catch {
>>  	set _gitdir [git rev-parse --absolute-git-dir]
>>  } err]} {
>>  	if {[is_gitvars_error $err]} {
>>  		exit 1
>> -	} else {
>> +	}
>> +	if {$run_picker_on_error} {
>>  		pick_repo
>> +	} else {
>> +		catch {wm withdraw .}
>> +		error_popup [strcat [mc "Git directory not found:"] "\n\n$err"]
>> +		exit 1
>>  	}
>>  }
>>  
>> @@ -3051,7 +3069,7 @@ gui {
>>  	# fall through to setup UI for commits
>>  }
>>  default {
>> -	set err "[mc usage:] $argv0 \[{blame|browser|citool}\]"
>> +	set err "[mc usage:] $argv0 \[{blame|browser|citool|gui|pick}\]"
>>  	if {[tk windowingsystem] eq "win32"} {
>>  		wm withdraw .
>>  		tk_messageBox -icon error -message $err \
> We don't need to switch on the new subcommands?
>
> As a follow-up to my comment on 04/11: How relevant is it that variable
> $picked is set in a 'git gui pick' invocation?
>
> -- Hannes
>
Repeating, I need to understand what "picked" really does in the gui. Will definitely
address your question.

Mark