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