From: Michael Haggerty Date: Wed, 17 Sep 2014 16:10:30 GMT Subject: Re: [PATCH v5 08/35] lock_file(): always add lock_file object to lock_file_list Message-ID: <5419B276.9000808@alum.mit.edu> In-Reply-To: <20140916205736.GI29050@google.com> 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 mhagger@alum.mit.edu