[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 --- builtin/diff-files.c | 2 +- builtin/diff-index.c | 2 +- builtin/diff-tree.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) 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); diff --git a/builtin/diff-index.c b/builtin/diff-index.c index 1af373d00..f084826a2 100644 --- a/builtin/diff-index.c +++ b/builtin/diff-index.c @@ -17,9 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) int i; int result; + 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); diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c index 326f88b65..36a3a1976 100644 --- a/builtin/diff-tree.c +++ b/builtin/diff-tree.c @@ -105,9 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix) struct setup_revision_opt s_r_opt; int read_stdin = 0; + git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ init_revisions(opt, prefix); gitmodules_config(); - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ opt->abbrev = 0; opt->diff = 1; opt->disable_stdin = 1; -- 2.13.0.rc1.15.g7dbea34e1.dirty
Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.
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 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
Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.
Jeff King writes: > 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 > > 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. > >... > >> +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); This somehow made me worried at the first glance, but the matches what is done by the Porcelain "git diff". In other words, it was a bug that the original called init_revisions() before doing the diff configuration, and it does not have much to do with "now let's have them honor indentHeuristic". Even if we wanted to keep the indent heuristic in the ui-config (not basic) section of the diff tweak knobs, we should be applying this patch as a bugfix. > 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. Yeah. The underlying diff machinery was built to be easily usable by revision traversal and that is probably the reason why we have this entanglement that we probably could (and maybe we would want to) detangle. Surely not a theme of this topic. Thanks.
Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.
On Sun, Apr 30, 2017 at 06:01:37PM -0700, Junio C Hamano wrote: > >> + 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); > > This somehow made me worried at the first glance, but the matches > what is done by the Porcelain "git diff". In other words, it was a > bug that the original called init_revisions() before doing the diff > configuration, and it does not have much to do with "now let's have > them honor indentHeuristic". Even if we wanted to keep the indent > heuristic in the ui-config (not basic) section of the diff tweak > knobs, we should be applying this patch as a bugfix. Yeah, I agree it is an existing bug. The only other case I found is dirstat. Doing: mkdir a b for i in 1 2; do echo content >a/$i; done for i in 1 2 3; do echo content >b/$i; done git -c diff.dirstat=50 diff-tree --dirstat --root HEAD should respect the config and show only "b", but it doesn't currently (and does with Marc's patch). -Peff
Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.
On Mon, May 01, 2017 at 01:17:58AM -0400, Jeff King wrote: > Yeah, I agree it is an existing bug. The only other case I found is > dirstat. Doing: > > mkdir a b > for i in 1 2; do echo content >a/$i; done > for i in 1 2 3; do echo content >b/$i; done > git -c diff.dirstat=50 diff-tree --dirstat --root HEAD > > should respect the config and show only "b", but it doesn't currently > (and does with Marc's patch). Er, there's a "git add a b; git commit -m foo" missing before running the diff, of course. -Peff