Code review comment for lp:~maria-captains/bzr/diffoptions

I'm sorry merge proposal was stalled for so long. I think somehow it was missing a review request for bzr-core, so it wasn't showing in the “Reviews I have to do” section of <https://code.launchpad.net/bzr/+activereviews>.

This looks like a nice idea to me. Thanks for proposing it!

I agree with mgz that'd be nice to support this in the internal diff, but it would be significantly more work (although I'd guess it might not be very hard either).

I like the idea of using the rules file for this setting. We do want to take care I think to distinguish between “diff for 'bzr diff' output” and “diff used by merge directives etc”. I'm not sure that the current patch does that, although I'm not very familiar with how other parts of bzrlib invoke this module.

Some superficial comments on the code itself:

 * compare functions, like internal_diff, using the 'is' operator rather than the '==' operator.
 * don't assume that the result of iter_search_rules(…).next() is a tuple; I think at the moment that happens to be the implementation, but that could easily change to e.g. a list or generator, which would then cause your “diff_show_function_re == ()” check to give the wrong answer in some cases.

This will need tests to be mergeable. Some cases that immediately occur to me:

 * what happens if rules file specifies a diff_show_function_re for a file, but no external diff could be run? I think ideally we'd warn the user (once per bzr invocation, not once per file diffed!) and fallback to internal diff.
 * what happens if the user does “bzr diff --diff-options=…”? I think the intent of your code is to ignore any diff_show_function_re setting in rules in this case, which seems reasonable to me, but it deserves a test.