Re: [PATCH 1/4] resolve_ref: close race condition for packed refs

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: May 16, 2013, 05:50 UTC
Message-ID: <519473BA.9030106@alum.mit.edu>
In-Reply-To: <20130516034721.GD13296@sigill.intra.peff.net>

On 05/16/2013 05:47 AM, Jeff King wrote:

Show 8 quoted lines

>> I probably would have separated the rest of the patch, which is a pure
>> refactoring, from this last chunk, which is a functional change.  But
>> that's just me.
> 
> Yeah, I go back and forth on whether it is better to have strict
> refactors followed by changes or not. Sometimes it is hard to understand
> the motivation for the refactor without seeing the change, and you end
> up explaining it twice.

A pure refactoring doesn't need much justification. Something like "extract function foo()" plus maybe "this function will soon have multiple callers" is IMO usually adequate, especially if the function is well-named and documented in the patch itself.

Show 13 quoted lines

> My usual rule of thumb is:
> 
>   - If you are factoring out some code, and then are going to change
>     that code, make it two separate changes. That keeps the diffs
>     readable (the first one is pure movement and you do not need to look
>     closely, and the second shows a sane diff of the change).
> 
>   - If you are factoring out some code, and then adding more callers,
>     it's OK to do it together. The new caller provides the motivation
>     for the refactor.
> 
> This is the latter case. But I'm open to arguments that the rule is not
> a good one. :)

Yes, I see how keeping the changes together makes the justification of the refactoring more obvious. On the other hand, splitting has the following benefits:

1. Reviewers have a single thing to check in each patch: "Did he
   transcribe the code correctly into a function and choose a good
   API?" vs. "Does it make sense to call the function from this new
   place?"  The threads of feedback emails about each patch are
   similarly separated.

   On the other hand, of course these two changes are not completely
   independent, because having an idea what new callers want to do
   with the function affects what its API should be.

2. If the patch series needs to be revised, it is quite possible that
   the revisions affect only one patch or the other.  Therefore, the
   unaffected patch can be carried along through interactive rebases
   etc. intact, or might serve as a building block for an alternative
   solution.

3. If there's a problem, bisect can figure out which half of the change
   was to blame.

That being said, this case is very much in the gray area where it is a matter of personal preference and I don't mind at all if you leave it as a single patch.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/