Re: [PATCH v5 08/35] lock_file(): always add lock_file object to lock_file_list

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sep 17, 2014, 16:10 UTC
Message-ID: <5419B276.9000808@alum.mit.edu>
In-Reply-To: <20140916205736.GI29050@google.com>

On 09/16/2014 10:57 PM, Jonathan Nieder wrote:

Show 8 quoted lines

> Michael Haggerty wrote:
> 
>> The ambiguity didn't have any ill effects, because lock_file objects
>> cannot be removed from the lock_file_list anyway.  But it is
>> unnecessary to leave this behavior inconsistent.
> 
> Nit: commit messages usually use present tense for current behavior
> (and imperative for the behavior after the patch).

OK, will change.

Show 7 quoted lines

> More importantly, the above + the diffstat don't leave me very happy
> about the change.
> 
> Can you spell out more about the intended effect?  E.g., is this about
> making sure other code is appropriately exercised to tolerate the
> signal handler even when there are a lot of errors (which should make
> debugging easier) or something?

This patch is mostly about two things:

* Making the state diagram for lock_file objects easier to document and
reason about.

* Defining a clear moment when the lock_file object is initialized,
early in the function. This will be useful later when its filename field
is turned into a strbuf and requires nontrivial initialization before it
can even be used to construct the filename of the lockfile.

By "diffstat" I assume you mean that some lines were added to the initialization. This is also to make the state diagram of lock_objects very clear.

I will rewrite the commit message to try to explain the commit's purpose better. Let me know if you still think that the commit is unjustified.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu