On Thu, Apr 27, 2017 at 04:50:37PM -0400, Marc Branchaud wrote: > Subject: [PATCH 2/2] Have the diff-* builtins configure diff before > initializing revisions. > > This makes the commands respect diff configuration options, such as > indentHeuristic. > > Signed-off-by: Marc Branchaud <marcn...@xiplink.com>
I think it would be helpful to future readers to explain what is going on here. I.e., the bit about calling diff_setup_done(), which copies the globals into the diff struct. The same comments about the subject line from the last patch apply here, too. > builtin/diff-files.c | 2 +- > builtin/diff-index.c | 2 +- > builtin/diff-tree.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) It would be nice to have a test. Testing that dirstat's permille option has an effect might be the easiest way to do so. > diff --git a/builtin/diff-files.c b/builtin/diff-files.c > index 15c61fd8d..a572da9d5 100644 > --- a/builtin/diff-files.c > +++ b/builtin/diff-files.c > @@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char > *prefix) > int result; > unsigned options = 0; > > + git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ > init_revisions(&rev, prefix); > gitmodules_config(); > - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ > rev.abbrev = 0; > precompose_argv(argc, argv); It's somewhat odd to me that diff_files uses a rev_info struct at all. It doesn't traverse at all, and doesn't respect most of the options. There's a half-hearted attempt to reject some obviously bogus cases, but most of the options are just silently ignored. I think it's mostly a historical wart (especially around the fact that some diff options like combine_merges are in rev_info, which they probably should not be). Anyway, none of that is your problem, and is way outside the scope of this patch. -Peff
