Re: [PATCH] git-gui: Update in-memory config when changing config options

From: Pratyush Yadav <me@yadavpratyush.com>
Date: Aug 26, 2019, 19:10 UTC
Message-ID: <20190826191006.dmcj6kipwxnttc3s@yadavpratyush.com>
In-Reply-To: <xmqqimqkknup.fsf@gitster-ct.c.googlers.com>

On 26/08/19 07:22AM, Junio C Hamano wrote:

Show 6 quoted lines

> Pratyush Yadav <me@yadavpratyush.com> writes:
> 
> 
> > Subject: Re: [PATCH] git-gui: Update in-memory config when changing config options
> 
> s/git-gui: Update/git-gui: update/

 
I fixed this in my tree, just didn't send a re-roll with it. I assumed 
you will pull from there so you'd get the updated subject.

Show 15 quoted lines

> >  lib/option.tcl | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/option.tcl b/lib/option.tcl
> > index e43971b..139cf44 100644
> > --- a/lib/option.tcl
> > +++ b/lib/option.tcl
> > @@ -344,6 +344,7 @@ proc do_save_config {w} {
> >  	if {[catch {save_config} err]} {
> >  		error_popup [strcat [mc "Failed to completely save options:"] "\n\n$err"]
> >  	}
> > +	load_config 1
> 
> This may make the symptom go away, and in that sense it would be a
> good change in the short term.

 
True.

Show 24 quoted lines

> But I have to suspect that it may indicate a misdesign in the "edit
> configuration" part of the program that the newly set configuration
> value must load back to the program from the filesystem.  That feels
> backwards.
> 
> NaaNaïvely, one would imagine a program wia capability to save and
> load run-time options to disk to behave this way, no?
> 
>  * a set of in-core variables exist to control various aspects of
>    the program (e.g. font size, background colour, etc.)
> 
>  * there is a "load config" helper function that can be called to
>    populate these in-core variables from an external file.
> 
>  * there is a "edit config" UI that can be used to toggle these
>    in-core variables (the checkboxes and radio buttons may not
>    directly be connected to the underlying variables, but to their
>    temporary counterparts and there may be a "OK" button in the UI
>    to commit the changes to the temporaries to the real in-core
>    variables).
> 
>  * there is a "save config" helper function that can be called to do
>    the reverse of "load config"; one of the places that calls this
>    helper is upon the success of "edit config".

I took a deeper look, and saving config should _in theory_ update the in-memory state, and this indeed does happen for repo-specific settings (which I unfortunately didn't test too well. Sorry). Changing global settings is what is flawed.

I leave it up to you to decide if you want to pull the current patch. I don't mind if you don't. I'll see if I can find some time to debug this and send a proper fix.

Thanks for your input.

Show 6 quoted lines

> I didn't look at the lib/option.tcl to check, but I would suspect
> that it would require a far larger change than your single liner if
> we wanted to restructure the option tweaking part in such a way, and
> it would be much more preferrable to use the single liner patch at
> least for now, but in the longer term you might want to consider
> such a clean-up.

-- 
Regards,
Pratyush Yadav