Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.
- From
Jeff King <peff@peff.net>
- Date
- Apr 28, 2017, 08:06 UTC
- Message-ID
- <20170428080620.cbnreh5invwpasxr@sigill.intra.peff.net>
- In-Reply-To
- <20170427205037.1787-3-marcnarc@xiplink.com>
On Thu, Apr 27, 2017 at 04:50:37PM -0400, Marc Branchaud wrote:
Show 6 quoted lines
> 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 <marcnarc@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.
Show 14 quoted lines
> 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