On Thu, Apr 27, 2017 at 04:50:35PM -0400, Marc Branchaud wrote:

So here's my attempt at fixing this.

The thing I was missing is that init_revisions() calls diff_setup(), which
sets the xdl options.  It's therefore necessary to have the
diff_indent_heuristic flag set before calling init_revisions().

A naive way to get the indentHeuristic config option respected in the
diff-* plumbing commands is to make them use the git_diff_heuristic_config()
callback right at the start of their main cmd functions.

But I did not like that for two reasons:

* It would make these commands invoke git_config() twice.

* It doesn't avoid the problem if/when someone creates a new diff-something
  plumbing command, and forgets to set the diff_indent_heuristic flag before
  calling init_revisions().

Yeah, I think that would be the wrong way to go. Either this option is
diff_basic_config() or it is not. And if it is not, the plumbing commands
have no business checking it.

Thinking on it more, I think it probably should be basic config. We
discussed all along making these options the default, which shows that
plumbing interfaces do not need to be protected from them.

So instead I chose to make the indentHeuristic option part of diff's basic
configuration, and in each of the diff plumbing commands I moved the call to
git_config() before the call to init_revisions().

Yes, I think that's the right thing to do.

This still doesn't really future-proof things for possible new diff plumbing
commands, because someone could still invoke init_revisions() before setting
up diff's basic configuration.  But I don't see an obvious way of ensuring
that the diff_indent_heuristic flag is respected regardless of when
diff_setup() is invoked.

I think the config-based stuff would have to be bumped down to
setup_revisions(), leaving init_revisions() as a initialization
function. But that would break new cases, as some callers would
want to do:

  init_revisions(&revs);
  revs.foo = 1; /* override any config */
  setup_revisions(argc, argv, &revs); /* let command-line override us */

So you'd really need three phases:

  init_revisions(&revs); /* baked-in defaults */
  setup_revision_config(&revs);
  setup_revision_args(argc, argv, &revs);

and you could override each set of defaults in between the calls.  But
that would require tweaking each caller. In practice, I think the rule
"set up your config before calling init_revisions" is probably OK, now
that we know about it.

I think there is existing breakage in any case where a diff_basic_config
option is handled in diff_setup(). The only one I see is the dirstat
stuff. So I think:

  git config diff.dirstat changes,5
  git rev-list HEAD | git diff-tree --stdin --dirstat

was supposed to respect that "5" but doesn't. I didn't test, though.

-Peff