Hi Jason, Could you please work on this patch some more and resubmit? My complaints are
1) Nitpick:
The description should be cleaned up a bit. I certainly
appreciate that you took pains to explain what's going
on and why, but I find that the language itself is a
little hard to follow.
For example, what's this business of "stop computing the patch for a
while?" A while? As in it stops computing, and then picks up later
on?
Also, I had trouble understandin this long chain without rereading
a couple of times. Could you maybe break it up into a couple of
sentences?
A change was needed for each because they use get_unrecorded
which was modified
so that it would pass on the necessary information to smart_diff
so that it would know when to skip reading file contents
2) NoSummary:
I would take this out if we don't really know what it does.
Which brings us to number 3...
> It may very well be redundant in some cases. I took a conservative
> approach (fearing corner cases). If you're not in summary mode and
> smart_diff behaves as if you are then your patches will be incorrectly
> created.
3) Since it's been a while since you last wrote that code, I would
like you to revisit it and ponder over it a little more.
The fact that there are corner cases makes me nervous. While I like
the cautiousness of your approach, my first reaction would be that
the NoSummary solution papers over the corner cases, without actually
addressing the root problem.
Could you please investigate the cause of these corner cases? For
example, is it because 'Summary' is somehow sneaking into opts where
it doesn't belong? Or is it something else? It this is that case,
then maybe sprinkling these NoSummary around would be a prudent way
to programmer-proof the code. Or maybe a better solution would be to
make the flag more explicit. I don't know.
In any case, I would be more comfortable accepting this patch if
you could explain and reproduce these issues
Best,
--
Eric Kow http://www.loria.fr/~kow
PGP Key ID: 08AC04F9 Merci de corriger mon français.
pgp1MlPBVYMHc.pgp
Description: PGP signature
_______________________________________________ darcs-devel mailing list [email protected] http://www.abridgegame.org/cgi-bin/mailman/listinfo/darcs-devel
