Re: [PATCH v2 1/2] Introduce config variable "diff.defaultOptions"

From: Jeff King <peff@peff.net>
Date: Mar 20, 2009, 19:49 UTC
Message-ID: <20090320194930.GB26934@coredump.intra.peff.net>
In-Reply-To: <alpine.GSO.2.00.0903200911530.16242@kiwi.cs.ucla.edu>

On Fri, Mar 20, 2009 at 10:11:27AM -0700, Keith Cascio wrote:

> (a) numeric data (integers, chars, and floats): define magic value(s)
> that represent pristineness.  Initialize all fields to PRISTINE.
> Later, if a field has any value other than PRISTINE, we know there was
> intent-to-change.

Good point. Though we will need to make sure that existing code is never looking at PRISTINE values, which aren't likely to make much sense (I suspect most will be INT_MAX or -1, as 0 is a reasonable value for many of the options). This should be easy for most code, since the flattening will get rid of PRISTINE. But remember that there are pieces of code that do something like:

  if (some_diff_option_is_set)
     set_some_other_related_diff_option;

which will need to be PRISTINE-aware.

Show 7 quoted lines

> >   3. Introduce some mechanism to help future callers get it right
> >   (since otherwise assigning directly is a subtle bug).
> 
> Yes, in the future, someone could write naughty code that sets a bit
> flag directly rather than using one of the macros.  In C, we probably
> can't make that impossible.  But generally speaking we can't protect
> against all forms of gross negligence.

I think you can safely ignore this complaint. I was worried we would need something like:

  DIFF_SET(&opt, stat_name_width, 10);

It is much easier to mistakenly write this as

  opt.state_name_width = 10;

than it is to accidentally do a bit-set when there is a DIFF_OPT_SET macro. That is, I think most people _want_ to use DIFF_OPT_SET because it is easier to read and less typing.

So I saw this as a problem more for non-bit options, but you have already addressed that above.

Show 10 quoted lines

> All in all, turns out v3 requires surprisingly little modification of
> existing code outside of diff.h/diff.c.  Actually, it only adds 3
> lines, that's it!!
>
>  builtin-diff.c                  |    2 +
>  builtin-log.c                   |    1 +
>  diff.c                          |  112 ++++++++++++++++++++++++-
>  diff.h                          |   17 +++-
>
> Shall I post v3?

Yes, please. It is much better to be talking about actual code than hypotheticals.

-Peff