[PATCH 2 of 8] diff: Move diffline to patch module

Matt Mackall mpm at selenic.com
Wed Nov 14 15:31:33 CST 2012


On Wed, 2012-11-14 at 14:35 -0600, Kevin Bullock wrote:
> On Nov 14, 2012, at 2:19 PM, bisho (Guillermo) wrote:
> 
> > 
> >>> The move of this function seems unrelated to the purpose of the patch.
> >> Probably because it didn't need to be a top-level function.
> > 
> >>> Also, why are we making it public?
> >> It's not public, it's a function nested in another.
> > 
> > Exactly, is not public, and I moved it within this patch because diffline
> > and that function follow the same behavior, they are helpers of trydiff,
> > so makes sense to move them together and nest them under trydiff.
> 
> 
> Okay, makes sense.

Kevin is of course absolutely right to point out that this patch doesn't
match its description and apparently includes unrelated changes.

This patch either needs a rewritten description or to be split into two
patches. In general, we much prefer the latter. In most cases, the
improved odds of smaller patches getting through the review process in
one round-trip far outweigh any efficiency gained by combining them into
larger patches (as evidenced by the five messages in this thread).

-- 
Mathematics is the supreme nostalgia of our time.




More information about the Mercurial-devel mailing list