v2, 2 partsIntroduce config variable "diff.primer"

Introduce config variable "diff.primer". Improve porcelain diff's accommodation of user preference by allowing some settings to (a) persist over all invocations and (b) stay consistent over multiple tools (e.g. command-line and gui). The approach taken here is good because it delivers the consistency a user expects without breaking any plumbing. It works by allowing the user, via git-config, to specify arbitrary options to pass to porcelain diff on every invocation, including internal invocations from other programs, e.g. git-gui. Introduce diff command-line options --primer and --no-primer. Affect only porcelain diff: we suppress primer options for plumbing diff-{files,index,tree}, format-patch, and all other commands unless explicitly requested using --primer (opt-in). Teach gitk to use --primer, but protect it from inapplicable options like --color.

Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
---
 Documentation/config.txt       |   14 +++++++
 Documentation/diff-options.txt |   10 +++++
 builtin-diff.c                 |    2 +
 diff.c                         |   77 +++++++++++++++++++++++++++++++++++-----
 diff.h                         |   14 ++++++--
 gitk-git/gitk                  |   16 ++++----
 6 files changed, 113 insertions(+), 20 deletions(-)

Test functionality of new config variable "diff.primer"

Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
---
 t/t4035-diff-primer.sh |  129 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 129 insertions(+), 0 deletions(-)
 create mode 100755 t/t4035-diff-primer.sh

Seems like the summary email for this patch refuses to deliver through the list. I can send it to anyone individually if you are interested.

                                          -- Keith

The next two patches introduce a means by which to specify non-default options porcelain diff automatically obeys. This version v2 fixes the problem with v1 (violation of plumbing guarantee) by switching to opt_in rather than opt_out semantics. v2 also corrects code style to mimic established convention.

At least one list poster expressed interest in implementing complimentary functionality, i.e. "primer.*": http://article.gmane.org/gmane.comp.version-control.git/107158 "primer.diff" compliments "diff.primer", the two styles in no way exclude each other, and therefore "primer.*" is a good opportunity for future work.

Keith Cascio (2):
 Introduce config variable "diff.primer"
 Test functionality of new config variable "diff.primer"

 Documentation/config.txt       |   14 ++++
 Documentation/diff-options.txt |   10 +++
 builtin-diff.c                 |    2 +
 diff.c                         |   77 +++++++++++++++++++++---
 diff.h                         |   14 ++++-
 gitk-git/gitk                  |   16 +++---
 t/t4035-diff-primer.sh         |  129 ++++++++++++++++++++++++++++++++++++++++
 7 files changed, 242 insertions(+), 20 deletions(-)

On Mon, Feb 02, 2009 at 10:20:54AM -0800, Keith Cascio wrote:

You don't need to repeat the subject in the body.

Paragraph breaks might have made this a bit easier to read.

Funny indentation?

This seems really clunky to list all of the options here. I thought the point was to respect _all_ of them, but do it from porcelain so that it is up to the user what they want to put in.

How was this list chosen?

Some of the manpages use a more terse form for negatable options, like:

  --[no-]primer::

which often helps focus the text a bit. Something like:

  --[no-]primer::
    Respect (or ignore) options specifed in the diff.primer
    configuration variable. By default, porcelain commands (such as `git
    diff` and `git log`) respect this variable, but plumbing commands
    (such as `git diff-{files,index,tree}`) do not.

Also, don't mention ".git/config" by name: configuration can come from ~/.gitconfig, a system-wide gitconfig, or .git/config.

Probably ALLOW_PRIMER is a more sensible name, to match ALLOW_EXTERNAL and ALLOW_TEXTCONV.

This doesn't appear to have any quoting mechanism. Is it impossible to have an option with spaces (e.g., --relative='foo bar')? I guess that is probably uncommon, but I would expect normal shell quoting rules to apply.

Style: long lines.

Style: no C99/C++ comments.

Style: whitespace between operands and operators.

I have to admit that this particular line is pretty dense to read. You have eliminated any meaning from the variable names (like the fact that you have a master/slave pair of flag/mask pairs). Yes, you point to the Quine-McCluskey algorithm in the comment above, but I think something like this would be easier to see what is going on:

  /*
   * Our desired flags are:
   *
   *   1. Anything the master hasn't explicitly set, we can take from
   *      the slave.
   *   2. Anything the slave didn't explicitly, we can take whether or
   *      not the master set it explicitly.
   *   3. Anything the master explicitly set, we take.
   */
  master->flags =
     /* (1) */ (~master->flags & slave->flags & slave->mask) |
     /* (2) */ (master->flags & ~slave->mask) |
     /* (3) */ (master->flags & master->mask);

Hmm. I haven't gotten to any changes to DIFF_OPT_{SET,CLR} yet. But it is a little worrisome that this patch is so invasive as to require a change like this. I wouldn't be surprised to find other spots outside of diff.c where the options are munged by various programs. Did you audit for all such spots?

First, don't use malloc. Use the xmalloc wrapper that will try to free pack memory and/or die if it fails.

Secondly, don't cast the result of malloc. At best it is pointless and verbose, and at worst it can hide errors caused by a missing function declaration.

It often makes the patch easier to review if you split changes like this out into a separate patch. Then your series is

  1/2: use DIFF_XDL_SET instead of raw bit-masking

       This is a cleanup in preparation for option-setting doing
       something more complex than just setting a bit-mask. The code
       should behave exactly the same.

  2/2: primer patch

       ... DIFF_XDL_SET tracks not only the set options, but which ones
       were set explicitly via a mask ...

Then we can all see pretty easily that patch 1/2 doesn't change the behavior, and each patch is a much smaller, succint chunk to review.

Ditto here with DIFF_OPT_SET.

OK, I see what it is supposed to do, but what does DRT stand for? Also, what practical use does it have? I don't see anybody _calling_ it.

Does gitk really want to respect --primer? Might it not make more sense for it, as a porcelain, to respect the diff.primer variable itself, and prepend it to the list of diff args? Then it has the power to veto any options which it doesn't handle.

Also, any gitk changes should almost certainly be split into a different patch.

All in all, this was a lot more complicated than I was expecting. Why isn't the behavior of "diff.primer" simply "pretend as if the options in diff.primer were prepended to the command line"? That is easy to explain, and easy to implement (the only trick is that you have to do an extra pass to find --[no-]primer). Is there some drawback to such a simple scheme that I am missing?

-Peff

Peff, First of all, thanks for the tips!

On Tue, 3 Feb 2009, Jeff King wrote:

OK.

How about:

Improve porcelain diff's accommodation of user preference by allowing some settings to (a) persist over all invocations and (b) stay consistent over multiple tools (e.g. command-line and gui). The approach taken here is good because it delivers the consistency a user expects without breaking any plumbing. It works by allowing the user, via git-config, to specify arbitrary options to pass to porcelain diff on every invocation, including internal invocations from other programs, e.g. git-gui.

Introduce diff command-line options --primer and --no-primer.

Affect only porcelain diff: we suppress primer options for plumbing diff-{files,index,tree}, format-patch, and all other commands unless explicitly requested using --primer (opt-in).

Teach gitk to use --primer, but protect it from inapplicable options like --color.

The last part is a backtick-quoted list, I think it either must occur on one line or as is.

The current version does not try to support all diff options. It only supports those that are recorded in struct diff_options.flags and .xdl_opts - that is the present list.

Agree.

OK.

Agree.

The current version only supports the flags listed above in Documentation/config.txt.

OK.

OK.

Your version is much better.

My proposal is to make DIFF_OPT_{SET,CLR} more meaningful. Instead of just flipping a bit, they mean "{un}set this bit and honor my intention in the future".

Yes I believe I did. The only danger zones are spots that use struct diff_options member "flags" or "xdl_opts" along with assignment and bitwise operators. The following recursive grep call (output included) satisfies me that no such spots exist after my patch.

grep -EHnr '--include=*.[ch]' '(flags|xdl_opts).*=.*(DIFF_OPT_|\b)(RECURSIVE|TREE_IN_RECURSIVE|BINARY|TEXT|FULL_INDEX|SILENT_ON_REMOVE|FIND_COPIES_HARDER|FOLLOW_RENAMES|COLOR_DIFF|COLOR_DIFF_WORDS|HAS_CHANGES|QUIET|NO_INDEX|ALLOW_EXTERNAL|EXIT_WITH_STATUS|REVERSE_DIFF|CHECK_FAILED|RELATIVE_NAME|IGNORE_SUBMODULES|DIRSTAT_CUMULATIVE|DIRSTAT_BY_FILE|ALLOW_TEXTCONV|PRIMER|XDF_NEED_MINIMAL|XDF_IGNORE_WHITESPACE|XDF_IGNORE_WHITESPACE_CHANGE|XDF_IGNORE_WHITESPACE_AT_EOL|XDF_PATIENCE_DIFF|XDF_WHITESPACE_FLAGS)\b' . | grep -Ev TST | less -S

merge-tree.c:109: xpp.flags = XDF_NEED_MINIMAL; merge-file.c:65: xpp.flags = XDF_NEED_MINIMAL; xdiff/xpatience.c:308: xpp.flags = map->xpp->flags & ~XDF_PATIENCE_DIFF; builtin-blame.c:41:static int xdl_opts = XDF_NEED_MINIMAL; combine-diff.c:217: xpp.flags = XDF_NEED_MINIMAL; builtin-rerere.c:101: xpp.flags = XDF_NEED_MINIMAL; diff.c:517: xpp.flags = XDF_NEED_MINIMAL; diff.c:1570: xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts; diff.c:1658: xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts; diff.c:1706: xpp.flags = XDF_NEED_MINIMAL; diff.c:3238: xpp.flags = XDF_NEED_MINIMAL;

OK.

OK.

Agree.

OK.

DRT means "dirty". The masks here are dirty bits. You're right, I never called it. But it seemed so meaningful to me, I felt it was right to include it while introducing the concept of dirty bits.

The point of --primer was for scripts like gitk and git-gui. I wouldn't say "respect --primer" so much as "pass --primer" (which causes diff-tree to respect diff.primer, which it normally wouldn't). Reviewing the options that diff.primer currently supports, except for --color which I specifically guarded against, I don't see a scenario that would break gitk as is.

OK.

I think we could call that simple scheme the linear walk, and it was my first impulse as well. We walk through all the places where diff options live, in order from lowest precedence to highest precedence, accumulating/overriding options as we go along. When we're done, we know the user's intention.

But as you say, we need that extra pass. Once I noticed we need that extra pass, the software engineer in me saw the concept of linear walk as too weak for this feature. These options occur in precedence layers and we need to combine those layers the right way. The right way means: honor the intuitive semantics the user expects. Hence the function name flatten_diff_options().

A multi-pass walk is one possible implementation of such semantics, but in my opinion not the best. My implementation is much more explicit, meaningful, and declarative. It's not at all hard to understand looking at the code. In fact, it positively declares itself to you and annouces exactly what it is doing. It "underlines the intent" and also "eases further extensions" in Samuel Tardieu's words: http://article.gmane.org/gmane.comp.version-control.git/105654

That's a desirability I believe you agree with :) http://article.gmane.org/gmane.comp.version-control.git/105657

I think introducing explicit dirty masks and explicit layer flattening is the right way to go forward, but I agree it would be better to split this up into smaller patches. My design would require a lot more lines of code if we wanted to support more diff options than are represented by struct diff_options.flags and .xdl_opts. That would mean introducing more masks and more CPP macros. But I have faith in declarative programming. 100 lines of clear-as-day meaningful code don't scare me nearly as much as 10 lines of secret obfuscation. Also, I'm not convinced it is necessary for diff.primer to support all diff options under the sun. Thoughts?

                                   -- Keith

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.

On Tue, 3 Feb 2009, Junio C Hamano wrote:

Yes but the trick is the flips and maskings happen on different structs. We accumulate the shell command line flags/masks in a separate struct from the primer flags/masks. IOW, there's a whole lotta flags and masks!!

There are indeed two layers of masks (and there can be as many as needed). In my current patch, the shell command line becomes "master" and primer becomes "slave". Both layers exist independently of each other, in two separate diff_option structs, until just before "go time", when I flatten them (but that does not destroy the slave, it is reused). As Peff put it: "a master/slave pair of flag/mask pairs". I specifically designed the code to make it easy to create an arbitrary number of layers, then flatten them all together just before it's time to do something. The code only needs to keep track of the order of precedence, i.e. always pass the higher precedence struct to flatten_diff_options() as master and the lower precedence struct as slave, and the bit logic in that function does the rest. I was specifically thinking of GIMP or Photoshop when I wrote this patch. The concept is the same. Those programs support an arbitrary number of layers, and when they produce the final image, they call it "flatten layers".

If you and Peff like this design then I could clean up everything based on all of Peff's suggestions (i.e. xmalloc instead of malloc, etc) and hopefully move on to the stage of building consensus for the actual name. No rush of course. Just give me the word. Peff?

                                      -- Keith

On Tue, Feb 03, 2009 at 10:36:48PM -0800, Keith Cascio wrote:

We have had trouble in the past figuring out exactly when "go time" is (not specifically for diff options, but for things like color config). That is, there is code which wants to munge the options based on some other input much later than the setting of most options, and so any finalizing work you do ends up happening too early. And maybe you can argue that such code is wrong or bad, but it does get written and it does cause problems.

So as I mentioned in another mail I just sent, I think you are best to stop thinking of it as a general flattening, and think of it more as a set of accessors that do the flattening Just In Time.

-Peff

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.

Jeff King <peff@peff.net> writes:

I agree with this 100%.

Also, I think we should explain the semantics of diff.primer to new people like that. "The diff Porcelain command acts as if whatever you have in diff.primer are prepended on your command line".

Which means that we do not have to touch the plumbing at all. That allows me sleep much better at night.

On Fri, 6 Feb 2009, Jeff King wrote:

Sir I appreciate the intention, as I interpret it, that it's always better to accomplish something without adding new vocabulary. I'd much rather avoid adding new vocab if possible. If I'm missing something, I apologize ahead of time, but let me describe the problem I see. Let's take the context size setting as an example, i.e. -U<n> or --unified=<n>. Default is 3. Let's say someone defines diff.primer = -U6. Now, without --no-primer, how does a program say "use the default value for context." Aren't there options for which no inverse counterpart exists? Is there command-line syntax to disable all whitespace ignore options, e.g. to disable -b? If not then we need --no-primer.

On Mon, Feb 09, 2009 at 09:24:37AM -0800, Keith Cascio wrote:

Good point. I think there are actually two separate problems you describe:

  1. some default behaviors which can be changed via an option have no
     option that represents the default. This is your "-b" example (and
     there are others, like "-a", "--full-index", etc).

     Generally I think we try to allow boolean options to be specified
     in either positive or negative ways. Our parse-options library even
     automatically handles --no-$foo by default for any boolean option
     (as long as it has a corresponding long option).

     So I consider the lack of --no-ignore-space-change to be a failing
     of git, but one that we can correct. Either manually or by moving
     the diff code to parse-options.

  2. For options which take a value, there is no way to say "pretend I
     didn't specify a value at all".

     Actually, that is not entirely true. parse-options handles
     "--foo=bar --no-foo" as if "--foo" was never specified at all.

     But there are still two failure cases:

       - as above, the diff options are not handled by parse-options

       - not all value options are quite as straightforward. Some
         options run callbacks that do things which take specialized
         code to undo.

     Both are fixable. But I have to wonder if it is really all that
     useful to say "use the default for this option". Either you don't
     care what the value is, in which case you can take the default
     given by your primer value. Or you do care, in which case wouldn't
     you want to be setting the value yourself?

So I think doing it right is a bit more work in the long run, but the extra work is generally improving git.

All that being said, though, I still think we can do the equivalent of --no-primer. The trick to avoiding multiple passes is for the option to exist outside of the set of primer'd options. I can think of two places:

  1. an environment variable, GIT_PRIMER. E.g., "GIT_PRIMER=0 git diff".
     This strikes me as hack-ish and unfriendly, but would work.

  2. in the git option list, but not the git command option list. IOW, we
     actually have two sets of options in any command:

        git [options] diff [options]

     So you could suppress primers (all of them, including diff.primer
     or primer.*) via:

       git --no-primer diff

Make sense?

-Peff

Hi,

On Fri, 13 Feb 2009, Jeff King wrote:

FWIW I cannot stop to hate the term "primer" for this thing.

A "primer" has been explained to me as being a short leaflet that introduces you to the basic principles of something. Or as a sequence of RNA (or in rare cases, not RNA but something similar) that starts replication of DNA.

IOW a primer is something that _has_ to come before something else. Without which the latter thing does not work.

Not a set of defaults, which this here patch is all about.

Ciao, Dscho

On Sat, Feb 14, 2009 at 07:03:26AM +0100, Johannes Schindelin wrote:

Yes, I also hate the name. I have been using it in the discussion for lack of a better term, but I really dislike it, as well. I think "diff.defaults" makes sense, but maybe something that suggests it is about command line options would be better.

-Peff

Hi,

On Sat, 14 Feb 2009, Jeff King wrote:

Why not spell it out?  diff.defaultOptions

Ciao, Dscho

On Sat, Feb 14, 2009 at 07:24:54AM +0100, Johannes Schindelin wrote:

Yes, that is much better than my suggestion.

-Peff

On Sat, 14 Feb 2009, Johannes Schindelin wrote:

OK in v3 I'll use "defaultOptions".  Now what do we call the switches?

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

ignore-default?

Peff,

On Fri, 13 Feb 2009, Jeff King wrote:

I like the idea of using parse-options to handle diff options and I too would like all switches negatable. I will come back to the other ideas you mention if necessary. You laid it all out nicely.

Assuming we can do away with the switches --[no-]default-options, thereby hopefully eliminating the need to accumulate options in any kind of fancy way, certainly the right place to "walk" is in diff_setup(). But diff_setup() must still ascertain at least one runtime fact: whether or not we are running one of the commands that respects default options {diff, log, show}. Is there an elegant way to ascertain that fact from inside diff_setup()? How do you recommend? (BTW I believe my design achieves this elegantly).

                                       -- Keith

On Mon, Feb 16, 2009 at 11:24:33PM -0800, Keith Cascio wrote:

If you are interested in parse-optification of diff options, search the archive for messages from Pierre Habouzit on the topic in the last 6 months or so. It was discussed at the GitTogether, and he had some preliminary patches.

You can impact the argument parsing by touching the diffopt struct before doing the parsing. I.e., something like:

  /* we generally get diff options from a rev_info structure */
  struct rev_info rev;
  /* initialize the structures */
  init_revisions(&rev, prefix);
  /* now set any preferences specific to this command */
  DIFF_OPT_SET(&rev.diffopt, ALLOW_DEFAULT_CONFIG);
  /* and then actually parse */
  setup_revisions(argc, argv, rev, "HEAD");

See for example how cmd_whatchanged does it in builtin-log.c. Any porcelains which wanted this feature would opt in to it.

-Peff

Peff,

I'm replying to http://permalink.gmane.org/gmane.comp.version-control.git/108158

On Tue, 3 Feb 2009, Jeff King wrote:

In order to answer your questions as convincingly as possible, I wrote up a 
one-page PDF document, downloadable here:
                http://preview.tinyurl.com/c769dd

You will see I clarified my arguments, and I found very compelling reasons for my design. Also, BTW, v3 supports all diff options under the sun, instead of a limited subset. That addresses your primary complaint WRT functionality. Please take a look at the PDF and I hope you agree.

                                 -- Keith

On Tue, Mar 17, 2009 at 09:05:18AM -0700, Keith Cascio wrote:

Thanks for following up on this. And I appreciate that it is probably nicer to compose in whatever you used to make this PDF due to enhanced typography and linked footnotes, but posting a link to a PDF has a few downsides:

  - the text in the PDF does not become part of the list archive; if
    your tinyurl or your hosted file ever goes away, the content of
    your message is permanently lost

  - it makes it very difficult for readers to reply inline to your
    comments

So please just send text emails in the future.

This time, however, I converted your PDF to text so I could reply.

Your PDF said:

OK, good, I think we are in agreement there.

I think there is a step 1.5, where callers can record their own "default" intentions -- things that this particular caller will default to, but which users can override via command-line options. E.g., "git log" tweaks several options in cmd_log_init, such as turning on ALLOW_TEXTCONV and RECURSIVE.

OK, makes sense.

More or less. I think in our past discussion, it came about that there are some things the user cannot say, like undoing certain options. This could be a problem if a caller defaults options to something un-doable. For example, I don't think there is a way to turn off OPT_RECURSIVE in git-log. In practice, this hasn't been a problem.

I don't agree that it is hopelessly flawed. It requires a new call at each callsite to say "I have set up my defaults, now take the user defaults from the config, before I proceed to step 2 and parse the user intentions". Which sounds awful to add a new call at each site, but I am not sure that is not necessary anyway.

I don't know that all callsites will _want_ to respect such a config option, especially not plumbing. So any callsite is going to have to opt into this functionality anyway.

I think I suggested last time that the idea of whether or not to perform the pre-load doesn't _have_ to come from the same set of user intention. That is, in the call

  git [git-options] diff [diff-options]

we actually parse [git-options] and [diff-options] at different times. Pre-load intention can go into [git-options], which avoids this dependency cycle.

Your poetry aside, I agree that way madness lies.

Because I outlined it above?

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.

  2. Fix existing locations by converting them to this infrastructure.

  3. Introduce some mechanism to help future callers get it right (since
     otherwise assigning directly is a subtle bug).

This is elementary encapsulation; in a language with better OO support, you would hide all of your struct members behind accessors. But this is C, and a dialect of C where that doesn't usually happen. So I think it is going to introduce a lot of code changes, and the resulting code will not look as much like the rest of git as it once did.

So what I am suggesting is that _if_ there is an easier way to do it, then it is worth exploring.

-Peff

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:

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.

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.

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

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

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.

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.

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

-Peff

Improve porcelain diff's accommodation of user preference by allowing any command-line option to (a) persist over all invocations and (b) stay consistent over multiple tools (e.g. command-line and gui). The approach taken here is good because it delivers the consistency a user expects without breaking any plumbing. It works by allowing the user, via git-config, to specify arbitrary options to pass to porcelain diff on every invocation, including internal invocations from other programs, e.g. git-gui.

Introduce diff command-line options --default-options and --no-default-options.

Affect only porcelain diff: we suppress default options for plumbing diff-{files,index,tree}, format-patch, and all other commands unless explicitly requested using --default-options (opt-in).

Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
---

Please notice v3 supports all diff options (improvement over v2).

This is a RFC.  I omitted the documentation and test portions for now.

                                    -- Keith

 diff.h         |   17 ++++++--
 diff.c         |  112 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 builtin-diff.c |    1 +
 builtin-log.c  |    1 +
 4 files changed, 125 insertions(+), 6 deletions(-)

The idea is from Keith Cascio.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	I do not particularly like what this patch does, but I like
	the non-intrusiveness and conciseness of it.

 Documentation/config.txt        |    4 ++++
 Documentation/git-diff.txt      |    3 +++
 builtin-diff.c                  |    1 +
 builtin-log.c                   |    4 ++++
 diff.c                          |   25 +++++++++++++++++++++++++
 diff.h                          |    2 ++
 t/t4037-diff-default-options.sh |   19 +++++++++++++++++++
 7 files changed, 58 insertions(+), 0 deletions(-)
 create mode 100755 t/t4037-diff-default-options.sh

Johannes,

On Sat, 21 Mar 2009, Johannes Schindelin wrote:

Your patch does not provide a command line opt_out flag. Let me describe a workflow situation and ask you how to handle it if the user were running your patch. Let diff.defaultOptions = "-b". The user is getting closer to submitting his patch and he wants to see patch output identical to what `git format-patch` will produce. What command should he use?

      `git format-patch --stdout master` ?

                                  -- Keith

Hi,

On Thu, 2 Apr 2009, Keith Cascio wrote:

The proper way would be to have options to _undo_ every diff option, I guess, as this would also help aliases in addition to defaultOptions.

In the case of format-patch, though, I am pretty certain that I do not want any diff.defaultOptions: the output is almost always intended for machine consumption, so it is a different kind of cattle.

Now, it is easy to put a patch on top of my patch to support something like --no-defaults.

Of course, to keep things simple, this has to be a separate patch.

Ciao, Dscho

On Thu, Apr 09, 2009 at 10:45:40AM +0200, Johannes Schindelin wrote:

I agree with this sentiment, no matter which approach is taken. I am more like to say "take my usual defaults, but tweak this one thing" than to say "turn off all of my defaults".

No, it's not. We went over this in great detail earlier in the thread. If you want:

  git diff --no-defaults

then you basically have to parse twice to avoid the chicken-and-egg problem. Which is why I suggested:

  git --no-defaults diff

which does work. Keith's solution does allow "git diff --no-defaults".

-Peff

Hi,

On Thu, 9 Apr 2009, Jeff King wrote:

So what? We parse the config a gazillion times, and there we have to access the _disk_.

Ciao, Dscho

On Thu, Apr 09, 2009 at 12:43:28PM +0200, Johannes Schindelin wrote:

But the first parse is only looking for "--no-defaults". So you need to:

  1. Understand the semantics of the other options to correctly parse
     around them (i.e., knowing which ones take arguments).

  2. Parse the arguments without actually respecting most of them, since
     they will be parsed again later.

This would actually be pretty easy if we had a declarative structure describing each option (like parseopt-ified options do). But the diff options are parsed by a big conditional statement.

Two ways to make it easier would be:

  1. You could loosen (1) above by assuming that --no-defaults will
     never appears as the argument to an option, and therefore any time
     you find it, it should be respected. Thus your first parse is just
     a simple loop looking for the option.

  2. You could loosen (2) above by assuming that all options are
     idempotent. I don't know whether this is the case (I think it
     isn't for all git options, but a cursory look shows that it may be
     for diff options).

-Peff

It would be desirable to undo every setting in diff.defaultOptions individually, but until there are options to reset every command line option, there is the "--no-defaults" option (which can be overridden by the "--defaults" option) to ignore the config setting.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

On Fri, 10 Apr 2009, Jeff King wrote:

I go with 1)

 builtin-diff.c                  |    2 +-
 builtin-log.c                   |    8 ++++----
 diff.c                          |   29 ++++++++++++++++++++++-------
 diff.h                          |    2 +-
 t/t4037-diff-default-options.sh |    5 +++++
 5 files changed, 33 insertions(+), 13 deletions(-)

On Tue, Apr 14, 2009 at 12:37:42AM +0200, Johannes Schindelin wrote:

This feels very hack-ish to me, but perhaps this is a case of "perfect is the enemy of the good".

-Peff

Hi,

On Thu, 16 Apr 2009, Jeff King wrote:

I have a strong feeling that none of our diff/rev options can sanely take a parameter looking like "--defaults" or "--no-defaults".

But I do not have the time to audit the options.  Maybe you have?

Ciao, Dscho

On Thu, Apr 16, 2009 at 11:25:08AM +0200, Johannes Schindelin wrote:

Right now, I think we are safe. A few options like "--default" do take a separated string argument, but saying "--default --no-defaults" seems a little crazy to me (besides being confusing because they are talking about two totally unrelated defaults).

Most of the string-taking options require --option=<arg> and don't support the separated version. If the code were ever parseopt-ified, they would start to support "--option <arg>"; however, at that time we should be able to write an "I know about these parseopt options, but please ignore them according to what we know about them taking an argument" function.

The one I would worry most about is "-S" since "-S--no-defaults" is a very reasonable thing to ask for. Right now its argument _must_ be connected. To be consistent with other git options, "-S --no-defaults" _should_ be the same thing. But we can perhaps ignore that because:

  1. That might never happen, because it breaks historical usage. IOW,
     it changes the meaning of "git log -S HEAD" to something else.

  2. If it does happen, it is likely to be in a transition to parseopt,
     which would fall under the case mentioned above.

I think the biggest danger is that it is a potential bomb for somebody to add a new revision option which takes an arbitrary string. They would probably need to keep it as "--option=<arg>" only.

-Peff

Jeff King <peff@peff.net> writes:

Maybe you guys have already considered and discarded this as too hacky, but isn't it the easiest to explain and code to declare --no-defaults is acceptable only at the beginning?

Hi,

On Thu, 16 Apr 2009, Junio C Hamano wrote:

That would not work if you use an alias:

	$ git config alias.grmpfl log --stat
	$ git grmpfl --no-defaults

Ciao, Dscho

On Thu, Apr 16, 2009 at 09:52:50AM -0700, Junio C Hamano wrote:

I discarded that as "too hacky". If I had to choose my poison between "insane string options don't work" and "option must inexplicably be at the front", I think I take the former. It is perhaps a more difficult rule to realize you are triggering, but it is much less likely to come up in practice.

But I think all of this is just ending up in the same place that Keith and I arrived at much earlier in the thread: you _are_ choosing a poison, and his patch was meant to avoid that. The question is whether the added code complexity is worth it.

-Peff

Hi,

On Fri, 17 Apr 2009, Jeff King wrote:

Well, I think I gave my answer in the form of two patches.

Besides, you still will have a poison:

	git config diff.defaultOptions --no-defaults

which is Russel's paradoxon right there.

Ciao, Dscho

Dscho,

On Fri, 17 Apr 2009, Johannes Schindelin wrote:

I can cleanly modify my v3 to handle this case. In diff_setup_done(), change this:

+ if (DIFF_OPT_TST(options, ALLOW_DEFAULT_OPTIONS)) + flatten_diff_options(options, defaults ? defaults : + parse_diff_defaults(diff_setup(defaults = + xmalloc(sizeof(struct diff_options)))));

to this:

+ if (DIFF_OPT_TST(options, ALLOW_DEFAULT_OPTIONS) && (defaults || + parse_diff_defaults(diff_setup(defaults = xmalloc( + sizeof(struct diff_options))))) && DIFF_OPT_TST( + defaults, ALLOW_DEFAULT_OPTIONS)) + flatten_diff_options(options, + defaults);

All I did there was add the test DIFF_OPT_TST(defaults, ALLOW_DEFAULT_OPTIONS) 
to the condition that controls whether to perform the flattening.  Clean and 
clear.
                                  -- Keith

Hi Keith,

On Sat, 18 Apr 2009, Keith Cascio wrote:

You cannot. --no-defaults means that diff.defaultOptions should be disregarded. If the diff.defaultOptions say that they should be disregarded themselves, then --no-defaults should be disregarded.

And I still do not like the intrusiveness of your patch. The last time we did something like that with options (some parseoptifications), we had a lot of fallout as a consequence.

Ciao, Dscho

Dscho,

On Sat, 18 Apr 2009, Johannes Schindelin wrote:

--no-defaults *could* mean as you say there. But a much better meaning for --no-defaults is: suppress the values in diff.defaultOptions after options processing. We don't have to disregard them, just suppress them after options processing. In that sense, --no-defaults is a meta-option. It is an option about options. Even users unfamiliar with set theory would assume the suppression semantics.

Nevertheless I applaud the Russell reference.  Very intriguing.

A reasonable worry!  But blind paranoia is paralyzing.  Peff expressed some 
specific concerns which he and I addressed: (1) whether I'd investigated all 
callsites for possible problems (yes I did), (2) whether we'd have to switch 
every callsite to a macro, rather than direct assignment (no we don't).  
Outside of diff.h/diff.c, my v3 deletes no lines and adds only two.  That 
doesn't really seem "intrusive" to me.  By comparison, your patch adds at least 
ten lines outside of diff.h/diff.c.  I'd rather call my patch "innovative".  
Possible?
                               -- Keith

Hi.

On Sat, 18 Apr 2009, Keith Cascio wrote:

My concerns were also very specific: your patches are way too large. There is a rule of thumb that the likelihood of a bug is the square of the number of changed lines, I am sure you heard that before.

That is _such_ a red herring.

Ciao, Dscho

Johannes,

On Thu, 9 Apr 2009, Johannes Schindelin wrote:

Just to clarify: I agree. I certainly would never want diff.defaultOptions to affect format-patch, and none of my patches did so. The reason I brought up format-patch is because, without an opt_out mechanism, it's harder for the user to use `git diff` to produce patch output identical to format-patch.

With all due respect sir, I think you would find that if you sit down and 
attempt to add such functionality on top of your version, it would be an 
unpleasant experience.  I predict the code would quickly turn inelegant and 
fragile.  I believe it would prompt you to consider a redesign (assuming you 
work and think quickly, after about 15 minutes), and the obvious solution would 
closely resemble my v3:
  http://comments.gmane.org/gmane.comp.version-control.git/114021

                             -- Keith

Johannes,

Hi.  Was my last message understandable or should I add more explanation?
  http://comments.gmane.org/gmane.comp.version-control.git/115506

                       -- Keith

Hi,

On Wed, 8 Apr 2009, Keith Cascio wrote:

Sorry, I am just short on time for that hobby called Git.

Ciao, Dscho

On Wed, Apr 08, 2009 at 05:44:29PM -0700, Keith Cascio wrote:

Your point made sense to me, and is the expected difference between the two approaches (as we discussed before).

I'm sorry I haven't had a chance to review your patch in detail. It is pleasantly much shorter than previous iterations, but I wanted to very carefully check a few of the diff callsites to make sure they work properly (e.g., some of the cases I laid out earlier in the thread).

I have a lot of regular life responsibilities right now, but I'll try take a close look this weekend.

-Peff

Keith Cascio <keith@cs.ucla.edu> writes:

I still think that naming this configuration variable (or configuration section in the reverse primer.diff) 'primer' is not a good idea, because it is quite obscure and not well known word. In computer related context I have seen it only when talking about introductory / novice / basic level documentation ('primer (textbook)' meaning). Git user, who might be not a native English speaker, shouldn't have to look up in dictionary what it is about...

I think that 'defaults' or 'options' would be a much better name.

On Tue, 3 Feb 2009, Jakub Narebski wrote:

Point well taken. Let's consider "primer" the working name. The final name will be the last finishing touch once (and if) we achieve consensus on the functionality. Ultimately the name is best chosen after the functionality is certain. IOW, open to discussion.

                                     -- Keith