On Tue, Feb 03, 2009 at 09:55:08AM -0800, Keith Cascio wrote:
Of course. Thank you for spending time to refine your patch.
Ah, yes, I see. I wonder if that is a sign that we can use some kind of
list construct instead. I also wonder if we need the exact list. It
seems like a poor user interface to have to enumerate each option. As a
user, should I be saying "I would like to put --foobar into my primer
variable. Let me check whether it is supported"? Or should there be some
sane and succint rule by which to say "I know whether --foobar is
supported or not, because it falls into categroy X"?
I think the latter is much nicer to users. And then the help text
describes that rule (and I think the rule should be "everything that
tweaks what diffs _look_ like is included" or something like that).
But more on that:
That is a very unsatisfying list, as it bears no relation to what users
actually want to put in diff.primer, or what even makes sense there. For
example:
- "-u" is not supported, but that is something I would expect people
to want to use (in fact, it is the _only_ thing supported by
GIT_DIFF_OPTS)
- "--follow" is supported, but it makes no sense to say "my diffs
should always use --follow" (and yes, this is a result of it
confusingly being part of diff opts, when it is really about
revision processing).
- "--exit-code" and "--quiet" are supported, but why would you want to
have them on all the time as defaults?
- "--relative" is supported, but "--relative=" is not.
So I think the behavior is quite confusing for potential users. I don't
mind as much that some options don't make sense (like --exit-code and
--follow), because people who put them in diff.primer get what they
deserve. But not supporting some options that people do want to use is
going to look like a bug.
Right, but you can see that I think that should change. :) I think there
are some quote-parsing routines that we use for config and for aliases
that may work for re-use, but I haven't checked.
Except I think in (1) it should be "~master->mask". Oops. :)
Right, I am calling into question whether we want "--primer" at all.
That is, if you think of it as just "prepend these command line options"
we can get the same thing with something like:
git diff-tree `git config diff.primer` $other_options
if the caller wants to be totally promiscuous, and
git diff-tree `git config diff.primer | filter_options` $other_options
if it wants to be paranoid (and obviously in tcl the code would be
different, but I think you can see the point).
Well, "--quiet" and "--exit-code" certainly would produce confusing
results. Though I can't imagine anyone putting those into their
diff.primer variable.
Hrm. Do we really need the multiple passes, though? Let's assume for a
minute that we don't want "--primer". The rule is: "if you are a
porcelain, you respect diff.primer; if you're not, you don't". And
because diff.primer is, by definition, a set of command line options, it
is trivial for a caller who wants to selectively apply them to a
plumbing to do so. Similarly, if somebody really wants to call a
porcelain and _disable_ options, I don't think "--no-primer" is
necessarily the right interface. Instead, the actual command line
options given override what's in diff.primer, so you can selectively
disable whatever you like.
And of course all of this extends naturally to a "primer.*" set of
config, rather than just "diff.primer".
I do think declarative interfaces can be nice, but I really have two
complaints with your approach:
1. It is somewhat fragile, because it is easy to bypass the
declarative nature in C. That is, there is an ordering constraint
with the flattening that we may not be fulfilling everywhere in
git. For example, let's say some code checks option A and then does
something with option B as a result. If this code runs before the
flattening, then it may fail to correctly pick up option A, since
it is in the slave spot. But if it runs after, the flattening may
logic may miss the option setting.
I think the "after" case above is not a problem in practice,
because we are only flattening two cases, and post-flattening we
always just operate directly on the master case. So I think we
really want to flatten at the last minute. So rather than
flattening everything and storing it, I think a nicer approach is
to put it in the accessor:
/* If master set it, take that. If not, and slave set it,
take that. If nobody set it, take the master as default. */
#define DIFF_OPT_TST(opts, flag) \
((opts)->master_mask & DIFF_OPT_##flag ? \
(opts)->master_flag & DIFF_OPT_##flag : \
(opts)->slave_mask & DIFF_OPT_##flag ? \
(opts)->slave_flag & DIFF_OPT_##flag : \
(opts)->master_flag & DIFF_OPT_##flag;
2. The current flattening code deals only with the flags field, which
is fairly easy to handle using a mask and some macros. But what
about fields that have a more complex implementation? I think the
amount of work to do flattening is going to be linear with the
number of options. Which means that now there is new work to be
done every time a new non-flag option is added, and a chance for
the developer to forget to do that work. Which hurts
maintainability in the long run.
So yes, I think a declarative solution can be nice, but there is really
very little language support in C for doing it in a safe way. I think
you can get by on the flags with some macros, but I don't think there is
really a nice general solution for all of the options that won't make
per-option work. If you really think it is possible, I invite you to try
to make a nice set of macros that cover all cases.
-Peff
PS I am behind on git mail and catching up slowly; I see there are some
other messages in this thread, but I don't have time to read them right
at this second. So don't think I am ignoring issues raised there -- I
just haven't gotten to them yet.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
