Re: [PATCH] Use DIFF_XDL_SET/DIFF_OPT_SET instead of raw bit-masking

[Jeff King <peff@peff.net>]

On Wed, Feb 18, 2009 at 02:59:29PM -0800, Keith Cascio wrote:

> Actually I appreciate your feedback, and the more direct the better.
> The chief value I see in revising the code to accomplish these bit
> settings uniformly using well known macros is that later, if someone
> has good reason to extend the macro so it results in some new side
> effect, e.g. to update a dirty bit mask, the new behavior
> automatically cascades to every appropriate use out there in the code.
> A macro is essentially a "code constant".  As with other constants,
> the benefit comes from defining it once and using it everywhere.  Is
> this effect not worth as much as I think it is?  Is there a hidden
> gotcha in my ideal?  Or does anyone else see value here?  Please speak
> up!

I think you are running into "if it's not broken, don't fix it".  That
is, there is a transaction cost to making changes to the code. It can be
as small a cost as "now somebody working in a related area has a
conflict (even if textual) and has to spend time resolving". Or it can
be as big as "you introduced new bugs".  In this case, I think any costs
would tend towards the former and not the latter.

That cost has to be weighed against the benefit.

The usual attitude in git is that minor cleanups or enhancements like
this should be coupled with patches that build on them. Saying "this
might help in the future if somebody does X" means you are paying the
cost now, but there may or may not ever _be_ any benefit.

In this case, I think there are actually two changes in your patch:

  1. consistently using DIFF_OPT_* macros, which are used in 99% of
     callsites already

  2. adding and using DIFF_XDL macros

I think (1) has immediate value. Anyone looking at the code and seeing
the existence of DIFF_OPT macros and how commonly they are used might
think they are used exclusively. And that means it is easy to miss
callsites when searching through the code. So to me, making things
consistent brings value.

For (2), that situation does not exist, since you are introducing new
macros.

Personally, I don't mind the abstraction layer of the XDL macros,
especially since they match the style of the existing DIFF_OPT macros
(and the only reason they are not in the DIFF_OPT bitfield at all is
that they must be passed separately to xdiff).

-Peff