Re: [PATCH v4 2/2] gitk: synchronize config write

From: Max Kirillov <max@max630.net>
Date: Mar 2, 2015, 20:43 UTC
Message-ID: <20150302204354.GB7622@wheezy.local>
In-Reply-To: <20150302001050.GC24862@iris.ozlabs.ibm.com>

On Mon, Mar 02, 2015 at 11:10:51AM +1100, Paul Mackerras wrote:

> The idea looks good; I have a couple of comments on the patch.  First,
> 50 tries over 5 seconds seems a bit excessive to me, wouldn't (say) 20
> tries be enough?  Is the 50 the result of some analysis?

5 seconds was just my personal feeling where "too long" is starting.

I have made some quick experiment: I opened ~10 instances and closing them with group close in Windows 7. With 1 second wait it often can hit the error while closing several instances at once. With 20 seemed to work reliably. Will look a bit more though, maybe also handle the failing case more nice, to avoid having leftover.

The interval of 100 milliseconds was also voluntary. Maybe need to measure how long the saving actually takes.

Show 7 quoted lines

> 
>> +	    error_popup "Probably there is stale $config_file_tmp file; config saving is going to fail. Check if it is being used by any existing gitk process and remove it otherwise"
> 
> I would word this as "There appears to be a stale $config_file_tmp
> file, which will prevent gitk from saving its configuration on exit.
> Please remove it if it is not being used by any existing gitk
> process."

ok, will change it

Show 34 quoted lines

> > @@ -2811,11 +2824,16 @@ proc savestuff {w} {
> >  
> >      if {$stuffsaved} return
> >      if {![winfo viewable .]} return
> > +    set remove_tmp 0
> >      catch {
> > -	if {[file exists $config_file_tmp]} {
> > -	    file delete -force $config_file_tmp
> > +	set try_count 0
> > +	while {[catch {set f [open $config_file_tmp {WRONLY CREAT EXCL}]}]} {
> > +	    if {[incr try_count] > 50} {
> > +		error "Unable to write config file: $config_file_tmp exists"
> > +	    }
> > +	    after 100
> >  	}
> > -	set f [open $config_file_tmp w]
> > +	set remove_tmp 1
> >  	if {$::tcl_platform(platform) eq {windows}} {
> >  	    file attributes $config_file_tmp -hidden true
> >  	}
> > @@ -2878,6 +2896,14 @@ proc savestuff {w} {
> >  	puts $f "}"
> >  	close $f
> >  	file rename -force $config_file_tmp $config_file
> > +	set remove_tmp 0
> > +	return ""
> > +    } err
> > +    if {$err ne ""} {
> > +	puts "Error saving config: $err"
> 
> I would suggest checking the return from the catch statement, like
> this:
> ...
> rather than doing a return inside the catch.

Yes, I can make proper error handling. Then I think it would better be a separated patch.

-- 
Max