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

From: Michael Haggerty <[email-masked]>
Date: Wed, 17 Sep 2014 18:10:30 +0200

On 09/16/2014 10:57 PM, Jonathan Nieder wrote:
> 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.

> 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
[email-masked]