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

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

quoted

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.

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/