From: Johannes Sixt <j6t@kdbg.org>
Date: Tue, 19 May 2026 08:21:55 GMT
Subject: Re: [PATCH v1 11/11] git-gui: add gui and pick as explicit subcommands
Message-ID: <3b16fbc6-074b-410d-861e-6f77794b02a0@kdbg.org>
In-Reply-To: <fad43240-1089-4447-b97d-ee553c34eef1@gmail.com>

Am 16.05.26 um 17:42 schrieb Mark Levedahl:
> On 5/16/26 4:18 AM, Johannes Sixt wrote:
>> Am 14.05.26 um 16:33 schrieb Mark Levedahl:
>>> 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 I would be happier with the structure

if not subcommand pick
    discover gitdir
    if error
        set subcommand pick

if subcommand pick
    pick_repo
    set subcommand gui

because this clarifies that pick_repo must erase all current traces of
GIT_DIR and GIT_WORK_TREE from the envionment and must complete with a
valid setup.

With the structure in the proposed patch

if subcommand pick
    pick_repo
    set subcommand gui

discover gitdir
if error
    pick_repo

we still need the same operation of pick_repo, but after it runs due to
a pick command, we go into "discover gitdir" mode in an already modified
environment, something that does not happen if pick_repo runs due to the
error in the gitdir discovery.

-- Hannes