Re: [PATCH v2 10/15] diff: stop allowing diff to have submodules configured in .git/config

[Brandon Williams <bmwill@google.com>]

On 08/03, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > Traditionally a submodule is comprised of a gitlink as well as a
> > corresponding entry in the .gitmodules file.  Diff doesn't follow this
> > paradigm as its config callback routine falls back to populating the
> > submodule-config if a config entry starts with 'submodule.'.
> >
> > Remove this behavior in order to be consistent with how the
> > submodule-config is populated, via calling 'gitmodules_config()' or
> > 'repo_read_gitmodules()'.
> 
> I am all for dropping special cases deep in the diff machinery, even
> though there may be submodule users who care about submodule.*.ignore
> 
> Does this change mean we can eventually get rid of the ugly
> DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG hack and also need for a patch
> like 03/15?

I think that this is a step toward getting rid of that.  We can either
do two things: 1) deprecate submodule.*.ignore and don't respect it
anymore or 2) flip the polarity of that flag so that by default we
don't respect the submodule.*.ignore config and instead callers must opt
in instead of the current opt out behavior.

> 
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  diff.c                    |  3 ---
> >  t/t4027-diff-submodule.sh | 67 -----------------------------------------------
> >  2 files changed, 70 deletions(-)
> >
> > diff --git a/diff.c b/diff.c
> > index 85e714f6c..e43519b88 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -346,9 +346,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
> >  		return 0;
> >  	}
> >  
> > -	if (starts_with(var, "submodule."))
> > -		return parse_submodule_config_option(var, value);
> > -
> >  	if (git_diff_heuristic_config(var, value, cb) < 0)
> >  		return -1;
> >  
> > diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
> > index 518bf9524..2ffd11a14 100755
> > --- a/t/t4027-diff-submodule.sh
> > +++ b/t/t4027-diff-submodule.sh
> > @@ -113,35 +113,6 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)'
> >  	! test -s actual4
> >  '
> >  
> > -test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.git/config]' '
> > -	git config diff.ignoreSubmodules all &&
> > -	git diff HEAD >actual &&
> > -	! test -s actual &&
> > -	git config submodule.subname.ignore none &&
> > -	git config submodule.subname.path sub &&
> > -	git diff HEAD >actual &&
> > -	sed -e "1,/^@@/d" actual >actual.body &&
> > -	expect_from_to >expect.body $subprev $subprev-dirty &&
> > -	test_cmp expect.body actual.body &&
> > -	git config submodule.subname.ignore all &&
> > -	git diff HEAD >actual2 &&
> > -	! test -s actual2 &&
> > -	git config submodule.subname.ignore untracked &&
> > -	git diff HEAD >actual3 &&
> > -	sed -e "1,/^@@/d" actual3 >actual3.body &&
> > -	expect_from_to >expect.body $subprev $subprev-dirty &&
> > -	test_cmp expect.body actual3.body &&
> > -	git config submodule.subname.ignore dirty &&
> > -	git diff HEAD >actual4 &&
> > -	! test -s actual4 &&
> > -	git diff HEAD --ignore-submodules=none >actual &&
> > -	sed -e "1,/^@@/d" actual >actual.body &&
> > -	expect_from_to >expect.body $subprev $subprev-dirty &&
> > -	test_cmp expect.body actual.body &&
> > -	git config --remove-section submodule.subname &&
> > -	git config --unset diff.ignoreSubmodules
> > -'
> > -
> >  test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules]' '
> >  	git config diff.ignoreSubmodules dirty &&
> >  	git diff HEAD >actual &&
> > @@ -208,24 +179,6 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'
> >  	! test -s actual4
> >  '
> >  
> > -test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.git/config]' '
> > -	git config submodule.subname.ignore all &&
> > -	git config submodule.subname.path sub &&
> > -	git diff HEAD >actual2 &&
> > -	! test -s actual2 &&
> > -	git config submodule.subname.ignore untracked &&
> > -	git diff HEAD >actual3 &&
> > -	! test -s actual3 &&
> > -	git config submodule.subname.ignore dirty &&
> > -	git diff HEAD >actual4 &&
> > -	! test -s actual4 &&
> > -	git diff --ignore-submodules=none HEAD >actual &&
> > -	sed -e "1,/^@@/d" actual >actual.body &&
> > -	expect_from_to >expect.body $subprev $subprev-dirty &&
> > -	test_cmp expect.body actual.body &&
> > -	git config --remove-section submodule.subname
> > -'
> > -
> >  test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' '
> >  	git config --add -f .gitmodules submodule.subname.ignore all &&
> >  	git config --add -f .gitmodules submodule.subname.path sub &&
> > @@ -261,26 +214,6 @@ test_expect_success 'git diff between submodule commits' '
> >  	! test -s actual
> >  '
> >  
> > -test_expect_success 'git diff between submodule commits [.git/config]' '
> > -	git diff HEAD^..HEAD >actual &&
> > -	sed -e "1,/^@@/d" actual >actual.body &&
> > -	expect_from_to >expect.body $subtip $subprev &&
> > -	test_cmp expect.body actual.body &&
> > -	git config submodule.subname.ignore dirty &&
> > -	git config submodule.subname.path sub &&
> > -	git diff HEAD^..HEAD >actual &&
> > -	sed -e "1,/^@@/d" actual >actual.body &&
> > -	expect_from_to >expect.body $subtip $subprev &&
> > -	test_cmp expect.body actual.body &&
> > -	git config submodule.subname.ignore all &&
> > -	git diff HEAD^..HEAD >actual &&
> > -	! test -s actual &&
> > -	git diff --ignore-submodules=dirty HEAD^..HEAD >actual &&
> > -	sed -e "1,/^@@/d" actual >actual.body &&
> > -	expect_from_to >expect.body $subtip $subprev &&
> > -	git config --remove-section submodule.subname
> > -'
> > -
> >  test_expect_success 'git diff between submodule commits [.gitmodules]' '
> >  	git diff HEAD^..HEAD >actual &&
> >  	sed -e "1,/^@@/d" actual >actual.body &&

-- 
Brandon Williams