Peff,
Thank you for this extremely thoughtful reply. First, I want to ease concern
over the point about "intent-to-change". BTW, everything I describe here is
already implemented in v3.
On Fri, 20 Mar 2009, Jeff King wrote:
> Look, I am not opposed to layer flattening if that's what is required to get
> it right. But consider the downside of layer flattening: we must always record
> intent-to-change when making a change to the struct (i.e., the "mask" variable
> in your original patches). This is fine for members hidden behind macros, but
> there are a lot of members that are assigned to directly. We would need to:
>
> 1. Introduce new infrastructure for assigning to these members.
Only the bit flag fields need special infrastructure! IOW, the macros are only
necessary for the bit flags. For numeric data or pointer data, there's no need
to keep extra state, and there's no need for callsites to change from direct
assignment. Only for bit flags, we need an extra bit to remember whether the
value is pristine or not. For all other data:
(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.
(b) pointer data: NULL is the pristine value. Any value other than NULL means
intent-to-change.
> 2. Fix existing locations by converting them to this infrastructure.
As of 628d5c2, all callsites that set bit flags are already updated to use the
macros. As mentioned above, all other locations can keep on keepin' on using
direct assignment. No change here.
> 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. In order to commit his crime, this hypothetical programmer must
ignore the fact that these bits are never set directly, anywhere in the code,
period. A competent programmer would, after deciding that he needs to set a
bit, look at other spots where such bits are set, and mimic. I think the normal
patch auditing process this community follows would raise alarms on patches
coming from negligent programmers (there are always tell-tale signs). And, in
the event that, nevertheless, Junio applies a bit-flag-direct-assignment patch,
it will result in a bug of precisely the following form: an explicitly-given
command-line option to a porcelain command fails to override a default option.
It will be noticed and fixed. It's not fatal, it doesn't corrupt data, it
affects only porcelain and it's not hidden. Of all the insect kingdom (grand
scheme of hypothetical bugs), this one isn't worth abandoning a good design
over.
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?
-- Keith
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
