Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.

[Jeff King]

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

Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.

[Jeff King]

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(, 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.

[Junio C Hamano]

Jeff King <p...@peff.net> 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 <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.
>
>...
>
>> +git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
>>  init_revisions(, 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.

[Jeff King]

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(, 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

[PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.

[Marc Branchaud]

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(, 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(, 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