Re: [PATCH 2/3] gitk: write only changed configuration variables

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 11, 2014, 20:26 UTC
Message-ID: <xmqq4mwdewf6.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140911191726.GA5549@wheezy.local>

Max Kirillov <max@max630.net> writes:

Show 28 quoted lines

> On Thu, Sep 11, 2014 at 10:19:56AM -0700, Junio C Hamano wrote:
>> Max Kirillov <max@max630.net> writes:
>> 
>>> If a variable is changed in a concurrent gitk or manually it is
>>> preserved unless it has changed in this instance
>> 
>> It would have been easier to understand why this is a desirable
>> change if you stated what problem you are trying to solve before
>> that sentence.  "If I do X, Y happens, which is bad for reason Z.
>> With this change, Y no longer happens as long as I do not do W."
>
> Something like:
>
> """
> 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.
>
> After the change, a parameter is stored in config only it is
> has been modified in the exiting instance. Otherwise, the
> value which currently is in file is preserved. This allows
> editing the configuration when several instances are
> running, and don't get rollback of the modification if some
> other instance where the cinfiguration was not edited is
> closed last.
> """
>
> Does it looks appropriate?

Yeah, except for the phrase "after the change" may give me a hiccup or two while reading, because it is unclear if "the change" refers to this patch (which is the intended interpretation) or the change made in one of these two instances of gitk process.

Expressing the behaviour your patch modifies in the imperative mood, as if you are ordering our codebase "to become like so", would avoid such a hiccup, i.e./e.g.

	Re-read the current on-disk configuration variables and
	overwrite only the ones changed in this process when saving
	the file, to preserve the changes made by other instances.

or something like that, perhaps?

> Though storing the old value and comparing to it makes sanse
> to do anyway, because trace may produce bogus events, so it
> would be better to doublecheck has the value actually been
> changed.

Exactly.