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

[Max Kirillov <max@max630.net>]

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.

> 
>> +	    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

> > @@ -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