On Thu, Jan 28, 2010 at 10:35:50PM +0100, Stefan Sperling wrote:
> > > Fixed, but with doubts. Passing only hi instead of (hi, n, fuzz) was
> > > fine but only passing target made it harder to understand why the caller
> > > calls copy_hunk(). But I've done it so I couldn't have been totally
> > > against it.
> >
> > Yes, copy_hunk() is a bad name now that this function does so much
> > mroe than just copying. Hence my suggestion to rename the function
> > to something more general.
> >
> > But that's a trivial change I can make, too. You don't need to post
> > another revision of this diff. I think it is very good now, thanks! :)
>
> Well... looking at this in detail made me realise that my idea
> to do all of this in a single function was pretty stupid. Sorry :(
>
> Still, I think we can make the code cleaner by splitting the old
> copy_hunk_lines() interface into two distinct reject_hunk() and
> apply_hunk() interfaces. Both copy lines from some hunk text to some
> target stream, but do so in very different ways and I think a little
> code duplication is justified and even increases readability of the code.
>
> What do you think of the diff below, based on yours?
> I've updated the log message, too. Patch tests pass.
Using reject_hunk() and apply_hunk() is cleaner. +1. A function should
do one thing and do it well. copy_hunk_{text,lines,} did too much.
I've looked so much at this piece of code that the code is
starting to blur in front of my eyes but I trust your judgement.
Daniel
Received on 2010-01-28 23:03:12 CET