On Mon, Nov 10, 2014 at 12:20:02AM +0200, Max Kirillov wrote:
> If several gitk instances are closed simultaneously, safestuff procedure
> can run at the same time, resulting in a conflict which may cause losing
> of some of the instance's changes, failing the saving operation or even
> corrupting the configuration file. This can happen, for example, at user
> session closing, or at group closing of all instances of an application
> which is possible in some desktop environments.
>
> To avoid this, make sure that only one saving operation is in progress.
> It is guarded by existance of $config_file_tmp file. Both creating the
> file and moving it to $config_file are atomic operations, so it should
> be reliable.
>
> Reading does not need to be syncronized, because moving is atomic
> operation, and the $config_file always refers to full and correct file.
> But, if there is a stale $config_file_tmp file, report it at gitk start.
> If such file is detected at saving, just abort the saving, because this
> is how gitk used to handle errors while saving.
>
> Signed-off-by: Max Kirillov <[email protected]>
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?
> + 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."
> @@ -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:
if {[catch {
...
file rename -force $config_file_tmp $config_file
} err]} {
puts "Error saving config: $err"
if {$remove_tmp} {
file delete -force $config_file_tmp
}
}
rather than doing a return inside the catch.
Paul.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
