* [PATCH v3 1/2] gitk: write only changed configuration variables
2014-11-09 7:56 [PATCH v3 0/2] gitk: save only changed configuration on exit Max Kirillov
@ 2014-11-09 7:57 ` Max Kirillov
2014-11-09 7:57 ` [PATCH v3 2/2] gitk: synchronize config write Max Kirillov
1 sibling, 0 replies; 3+ messages in thread
From: Max Kirillov @ 2014-11-09 7:57 UTC (permalink / raw)
To: Paul Mackerras; +Cc: git, Max Kirillov
When gitk contains some changed parameter, and there is existing
instance of gitk where the parameter is still old, it is reverted to
that old value when the instance exits.
Instead, store a parameter in config only it is has been modified in the
exiting instance. Otherwise, preserve the value which currently is in
file. This allows editing the configuration when several instances are
running, and don't get rollback of the modification if some other
instance where the configuration was not edited is closed last.
For scalar variables, use trace(3tcl) to detect their change. Since `trace` can
send bogus events, doublecheck if the value has really been changed, but once
it is marked as changed, do not reset it back to unchanged ever, because if
user has restored the original value, it's the decision which should be stored
as well as modified value.
Treat view list especially: instead of rewriting the whole list, merge
individual views. Place old and updated views at their older placed, add
new ones to the end of list. Collect modified view explicitly, in newviewok{}.
Do not merge geometry values. They are almost always changing because
user moves and resises windows, and there is no way to find which one of
the geometries is most desired. Just overwrite them unconditionally,
like earlier.
Signed-off-by: Max Kirillov <max@max630.net>
---
gitk | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 74 insertions(+), 8 deletions(-)
diff --git a/gitk b/gitk
index 78358a7..f6409c2 100755
--- a/gitk
+++ b/gitk
@@ -2776,12 +2776,38 @@ proc doprogupdate {} {
}
}
+proc config_init_trace {name} {
+ global config_variable_changed config_variable_original
+
+ upvar #0 $name var
+ set config_variable_changed($name) 0
+ set config_variable_original($name) $var
+}
+
+proc config_variable_change_cb {name name2 op} {
+ global config_variable_changed config_variable_original
+
+ upvar #0 $name var
+ if {$op eq "write" &&
+ (![info exists config_variable_original($name)] ||
+ $config_variable_original($name) ne $var)} {
+ set config_variable_changed($name) 1
+ }
+}
+
proc savestuff {w} {
- global viewname viewfiles viewargs viewargscmd viewperm nextviewnum
- global use_ttk
global stuffsaved
global config_file config_file_tmp
- global config_variables
+ global config_variables config_variable_changed
+ global viewchanged
+
+ upvar #0 viewname current_viewname
+ upvar #0 viewfiles current_viewfiles
+ upvar #0 viewargs current_viewargs
+ upvar #0 viewargscmd current_viewargscmd
+ upvar #0 viewperm current_viewperm
+ upvar #0 nextviewnum current_nextviewnum
+ upvar #0 use_ttk current_use_ttk
if {$stuffsaved} return
if {![winfo viewable .]} return
@@ -2793,16 +2819,24 @@ proc savestuff {w} {
if {$::tcl_platform(platform) eq {windows}} {
file attributes $config_file_tmp -hidden true
}
+ if {[file exists $config_file]} {
+ source $config_file
+ }
foreach var_name $config_variables {
upvar #0 $var_name var
- puts $f [list set $var_name $var]
+ upvar 0 $var_name old_var
+ if {!$config_variable_changed($var_name) && [info exists old_var]} {
+ puts $f [list set $var_name $old_var]
+ } else {
+ puts $f [list set $var_name $var]
+ }
}
puts $f "set geometry(main) [wm geometry .]"
puts $f "set geometry(state) [wm state .]"
puts $f "set geometry(topwidth) [winfo width .tf]"
puts $f "set geometry(topheight) [winfo height .tf]"
- if {$use_ttk} {
+ if {$current_use_ttk} {
puts $f "set geometry(pwsash0) \"[.tf.histframe.pwclist sashpos 0] 1\""
puts $f "set geometry(pwsash1) \"[.tf.histframe.pwclist sashpos 1] 1\""
} else {
@@ -2812,11 +2846,34 @@ proc savestuff {w} {
puts $f "set geometry(botwidth) [winfo width .bleft]"
puts $f "set geometry(botheight) [winfo height .bleft]"
+ array set view_save {}
+ array set views {}
+ if {![info exists permviews]} { set permviews {} }
+ foreach view $permviews {
+ set view_save([lindex $view 0]) 1
+ set views([lindex $view 0]) $view
+ }
puts -nonewline $f "set permviews {"
- for {set v 0} {$v < $nextviewnum} {incr v} {
- if {$viewperm($v)} {
- puts $f "{[list $viewname($v) $viewfiles($v) $viewargs($v) $viewargscmd($v)]}"
+ for {set v 1} {$v < $current_nextviewnum} {incr v} {
+ if {$viewchanged($v)} {
+ if {$current_viewperm($v)} {
+ set views($current_viewname($v)) [list $current_viewname($v) $current_viewfiles($v) $current_viewargs($v) $current_viewargscmd($v)]
+ } else {
+ set view_save($current_viewname($v)) 0
+ }
+ lappend views_modified_names $current_viewname($v)
+ }
+ }
+ # write old and updated view to their places and append remaining to the end
+ foreach view $permviews {
+ set view_name [lindex $view 0]
+ if {$view_save($view_name)} {
+ puts $f "{$views($view_name)}"
}
+ unset views($view_name)
+ }
+ foreach view_name [array names views] {
+ puts $f "{$views($view_name)}"
}
puts $f "}"
close $f
@@ -4259,6 +4316,7 @@ proc newviewok {top n {apply 0}} {
incr nextviewnum
set viewname($n) $newviewname($n)
set viewperm($n) $newviewopts($n,perm)
+ set viewchanged($n) 1
set viewfiles($n) $files
set viewargs($n) $newargs
set viewargscmd($n) $newviewopts($n,cmd)
@@ -4271,6 +4329,7 @@ proc newviewok {top n {apply 0}} {
} else {
# editing an existing view
set viewperm($n) $newviewopts($n,perm)
+ set viewchanged($n) 1
if {$newviewname($n) ne $viewname($n)} {
set viewname($n) $newviewname($n)
doviewmenu .bar.view 5 [list showview $n] \
@@ -12122,6 +12181,10 @@ set config_variables {
linehoveroutlinecolor mainheadcirclecolor workingfilescirclecolor
indexcirclecolor circlecolors linkfgcolor circleoutlinecolor
}
+foreach var $config_variables {
+ config_init_trace $var
+ trace add variable $var write config_variable_change_cb
+}
parsefont mainfont $mainfont
eval font create mainfont [fontflags mainfont]
@@ -12249,6 +12312,7 @@ set highlight_related [mc "None"]
set highlight_files {}
set viewfiles(0) {}
set viewperm(0) 0
+set viewchanged(0) 0
set viewargs(0) {}
set viewargscmd(0) {}
@@ -12307,6 +12371,7 @@ if {$cmdline_files ne {} || $revtreeargs ne {} || $revtreeargscmd ne {}} {
set viewargs(1) $revtreeargs
set viewargscmd(1) $revtreeargscmd
set viewperm(1) 0
+ set viewchanged(1) 0
set vdatemode(1) 0
addviewmenu 1
.bar.view entryconf [mca "Edit view..."] -state normal
@@ -12322,6 +12387,7 @@ if {[info exists permviews]} {
set viewargs($n) [lindex $v 2]
set viewargscmd($n) [lindex $v 3]
set viewperm($n) 1
+ set viewchanged($n) 0
addviewmenu $n
}
}
--
2.0.1.1697.g73c6810
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH v3 2/2] gitk: synchronize config write
2014-11-09 7:56 [PATCH v3 0/2] gitk: save only changed configuration on exit Max Kirillov
2014-11-09 7:57 ` [PATCH v3 1/2] gitk: write only changed configuration variables Max Kirillov
@ 2014-11-09 7:57 ` Max Kirillov
1 sibling, 0 replies; 3+ messages in thread
From: Max Kirillov @ 2014-11-09 7:57 UTC (permalink / raw)
To: Paul Mackerras; +Cc: git, Max Kirillov
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 <max@max630.net>
---
gitk | 33 ++++++++++++++++++++++++++++++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/gitk b/gitk
index f6409c2..0613264 100755
--- a/gitk
+++ b/gitk
@@ -2776,6 +2776,19 @@ proc doprogupdate {} {
}
}
+proc config_check_tmp_exists {tries_left} {
+ global config_file_tmp
+
+ if {[file exists $config_file_tmp]} {
+ incr tries_left -1
+ if {$tries_left > 0} {
+ after 100 [list config_check_tmp_exists $tries_left]
+ } else {
+ 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"
+ }
+ }
+}
+
proc config_init_trace {name} {
global config_variable_changed config_variable_original
@@ -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"
+ }
+ if {$remove_tmp} {
+ file delete -force $config_file_tmp
}
set stuffsaved 1
}
@@ -12168,6 +12194,7 @@ catch {
}
source $config_file
}
+config_check_tmp_exists 50
set config_variables {
mainfont textfont uifont tabstop findmergefiles maxgraphpct maxwidth
--
2.0.1.1697.g73c6810
^ permalink raw reply related [flat|nested] 3+ messages in thread