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

Keith Cascio <keith@CS.UCLA.EDU> writes:

I am not so sure about this claim. I kind of like idea of "we know the desirable value of this bit, do not touch it any further" mask, but I think it is independent of flattening. In fact, I do not think flattening is a good thing.

Any codepath could call DIFF_OPT_SET()/CLR(), whether it is in response to end user's input from the command line (e.g. "the user said --foo, so I am flipping the foo bit on/off), or to enforce restriction to achieve sane semantics (e.g. "it does not make any sense to run this internal diff without --binary set, so I am using OPT_SET()"). Is it still true, or do some bit flipping now need to be protected by "is it locked" check and some others don't?

The latter one (i.e. in the above example, "this internal diff must run with --binary") we may want to use "opts->mask" to lock down (I wouldn't call it "dirty", it is more like "locked") the "binary" bit, and we may even want to issue a warning or an error when the end user attempts to countermand with --no-binary. Similarly, I think you would want to lock down what you got from the true command line so that you can leave them untouched when you process the value you read from diff.primer.

Doesn't it suggest that you may want two layers of masks, not a flat one, if you really want the mechanism to scale?

In any case, I think the mechanism based on the lock-down mask is worth considering when we enhance the option parsing mechanism for the diff and log family, and if it is done right, I think the code to parse revision options would benefit quite a bit. There are codepaths that initialize the bits to the command's liking (e.g. show wants to always have diff), let setup_revisions() to process command line flags, and then further tweak the remaining bits (e.g. whatchanged wants to default to raw), all interacting in quite subtle ways.

But that should probably be for later cycle, post 1.6.2.