diff: do not show submodule with untracked files as "-dirty"

From: sangu09 <[email protected]>

Git diff reports a submodule directory as -dirty even when there are only untracked files in the submodule directory. This is inconsistent with what git describe --dirty says when run in the submodule directory in that state.

So this patch makes git diff HEAD consistent with git describe --dirty in case of untracked files. This can be solved by including the "--ignore-submodules=untracked" feature in git diff by default. So in order to make this as default behaviour I have added the behaviour of ignore-submodules in repo_diff_setup() function.

Also, to avoid ignoreSubmodules in user config being overwritten, I have made a global variable >diff_ignore_submodule_config to keep a track whether handle_ignore_submodules_arg is called before or not.

Signed-off-by Sangeeta Jain <[email protected]>

---
[Outreachy] diff: do not show submodule with untracked files as "-dirty"

Git diff reports a submodule directory as -dirty even when there are only untracked files in the submodule directory. This is inconsistent with what git describe --dirty says when run in the submodule directory in that state.

So this patch makes git diff HEAD consistent with git describe --dirty in case of untracked files. This can be solved by including the "--ignore-submodules=untracked" feature in git diff by default. So in order to make this as default behaviour I have added the behaviour of ignore-submodules in repo_diff_setup() function. Also, to avoid ignoreSubmodules in user config being overwritten, I have made a global variable diff_ignore_submodule_config to keep a track whether handle_ignore_submodules_arg is called before or not.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-751%2Fsangu09%2Fhandle_untracked-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-751/sangu09/handle_untracked-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/751

  diff.c                                       |  1 +
  submodule.c                                  |  2 ++
  t/t3701-add-interactive.sh                   |  2 +-
  t/t4027-diff-submodule.sh                    |  6 ++--
  t/t4041-diff-submodule-option.sh             | 16 ++++-----
  t/t4060-diff-submodule-option-diff-format.sh | 16 ++++-----
  t/t7064-wtstatus-pv2.sh                      |  8 ++---
  t/t7506-status-submodule.sh                  | 38 +++++---------------
  8 files changed, 36 insertions(+), 53 deletions(-)

diff.c +0 -0

index 2bb2f8f57e..07d89e3e2b 100644

submodule.c +0 -0

  	prepare_submodule_repo_env(&cp.env_array);
  	cp.git_cmd = 1;

t/t3701-add-interactive.sh +1 -1

index ca04fac417..98e46ad1ae 100755
@@ -761,7 +761,7 @@ test_expect_success 'setup different kinds of dirty submodules' '
  		echo dirty >>initial &&
  		: >untracked
  	) &&
-	git -C for-submodules diff-files --name-only >actual &&
+	git -C for-submodules diff-files --name-only --ignore-submodules=none >actual &&
  	cat >expected <<-\EOF &&
  	dirty-both-ways
  	dirty-head

t/t4027-diff-submodule.sh +1 -1

index d7145ccca4..7bf2cb9c88 100755
@@ -93,7 +93,7 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked)' '
  	) &&
  	git diff HEAD >actual &&
  	sed -e "1,/^@@/d" actual >actual.body &&
-	expect_from_to >expect.body $subtip $subprev-dirty &&
+	expect_from_to >expect.body $subtip $subprev &&
  	test_cmp expect.body actual.body
  '
 

@@ -168,13 +168,13 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'
  		git clean -qfdx &&
  		>cruft

t/t4027-diff-submodule.sh +0 -0

  	test_must_be_empty actual3 &&
  	git diff --ignore-submodules=dirty HEAD >actual4 &&
  	test_must_be_empty actual4

t/t4041-diff-submodule-option.sh +8 -8

index f852136585..bb368b685d 100755
@@ -262,7 +262,7 @@ test_expect_success 'submodule is up to date' '
 
  test_expect_success 'submodule contains untracked content' '
  	echo new > sm1/new-file &&
-	git diff-index -p --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD --ignore-submodules=none >actual &&
  	cat >expected <<-EOF &&
  	Submodule sm1 contains untracked content
  	EOF

@@ -270,7 +270,7 @@ test_expect_success 'submodule contains untracked content' '
  '
 
  test_expect_success 'submodule contains untracked content (untracked ignored)' '
-	git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD >actual &&
  	test_must_be_empty actual
  '
 

@@ -286,7 +286,7 @@ test_expect_success 'submodule contains untracked content (all ignored)' '
 
  test_expect_success 'submodule contains untracked and modified content' '
  	echo new > sm1/foo6 &&
-	git diff-index -p --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD --ignore-submodules=none >actual &&
  	cat >expected <<-EOF &&
  	Submodule sm1 contains untracked content
  	Submodule sm1 contains modified content

@@ -296,7 +296,7 @@ test_expect_success 'submodule contains untracked and modified content' '
 
  test_expect_success 'submodule contains untracked and modified content (untracked ignored)' '
  	echo new > sm1/foo6 &&
-	git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD >actual &&
  	cat >expected <<-EOF &&
  	Submodule sm1 contains modified content
  	EOF

@@ -337,7 +337,7 @@ test_expect_success 'submodule is modified' '
 
  test_expect_success 'modified submodule contains untracked content' '
  	echo new > sm1/new-file &&
-	git diff-index -p --submodule=log HEAD >actual &&
+	git diff-index -p  --ignore-submodules=none --submodule=log HEAD >actual &&
  	cat >expected <<-EOF &&
  	Submodule sm1 contains untracked content
  	Submodule sm1 $head6..$head8:

@@ -347,7 +347,7 @@ test_expect_success 'modified submodule contains untracked content' '
  '
 
  test_expect_success 'modified submodule contains untracked content (untracked ignored)' '
-	git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD >actual &&
  	cat >expected <<-EOF &&
  	Submodule sm1 $head6..$head8:
  	  > change

@@ -371,7 +371,7 @@ test_expect_success 'modified submodule contains untracked content (all ignored)
 
  test_expect_success 'modified submodule contains untracked and modified content' '
  	echo modification >> sm1/foo6 &&
-	git diff-index -p --submodule=log HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=log HEAD >actual &&
  	cat >expected <<-EOF &&
  	Submodule sm1 contains untracked content
  	Submodule sm1 contains modified content

@@ -383,7 +383,7 @@ test_expect_success 'modified submodule contains untracked and modified content'
 
  test_expect_success 'modified submodule contains untracked and modified content (untracked ignored)' '
  	echo modification >> sm1/foo6 &&
-	git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD >actual &&
  	cat >expected <<-EOF &&
  	Submodule sm1 contains modified content
  	Submodule sm1 $head6..$head8:

t/t4060-diff-submodule-option-diff-format.sh +7 -7

index fc8229c726..dc7b242697 100755
@@ -409,7 +409,7 @@ test_expect_success 'submodule is up to date' '
 
  test_expect_success 'submodule contains untracked content' '
  	echo new > sm1/new-file &&
-	git diff-index -p --submodule=diff HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
  	cat >expected <<-EOF &&
  	Submodule sm1 contains untracked content
  	EOF

@@ -417,7 +417,7 @@ test_expect_success 'submodule contains untracked content' '
  '
 
  test_expect_success 'submodule contains untracked content (untracked ignored)' '
-	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	git diff-index -p --submodule=diff HEAD >actual &&
  	test_must_be_empty actual
  '
 

@@ -433,7 +433,7 @@ test_expect_success 'submodule contains untracked content (all ignored)' '
 
  test_expect_success 'submodule contains untracked and modified content' '
  	echo new > sm1/foo6 &&
-	git diff-index -p --submodule=diff HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
  	cat >expected <<-EOF &&
  	Submodule sm1 contains untracked content
  	Submodule sm1 contains modified content

@@ -451,7 +451,7 @@ test_expect_success 'submodule contains untracked and modified content' '
  # NOT OK
  test_expect_success 'submodule contains untracked and modified content (untracked ignored)' '
  	echo new > sm1/foo6 &&
-	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	git diff-index -p --submodule=diff HEAD >actual &&
  	cat >expected <<-EOF &&
  	Submodule sm1 contains modified content
  	diff --git a/sm1/foo6 b/sm1/foo6

@@ -512,7 +512,7 @@ test_expect_success 'submodule is modified' '
 
  test_expect_success 'modified submodule contains untracked content' '
  	echo new > sm1/new-file &&
-	git diff-index -p --submodule=diff HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
  	cat >expected <<-EOF &&
  	Submodule sm1 contains untracked content
  	Submodule sm1 $head7..$head8:

@@ -528,7 +528,7 @@ test_expect_success 'modified submodule contains untracked content' '
  '
 
  test_expect_success 'modified submodule contains untracked content (untracked ignored)' '
-	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	git diff-index -p --submodule=diff HEAD >actual &&
  	cat >expected <<-EOF &&
  	Submodule sm1 $head7..$head8:
  	diff --git a/sm1/foo6 b/sm1/foo6

@@ -564,7 +564,7 @@ test_expect_success 'modified submodule contains untracked content (all ignored)
 
  test_expect_success 'modified submodule contains untracked and modified content' '
  	echo modification >> sm1/foo6 &&
-	git diff-index -p --submodule=diff HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
  	cat >expected <<-EOF &&
  	Submodule sm1 contains untracked content
  	Submodule sm1 contains modified content

t/t7064-wtstatus-pv2.sh +2 -2

  	(	cd super_repo &&
  		## create untracked file in the submodule.
  		(	cd sub1 &&

@@ -500,7 +500,7 @@ test_expect_success 'untracked changes in added submodule (AM S..U)' '
  		# branch.upstream origin/master
  		# branch.ab +0 -0
  		1 A. N... 000000 100644 100644 $ZERO_OID $HMOD .gitmodules
-		1 AM S..U 000000 160000 160000 $ZERO_OID $HSUB sub1
+		1 A. S... 000000 160000 160000 $ZERO_OID $HSUB sub1
  		EOF
 
  		git status --porcelain=v2 --branch --untracked-files=all >actual &&

@@ -560,7 +560,7 @@ test_expect_success 'staged and unstaged changes in added (AM S.M.)' '
  	)
  '
 
-test_expect_success 'staged and untracked changes in added submodule (AM S.MU)' '
+test_expect_success 'staged and untracked changes in added submodule (AM S.M.)' '
  	(	cd super_repo &&
  		(	cd sub1 &&
  			## stage new changes in tracked file.

t/t7064-wtstatus-pv2.sh +0 -0

  		git status --porcelain=v2 --branch --untracked-files=all >actual &&

t/t7506-status-submodule.sh +9 -28

index 3fcb44767f..b7ff7928fb 100755
@@ -95,7 +95,7 @@ test_expect_success 'status with untracked file in submodule' '
  	(cd sub && git reset --hard) &&
  	echo "content" >sub/new-file &&
  	git status >output &&
-	test_i18ngrep "modified:   sub (untracked content)" output
+	test_i18ngrep "^nothing to commit" output
  '
 
  test_expect_success 'status -uno with untracked file in submodule' '

@@ -105,23 +105,19 @@ test_expect_success 'status -uno with untracked file in submodule' '
 
  test_expect_success 'status with untracked file in submodule (porcelain)' '
  	git status --porcelain >output &&
-	diff output - <<-\EOF
-	 M sub
-	EOF
+	test_must_be_empty output
  '
 
  test_expect_success 'status with untracked file in submodule (short)' '
  	git status --short >output &&
-	diff output - <<-\EOF
-	 ? sub
-	EOF
+	test_must_be_empty output
  '
 
  test_expect_success 'status with added and untracked file in submodule' '
  	(cd sub && git reset --hard && echo >foo && git add foo) &&
  	echo "content" >sub/new-file &&
  	git status >output &&
-	test_i18ngrep "modified:   sub (modified content, untracked content)" output
+	test_i18ngrep "modified:   sub (modified content)" output
  '
 
  test_expect_success 'status with added and untracked file in submodule (porcelain)' '

@@ -169,7 +165,7 @@ test_expect_success 'status with untracked file in modified submodule' '
  	(cd sub && git reset --hard) &&
  	echo "content" >sub/new-file &&
  	git status >output &&
-	test_i18ngrep "modified:   sub (new commits, untracked content)" output
+	test_i18ngrep "modified:   sub (new commits)" output
  '
 
  test_expect_success 'status with untracked file in modified submodule (porcelain)' '

@@ -183,7 +179,7 @@ test_expect_success 'status with added and untracked file in modified submodule'
  	(cd sub && git reset --hard && echo >foo && git add foo) &&
  	echo "content" >sub/new-file &&
  	git status >output &&
-	test_i18ngrep "modified:   sub (new commits, modified content, untracked content)" output
+	test_i18ngrep "modified:   sub (new commits, modified content)" output
  '
 
  test_expect_success 'status with added and untracked file in modified submodule (porcelain)' '

@@ -350,30 +346,17 @@ test_expect_success 'setup superproject with untracked file in nested submodule'
 
  test_expect_success 'status with untracked file in nested submodule (porcelain)' '
  	git -C super status --porcelain >output &&
-	diff output - <<-\EOF
-	 M sub1
-	 M sub2
-	 M sub3
-	EOF
+	test_must_be_empty output
  '
 
  test_expect_success 'status with untracked file in nested submodule (porcelain=2)' '
  	git -C super status --porcelain=2 >output &&
-	sanitize_output output &&
-	diff output - <<-\EOF
-	1 .M S..U 160000 160000 160000 HASH HASH sub1
-	1 .M S..U 160000 160000 160000 HASH HASH sub2
-	1 .M S..U 160000 160000 160000 HASH HASH sub3
-	EOF
+	test_must_be_empty output
  '
 
  test_expect_success 'status with untracked file in nested submodule (short)' '
  	git -C super status --short >output &&
-	diff output - <<-\EOF
-	 ? sub1
-	 ? sub2
-	 ? sub3
-	EOF
+	test_must_be_empty output
  '
 
  test_expect_success 'setup superproject with modified file in nested submodule' '

@@ -386,7 +369,6 @@ test_expect_success 'status with added file in nested submodule (porcelain)' '
  	diff output - <<-\EOF
  	 M sub1
  	 M sub2
-	 M sub3
  	EOF
  '
 

@@ -396,7 +378,6 @@ test_expect_success 'status with added file in nested submodule (porcelain=2)' '
  	diff output - <<-\EOF
  	1 .M S.M. 160000 160000 160000 HASH HASH sub1
  	1 .M S.M. 160000 160000 160000 HASH HASH sub2
-	1 .M S..U 160000 160000 160000 HASH HASH sub3
  	EOF
  '
 

base-commit: d98273ba77e1ab9ec755576bc86c716a97bf59d7

I think we require your full name on the From line to match the Signed-off-by line below (c.f.
https://lore.kernel.org/git/[email protected])

That is a good summary of the issue that this change addresses, we normally wrap lines at 72 characters though.

This unconditionally overrides diff.ignoreSubmodules, grepping seems to show that we don't have any tests that test a config value of "none". There are a few that check "dirty" which we should look at to consider if they still add value now we've changed the default.

      if (diff_no_prefix) {
              options->a_prefix = options->b_prefix = "";

submodule.c +2 -0

index b3bb59f066..762298c010 100644
@@ -1678,6 +1678,8 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
      strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
      if (ignore_untracked)
              strvec_push(&cp.args, "-uno");
+     else
+        strvec_push (&cp.args, "--ignore-submodules=none");
 

We need to do this as a consequence of changing the default for '--ignore-submodules, we should think what the consequences are for other users of git status` and whether we need to do something similar there if diff.ignoreSubmodules is not set.

I think we want to keep testing that we get the correct output when --ignore-submodules=none as well as maybe adding a couple of new tests that check the default in this file, rather than changing the expected output.

Hey Phillip,

Quoted 3 lines

I think we require your full name on the From line to match the Signed-off-by line below (c.f.
https://lore.kernel.org/git/[email protected])

Quoted 2 lines

That is a good summary of the issue that this change addresses, we normally wrap lines at 72 characters though.

This was sent by gitgitgadget so I am unable to find how I can customize it. Can you help me with this? Or else I have set the Travis so should I send another patch using send-email?

Quoted 4 lines

This unconditionally overrides diff.ignoreSubmodules, grepping seems to show that we don't have any tests that test a config value of "none". There are a few that check "dirty" which we should look at to consider if they still add value now we've changed the default.

Yes, sorry, noticed it now. I have now added a flag in diff_flags. Also, do I have to add tests that check whether diff.ignoreSubmodules is not being overwritten?

Quoted 14 lines

       if (diff_no_prefix) {
               options->a_prefix = options->b_prefix = "";

submodule.c +2 -0

index b3bb59f066..762298c010 100644
@@ -1678,6 +1678,8 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
       strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
       if (ignore_untracked)
               strvec_push(&cp.args, "-uno");
+     else
+        strvec_push (&cp.args, "--ignore-submodules=none");
 

Quoted 5 lines

We need to do this as a consequence of changing the default for '--ignore-submodules, we should think what the consequences are for other users of git status` and whether we need to do something similar there if diff.ignoreSubmodules is not set.

Oh okay. I understood what you said. But I couldn't figure out how to do that.

As all the tests of status were passing so I don't think there might be any issue with this.

Quoted 5 lines

I think we want to keep testing that we get the correct output when --ignore-submodules=none as well as maybe adding a couple of new tests that check the default in this file, rather than changing the expected output.

Thanks, I have added more tests.

Thanks and Regards,
Sangeeta

Again, it's a judgment call whether to go with modern style or follow existing style of the file.

Another option is to have a preparatory patch which first modernizes the script, and then your new tests would follow modern style. But, that may be outside of scope of your submission.

To summarize: The only really actionable review comments are the minor style nits in the C code. The nits about style issues in the tests are judgement calls, and could be handled (by someone) at a later date.

Quoted 4 lines

Another option is to have a preparatory patch which first modernizes the script, and then your new tests would follow modern style. But, that may be outside of scope of your submission.

Quoted 3 lines

To summarize: The only really actionable review comments are the minor style nits in the C code. The nits about style issues in the tests are judgement calls, and could be handled (by someone) at a later date.

For the style issues in the tests maybe what I can do is I can submit another patch with this patch to modernize the test scripts of these files? I am expecting some feedback from the community on how to handle this.

From: sangu09 <[email protected]>

Git diff reports a submodule directory as -dirty even when there are only untracked files in the submodule directory.
This is inconsistent with what git describe --dirty says when run in the submodule directory in that state.

This patch makes git diff HEAD consistent with git describe --dirty when a submodule contains untracked files by making --ignore-submodules=untracked the default.

Also, to avoid ignoreSubmodules in user config being overwritten, I have made added a flag in diff_flags to keep a track whether handle_ignore_submodules_arg is called before or not.

diff.h +1 -0

index 11de52e9e9..1e18e6b1c3 100644
@@ -178,6 +178,7 @@ struct diff_flags {
 	unsigned diff_from_contents;
 	unsigned dirty_submodules;
 	unsigned ignore_untracked_in_submodules;
+	unsigned ignore_submodule_set;
 	unsigned ignore_dirty_submodules;
 	unsigned override_submodule_config;
 	unsigned dirstat_by_line;

submodule.c +0 -0

index b3bb59f066..a7956e1539 100644

Oops?

I'd suggest to follow the excellent first paragraph above with something like the following.

Make --ignore-submodules=untracked the default for git diff when there is no configuration variable or command line option, so that the command would not give '-dirty' suffix to a submodule whose working tree has untracked files, to make it consistent with git describe --dirty that is run in the submodule working tree.

This new boolean is meant to be set only when non-default ignore-submodules option is given either from the command line or from the configuration---when the bit is unset, we are told to do whatever it is that is the default for us.

And the default is to flip only this bit on. Do we need to turn off other bits that submodule.c::handle_ignore_submodules_arg() function touches? [*1*]

I like the general idea of having one bit that is set if and only if the command line or configuration told us specifically what to do, so that we can dictate the default after they were taken care of.

But I am not sure if this is a good implementation of that idea.

Case in point. I was wondering if the most future-proof way to answer the question I asked (marked with [*1*] above) was to avoid flipping the bits in options->flags ourselves, but to make a call

        handle_ignore_submodules_arg(&options, "untracked");

in repo_diff_setup(). When such an improvement is made after this patch lands, the assumption that only the end-user preference will call this function no longer holds.

Even without anticipating such a change in the future, there already is a callsite of this function in wt-status.c that hands a hardcoded string "dirty" to it. That is *not* caused by an end-user request (be it a configuration variable or a command line option), so in a sense, the assumption is already broken.

I wonder, if we can do things in a more natural way (at least the natural way in this codebase). Usually we do things in this order:

- initialize the status and option variables to their default state.

- read the configuration files to allow the state of these variables to be modified from their default state.

- parse the command line arguments to further allow the state of these variables to be modified.

and then use the final state of these variables. That way, we do not even need the extra bit that is only required if we did things in an unnatural way, which is

- read the config; remember if any bits were toggled
- parse the command line; remember if any bits were toggled
- only if bits weren't toggled in the above, set the default

I dunno.

Noise.

Misindented.

Sorry for sending it over and over again. I couldn't figure out why the mail is not reaching the community.

Quoted 4 lines

This new boolean is meant to be set only when non-default ignore-submodules option is given either from the command line or from the configuration---when the bit is unset, we are told to do whatever it is that is the default for us.

I fear I might not get your question clearly here. But from what I understood this boolean( flags.ignore_submodule_set ) is set only when diff.ignoreSubmodules is set in user-config.

Quoted 4 lines

And the default is to flip only this bit on. Do we need to turn off other bits that submodule.c::handle_ignore_submodules_arg() function touches? [*1*]

We are not flipping the bit, we are setting it to 1.

I guess we don't
have to turn off other bits because in case if submodule.c::handle_ignore_submodules_arg() is called we don't have to set the default value.

So there's no point in flipping any other bits
as if any of them is set, the user should have added some arguments in
ignoreSubmodules and therefore flags.ignore_submodule_set would be
set to 1 and we won't be setting the default value.

Quoted 9 lines

I like the general idea of having one bit that is set if and only if the command line or configuration told us specifically what to do, so that we can dictate the default after they were taken care of.
...
Even without anticipating such a change in the future, there already is a callsite of this function in wt-status.c that hands a hardcoded string "dirty" to it. That is *not* caused by an end-user request (be it a configuration variable or a command line option), so in a sense, the assumption is already broken.

I couldn't fully understand what assumption are you talking about. I would be glad if you can explain it to me in a little more detail.

Quoted 2 lines

Noise.

I hope this is a substitute for nice. XD. Hearing this after a long time. I used to hear this more frequently when I was in college.

From: sangu09 <[email protected]>

Make --ignore-submodules=untracked the default for git diff when there is no configuration variable or command line option, so that the command would not give '-dirty' suffix to a submodule whose working tree has untracked files, to make it consistent with git describe --dirty that is run in the submodule working tree.

t/t7064-wtstatus-pv2.sh +45 -0

index 537787e598..78cd86be3a 100755
@@ -503,6 +503,31 @@ test_expect_success 'untracked changes in added submodule (AM S..U)' '
 		1 AM S..U 000000 160000 160000 $ZERO_OID $HSUB sub1
 		EOF
 
+		git status --porcelain=v2 --branch --untracked-files=all --ignore-submodules=none >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'untracked changes in added submodule (A. S...) (untracked ignored)' '
+	(	cd super_repo &&
+		## create untracked file in the submodule.
+		(	cd sub1 &&
+			echo "xxxx" >file_in_sub
+		) &&
+
+		HMOD=$(git hash-object -t blob -- .gitmodules) &&
+		HSUP=$(git rev-parse HEAD) &&
+		HSUB=$HSUP &&
+
+		cat >expect <<-EOF &&
+		# branch.oid $HSUP
+		# branch.head master
+		# branch.upstream origin/master
+		# branch.ab +0 -0
+		1 A. N... 000000 100644 100644 $ZERO_OID $HMOD .gitmodules
+		1 A. S... 000000 160000 160000 $ZERO_OID $HSUB sub1
+		EOF
+
 		git status --porcelain=v2 --branch --untracked-files=all >actual &&
 		test_cmp expect actual
 	)

@@ -582,6 +607,33 @@ test_expect_success 'staged and untracked changes in added submodule (AM S.MU)'
 		1 AM S.MU 000000 160000 160000 $ZERO_OID $HSUB sub1
 		EOF
 
+		git status --porcelain=v2 --branch --untracked-files=all --ignore-submodules=none >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'staged and untracked changes in added submodule (AM S.M.) (untracked ignored)' '
+	(	cd super_repo &&
+		(	cd sub1 &&
+			## stage new changes in tracked file.
+			git add file_in_sub &&
+			## create new untracked file.
+			echo "yyyy" >>another_file_in_sub
+		) &&
+
+		HMOD=$(git hash-object -t blob -- .gitmodules) &&
+		HSUP=$(git rev-parse HEAD) &&
+		HSUB=$HSUP &&
+
+		cat >expect <<-EOF &&
+		# branch.oid $HSUP

The four-line paragraph I wrote above is not a question. It is normal to "think aloud" to express what the reader understood what the patch does/intends to do during the review, which gives the contributor a chance to spot misunderstanding by the reviewer.

In any case, when "git diff --ignore-submodules=<when>" command line option is parsed, diff_opt_ignore_submodules() gets called, which in turn calls handle_ignore_submodules_arg(), and you set the bit there.

So this is not limited to configuration, I think. Command line option given by the user can also affect it.

Yeah, flipping it to on is setting it to 1. We are saying the same thing, aren't we?

The reason why a helper function handle_ignore_submodules_arg() exists in the first place is because the way ignore_* bits are managed in the diff_options.flags is tricky and with a helper in between the callers and the actual bits, is easier to update in the future if needed.

So even if we assume that everybody who reacted to end-user preference would have called handle_ignore_submodules_arg(), hence we do not want to force our default when the .ignore_submodule_set bit set and we do want to do so when the bit is unset, we should do

        if (!options->flags.ignore_submodule_set)
                handle_ignore_submodules_arg("untracked");

instead of only toggling the .ignore_untracked_in_submodules bit on manually.

Have you looked for "dirty" (with double-quote around the word) in wt-status.c already? The call to handle_ignore_submodules_arg(), which would cause your new bit set, is made not as a reaction to any end-user input. Rather it is "if end-user did not say anything, use this hardcoded setting". And it overrides the "if the end user did not specify anything, we want to use this default" logic you added with this change, because it dictates "dirty" there without end-user asking.

Is that desirable? I dunno.

I found this change a "noise":

        strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
        if (ignore_untracked)

submodule.c +1 -1

-               strvec_push(&cp.args, "-uno");
+               strvec_push (&cp.args, "-uno");

If it were going the other direction, "we fix coding style violation while at it" may be a good justification to do so, but this particular change (1) is not neeeded for the purpose of this patch, and (2) is making the code worse by deviating from the coding guideline. Please drop it.

What happened to the first/introductory paragraph that was excellently written in the previous round?

The usaul way to compose a log message is to

- Give an observation on how the current system work in the present tense (so no need to say "Currently X is Y", just "X is Y"), and discuss what you perceive as a problem in it.

- Propose a solution (optional---often, problem description trivially leads to an obvious solution in reader's minds).

- Give commands to the codebase to "become like so".

The first paragraph you had in the previous round was a good example of the "observation". What we see above is only the "commands" part. To those who have been intimately following the discussion, it often is understandable without both, but we are not writing for those who review the patches. We are writing for future readers who are not aware of these exchanges we are having, so we should give something to prepare them by setting the stage and stating the objective.

Quoted 12 lines

t/t3701-add-interactive.sh +1 -1

index ca04fac417..98e46ad1ae 100755
@@ -761,7 +761,7 @@ test_expect_success 'setup different kinds of dirty submodules' '
              echo dirty >>initial &&
              : >untracked
      ) &&
-     git -C for-submodules diff-files --name-only >actual &&
+     git -C for-submodules diff-files --name-only --ignore-submodules=none >actual &&
      cat >expected <<-\EOF &&

t/t3701-add-interactive.sh test_expect_success 'setup different kinds of dirty submodules' ' · line 767

Quoted 2 lines

t/t3701-add-interactive.sh +0 -0

      dirty-both-ways
      dirty-head

Hmph, it seems that you chose to give options that are no longer the default to command invocations, so that the expected output from the previous world order before this patch can be reused, but is that a sensible way to go? If we are changing the default behaviour, shouldn't we rather be testing that new behaviour, I wonder?

By the way, in builtin/describe.c there is an invocation of "diff-index" without any --ignore-submodules=<what> option.

        /* diff-index command arguments to check if working tree is dirty. */
        static const char *diff_index_args[] = {
                "diff-index", "--quiet", "HEAD", "--", NULL
        };

Would the behaviour of diff-index invocation in "git describe --dirty" affected by the change of the default in any way?

Quoted 1 line

I found this change a "noise":

Oh okay, Again sorry for the misunderstanding.

Quoted 6 lines

        strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
        if (ignore_untracked)

submodule.c +1 -1

-               strvec_push(&cp.args, "-uno");
+               strvec_push (&cp.args, "-uno");

Quoted 6 lines

If it were going the other direction, "we fix coding style violation while at it" may be a good justification to do so, but this particular change (1) is not neeeded for the purpose of this patch, and (2) is making the code worse by deviating from the coding guideline. Please drop it.

This part of the change was introduced because we had a failing test here[3]. There was some problem it getting both the flags propagated through ...

Quoted 16 lines · Sangeeta NB

Quoted 1 line

I found this change a "noise":

Oh okay, Again sorry for the misunderstanding.

Quoted 6 lines

        strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
        if (ignore_untracked)

submodule.c +1 -1

-               strvec_push(&cp.args, "-uno");
+               strvec_push (&cp.args, "-uno");

Quoted 6 lines

If it were going the other direction, "we fix coding style violation while at it" may be a good justification to do so, but this particular change (1) is not neeeded for the purpose of this patch, and (2) is making the code worse by deviating from the coding guideline. Please drop it.

Quoted 3 lines

This part of the change was introduced because we had a failing test here[3]. There was some problem it getting both the flags propagated through ...

Are you talking about the new "else" clause added to the "if" statement we see above? I am not saying it is a "noise".

But look at what you did to the existing call to strvec_push() to add "-uno" shown above in the patch, i.e. the addition of space before the parenthesis. We cannot justify that change, can we? That's noise as far as I can see.

Quoted 8 lines

By the way, in builtin/describe.c there is an invocation of "diff-index" without any --ignore-submodules=<what> option.

        /* diff-index command arguments to check if working tree is dirty. */
        static const char *diff_index_args[] = {
                "diff-index", "--quiet", "HEAD", "--", NULL
        };

Quoted 2 lines

Would the behaviour of diff-index invocation in "git describe --dirty" affected by the change of the default in any way?

I don't think so because describe was already behaving in the way that we won't. We changed the behavior of git diff to match it with that. So it doesn't make sense to add anything to that. Tell me if I am missing something.

t/t4041-diff-submodule-option.sh +1 -1

index f852136585..bb368b685d 100755
@@ -262,7 +262,7 @@ test_expect_success 'submodule is up to date' '
 
 test_expect_success 'submodule contains untracked content' '
 	echo new > sm1/new-file &&
-	git diff-index -p --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD --ignore-submodules=none >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	EOF

t/t4027-diff-submodule.sh test_expect_success 'git diff HEAD with dirty submodule (untracked)' ' · line 107

Quoted 10 lines · Sangeeta Jain

t/t4027-diff-submodule.sh +8 -0

...
+test_expect_success 'git diff HEAD with dirty submodule (untracked) (none ignored)' '
+     git config diff.ignoreSubmodules none &&
+     git diff HEAD >actual &&
+     sed -e "1,/^@@/d" actual >actual.body &&
+     expect_from_to >expect.body $subtip $subprev-dirty &&
+     test_cmp expect.body actual.body &&
+     git config --unset diff.ignoreSubmodules
+'

If any step concatenated by && in the above sequence fails, the test repository will have diff.ignoreSubmodules set to none (without getting it reset, since &&-chain prevents the last "config --unset" step from running). Unless the test is run under the --immediate option, that would affect the environment in which subsequent tests are run.

Instead, our test usually do this:

        test_expect_success 'test title' '
                test_config diff.ignoresubmodules none &&
                git diff HEAD >actual &&
                ...
                test_cmp expect.body actual.body
        '

"test_config" sets up a trigger, which will _always_ fire, whether the test fails at any intermediate steps or runs to the end and succeeds, to remove the configuration it just added. It is a short-hand for writing

test_when_finished test_unconfig diff.ignoresubmodules && git config diff.ignoresubmodules none

i.e. to use test_when_finished to set up the trigger.

t/t4027-diff-submodule.sh test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)' · line 186

Quoted 9 lines

t/t4027-diff-submodule.sh +2 -2

-     git diff HEAD >actual &&
+     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 diff --ignore-submodules=all HEAD >actual2 &&
      test_must_be_empty actual2 &&
-     git diff --ignore-submodules=untracked HEAD >actual3 &&
+     git diff HEAD >actual3 &&

This line can be left as-is, no?

Quoted 11 lines

t/t4027-diff-submodule.sh test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)' · line 186

Quoted 9 lines

t/t4027-diff-submodule.sh +2 -2

-     git diff HEAD >actual &&
+     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 diff --ignore-submodules=all HEAD >actual2 &&
      test_must_be_empty actual2 &&
-     git diff --ignore-submodules=untracked HEAD >actual3 &&
+     git diff HEAD >actual3 &&

This line can be left as-is, no?

Ya, they can be left as it is but I removed the --ignore-submodules=untracked because we also have to check the default git behavior and adding that doesn't change anything in the code so I removed that.
In general, in all the tests I followed this practice:
1. I added --ignore-submodules=none where there were no options specified(as this was the earlier default)
2. I removed --ignore-submodules=untracked(as this should even work if no options are specified)

I would put that back if you still want it?

Quoted 10 lines

Quoted 10 lines

* sj/untracked-files-in-submodule-directory-is-not-dirty (2020-10-26) 1 commit
- diff: do not show submodule with untracked files as "-dirty"

"git diff" showed a submodule working tree with untracked cruft as "Submodule commit <objectname>-dirty", but a natural expectation is that the "-dirty" indicator would align with "git describe --dirty", which does not consider having untracked files in the working tree as source of dirtiness. The inconsistency has been fixed.

Needs doc update.

Quoted 3 lines

I *think* the original rationale for marking submodules with untracked (_un-ignored_) files was to avoid deleting a submodule that has uncommitted (because untracked) files.

I agree with you that that the motivation was exactly that, but I have a suspicion that its execution was misguided.

When one has a subdirectory D where one might or might not have an untracked new file that one does not want to lose, but one otherwise would want to clean up (perhaps an errant process created tons of garbage files in the directory), one would not

git diff D

to see if there are important changes in that directory before doing

        rm -rf D && git checkout D

to bring it back to pristine state, exactly because one would not want to lose newly-created but not added files. One would instead use

git status D

for checking. Why would a user change the habit when D happens to be a submodule?

So I would say that "git status [D]", if it does not let the user notice that there is untracked contents in the submodule working tree, would be a bad idea. "git diff" that ignores untracked paths in the submodule working tree, on the other hand, is a good thing.

Now, the patch as-is may change the behaviour of "git status D" in this case, and the internal invocation of diff-files made somewhere in wt-status.c may have to be adjusted to keep such a submodule with forgotten newly created files shown as modified.

Further discussion on this should be done on the original review thread for continuity, not here, in any case.

Hey Johannes and Junio,

In continuation of the discussion from here[1]

[1] https://lore.kernel.org/git/[email protected]/

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

* sj/untracked-files-in-submodule-directory-is-not-dirty (2020-10-26) 1 commit
- diff: do not show submodule with untracked files as "-dirty"

"git diff" showed a submodule working tree with untracked cruft as "Submodule commit <objectname>-dirty", but a natural expectation is that the "-dirty" indicator would align with "git describe --dirty", which does not consider having untracked files in the working tree as source of dirtiness. The inconsistency has been fixed.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Needs doc update.

Do I need to add the doc update in the same patch?

Quoted 6 lines

Quoted 3 lines

I *think* the original rationale for marking submodules with untracked (_un-ignored_) files was to avoid deleting a submodule that has uncommitted (because untracked) files.

I agree with you that that the motivation was exactly that, but I have a suspicion that its execution was misguided.

I am really very sorry if I ended up adding some unwanted code.

So we want that git status should behave in the same way as it was behaving before? Can we do this by passing --ignore-submodules=none as the default args for status?

Another approach might be to figure out
how the diff is being called(like is this being called from git status or git diff) and then add the --ignore-submodules=untracked behavior to it accordingly. Though I have no idea how to do that now.

t/t3701-add-interactive.sh +0 -0

 	dirty-otherwise
 	EOF
 	test_cmp expected actual &&

t/t4027-diff-submodule.sh +10 -2

index d7145ccca4..894a11b224 100755
@@ -93,6 +93,14 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked)' '
 	) &&
 	git diff HEAD >actual &&
 	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subtip $subprev &&
+	test_cmp expect.body actual.body
+'
+
+test_expect_success 'git diff HEAD with dirty submodule (untracked) (none ignored)' '
+	test_config diff.ignoreSubmodules none &&
+	git diff HEAD >actual &&
+	sed -e "1,/^@@/d" actual >actual.body &&
 	expect_from_to >expect.body $subtip $subprev-dirty &&
 	test_cmp expect.body actual.body
 '

@@ -168,13 +176,13 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'
 		git clean -qfdx &&
 		>cruft
 	) &&
-	git diff HEAD >actual &&
+	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 diff --ignore-submodules=all HEAD >actual2 &&
 	test_must_be_empty actual2 &&
-	git diff --ignore-submodules=untracked HEAD >actual3 &&
+	git diff HEAD >actual3 &&
 	test_must_be_empty actual3 &&
 	git diff --ignore-submodules=dirty HEAD >actual4 &&
 	test_must_be_empty actual4

t/t4041-diff-submodule-option.sh +0 -0

index f852136585..bb368b685d 100755

t/t4041-diff-submodule-option.sh +7 -7

 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	EOF

@@ -270,7 +270,7 @@ test_expect_success 'submodule contains untracked content' '
 '
 
 test_expect_success 'submodule contains untracked content (untracked ignored)' '
-	git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD >actual &&
 	test_must_be_empty actual
 '
 

@@ -286,7 +286,7 @@ test_expect_success 'submodule contains untracked content (all ignored)' '
 
 test_expect_success 'submodule contains untracked and modified content' '
 	echo new > sm1/foo6 &&
-	git diff-index -p --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD --ignore-submodules=none >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	Submodule sm1 contains modified content

@@ -296,7 +296,7 @@ test_expect_success 'submodule contains untracked and modified content' '
 
 test_expect_success 'submodule contains untracked and modified content (untracked ignored)' '
 	echo new > sm1/foo6 &&
-	git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains modified content
 	EOF

@@ -337,7 +337,7 @@ test_expect_success 'submodule is modified' '
 
 test_expect_success 'modified submodule contains untracked content' '
 	echo new > sm1/new-file &&
-	git diff-index -p --submodule=log HEAD >actual &&
+	git diff-index -p  --ignore-submodules=none --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	Submodule sm1 $head6..$head8:

@@ -347,7 +347,7 @@ test_expect_success 'modified submodule contains untracked content' '
 '
 
 test_expect_success 'modified submodule contains untracked content (untracked ignored)' '
-	git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head6..$head8:
 	  > change

@@ -371,7 +371,7 @@ test_expect_success 'modified submodule contains untracked content (all ignored)
 
 test_expect_success 'modified submodule contains untracked and modified content' '
 	echo modification >> sm1/foo6 &&
-	git diff-index -p --submodule=log HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	Submodule sm1 contains modified content

@@ -383,7 +383,7 @@ test_expect_success 'modified submodule contains untracked and modified content'
 
 test_expect_success 'modified submodule contains untracked and modified content (untracked ignored)' '
 	echo modification >> sm1/foo6 &&
-	git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains modified content
 	Submodule sm1 $head6..$head8:

t/t4060-diff-submodule-option-diff-format.sh +8 -8

index fc8229c726..dc7b242697 100755
@@ -409,7 +409,7 @@ test_expect_success 'submodule is up to date' '
 
 test_expect_success 'submodule contains untracked content' '
 	echo new > sm1/new-file &&
-	git diff-index -p --submodule=diff HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	EOF

@@ -417,7 +417,7 @@ test_expect_success 'submodule contains untracked content' '
 '
 
 test_expect_success 'submodule contains untracked content (untracked ignored)' '
-	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	git diff-index -p --submodule=diff HEAD >actual &&
 	test_must_be_empty actual
 '
 

@@ -433,7 +433,7 @@ test_expect_success 'submodule contains untracked content (all ignored)' '
 
 test_expect_success 'submodule contains untracked and modified content' '
 	echo new > sm1/foo6 &&
-	git diff-index -p --submodule=diff HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	Submodule sm1 contains modified content

@@ -451,7 +451,7 @@ test_expect_success 'submodule contains untracked and modified content' '
 # NOT OK
 test_expect_success 'submodule contains untracked and modified content (untracked ignored)' '
 	echo new > sm1/foo6 &&
-	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains modified content
 	diff --git a/sm1/foo6 b/sm1/foo6

@@ -512,7 +512,7 @@ test_expect_success 'submodule is modified' '
 
 test_expect_success 'modified submodule contains untracked content' '
 	echo new > sm1/new-file &&
-	git diff-index -p --submodule=diff HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	Submodule sm1 $head7..$head8:

@@ -528,7 +528,7 @@ test_expect_success 'modified submodule contains untracked content' '
 '
 
 test_expect_success 'modified submodule contains untracked content (untracked ignored)' '
-	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head7..$head8:
 	diff --git a/sm1/foo6 b/sm1/foo6

@@ -564,7 +564,7 @@ test_expect_success 'modified submodule contains untracked content (all ignored)
 
 test_expect_success 'modified submodule contains untracked and modified content' '
 	echo modification >> sm1/foo6 &&
-	git diff-index -p --submodule=diff HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	Submodule sm1 contains modified content

@@ -583,7 +583,7 @@ test_expect_success 'modified submodule contains untracked and modified content'
 
 test_expect_success 'modified submodule contains untracked and modified content (untracked ignored)' '
 	echo modification >> sm1/foo6 &&
-	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains modified content
 	Submodule sm1 $head7..$head8:

t/t7064-wtstatus-pv2.sh +1 -0

index 537787e598..78cd86be3a 100755
@@ -503,6 +503,31 @@ test_expect_success 'untracked changes in added submodule (AM S..U)' '
 		1 AM S..U 000000 160000 160000 $ZERO_OID $HSUB sub1
 		EOF
 
+		git status --porcelain=v2 --branch --untracked-files=all --ignore-submodules=none >actual &&

t/t7064-wtstatus-pv2.sh +41 -0

+
+		HMOD=$(git hash-object -t blob -- .gitmodules) &&
+		HSUP=$(git rev-parse HEAD) &&
+		HSUB=$HSUP &&
+
+		cat >expect <<-EOF &&
+		# branch.oid $HSUP
+		# branch.head master
+		# branch.upstream origin/master
+		# branch.ab +0 -0
+		1 A. N... 000000 100644 100644 $ZERO_OID $HMOD .gitmodules
+		1 A. S... 000000 160000 160000 $ZERO_OID $HSUB sub1
+		EOF
+
 		git status --porcelain=v2 --branch --untracked-files=all >actual &&
 		test_cmp expect actual
 	)

@@ -582,6 +607,33 @@ test_expect_success 'staged and untracked changes in added submodule (AM S.MU)'
 		1 AM S.MU 000000 160000 160000 $ZERO_OID $HSUB sub1
 		EOF
 
+		git status --porcelain=v2 --branch --untracked-files=all --ignore-submodules=none >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'staged and untracked changes in added submodule (AM S.M.) (untracked ignored)' '
+	(	cd super_repo &&
+		(	cd sub1 &&
+			## stage new changes in tracked file.
+			git add file_in_sub &&
+			## create new untracked file.
+			echo "yyyy" >>another_file_in_sub
+		) &&
+
+		HMOD=$(git hash-object -t blob -- .gitmodules) &&
+		HSUP=$(git rev-parse HEAD) &&
+		HSUB=$HSUP &&
+
+		cat >expect <<-EOF &&
+		# branch.oid $HSUP
+		# branch.head master
+		# branch.upstream origin/master
+		# branch.ab +0 -0
+		1 A. N... 000000 100644 100644 $ZERO_OID $HMOD .gitmodules
+		1 AM S.M. 000000 160000 160000 $ZERO_OID $HSUB sub1
+		EOF
+
 		git status --porcelain=v2 --branch --untracked-files=all >actual &&
 		test_cmp expect actual
 	)

t/t7506-status-submodule.sh +90 -12

index 3fcb44767f..72da00a962 100755
@@ -94,36 +94,60 @@ test_expect_success 'status with added file in submodule (short)' '
 test_expect_success 'status with untracked file in submodule' '
 	(cd sub && git reset --hard) &&
 	echo "content" >sub/new-file &&
-	git status >output &&
+	git status --ignore-submodules=none >output &&
 	test_i18ngrep "modified:   sub (untracked content)" output
 '
 
+test_expect_success 'status with untracked file in submodule (untracked ignored)' '
+	(cd sub && git reset --hard) &&
+	echo "content" >sub/new-file &&
+	git status >output &&
+	test_i18ngrep "^nothing to commit" output
+'
+
 test_expect_success 'status -uno with untracked file in submodule' '
 	git status -uno >output &&
 	test_i18ngrep "^nothing to commit" output
 '
 
 test_expect_success 'status with untracked file in submodule (porcelain)' '
-	git status --porcelain >output &&
+	git status --porcelain --ignore-submodules=none >output &&
 	diff output - <<-\EOF
 	 M sub
 	EOF
 '
 
+test_expect_success 'status with untracked file in submodule (porcelain) (untracked ignored)' '
+	git status --porcelain >output &&
+	test_must_be_empty output
+'
+
 test_expect_success 'status with untracked file in submodule (short)' '
-	git status --short >output &&
+	git status --short --ignore-submodules=none >output &&
 	diff output - <<-\EOF
 	 ? sub
 	EOF
 '
 
+test_expect_success 'status with untracked file in submodule (short) (untracked ignored)' '
+	git status --short >output &&
+	test_must_be_empty output
+'
+
 test_expect_success 'status with added and untracked file in submodule' '
 	(cd sub && git reset --hard && echo >foo && git add foo) &&
 	echo "content" >sub/new-file &&
-	git status >output &&
+	git status --ignore-submodules=none >output &&
 	test_i18ngrep "modified:   sub (modified content, untracked content)" output
 '
 
+test_expect_success 'status with added and untracked file in submodule (untracked ignored)' '
+	(cd sub && git reset --hard && echo >foo && git add foo) &&
+	echo "content" >sub/new-file &&
+	git status >output &&
+	test_i18ngrep "modified:   sub (modified content)" output
+'
+
 test_expect_success 'status with added and untracked file in submodule (porcelain)' '
 	(cd sub && git reset --hard && echo >foo && git add foo) &&
 	echo "content" >sub/new-file &&

@@ -168,10 +192,17 @@ test_expect_success 'status with added file in modified submodule (porcelain)' '
 test_expect_success 'status with untracked file in modified submodule' '
 	(cd sub && git reset --hard) &&
 	echo "content" >sub/new-file &&
-	git status >output &&
+	git status --ignore-submodules=none >output &&
 	test_i18ngrep "modified:   sub (new commits, untracked content)" output
 '
 
+test_expect_success 'status with untracked file in modified submodule (untracked ignored)' '
+	(cd sub && git reset --hard) &&
+	echo "content" >sub/new-file &&
+	git status >output &&
+	test_i18ngrep "modified:   sub (new commits)" output
+'
+
 test_expect_success 'status with untracked file in modified submodule (porcelain)' '
 	git status --porcelain >output &&
 	diff output - <<-\EOF

@@ -182,10 +213,17 @@ test_expect_success 'status with untracked file in modified submodule (porcelain
 test_expect_success 'status with added and untracked file in modified submodule' '
 	(cd sub && git reset --hard && echo >foo && git add foo) &&
 	echo "content" >sub/new-file &&
-	git status >output &&
+	git status --ignore-submodules=none >output &&
 	test_i18ngrep "modified:   sub (new commits, modified content, untracked content)" output
 '
 
+test_expect_success 'status with added and untracked file in modified submodule (untracked ignored)' '
+	(cd sub && git reset --hard && echo >foo && git add foo) &&
+	echo "content" >sub/new-file &&
+	git status >output &&
+	test_i18ngrep "modified:   sub (new commits, modified content)" output
+'
+
 test_expect_success 'status with added and untracked file in modified submodule (porcelain)' '
 	(cd sub && git reset --hard && echo >foo && git add foo) &&
 	echo "content" >sub/new-file &&

@@ -349,7 +387,7 @@ test_expect_success 'setup superproject with untracked file in nested submodule'
 '
 
 test_expect_success 'status with untracked file in nested submodule (porcelain)' '
-	git -C super status --porcelain >output &&
+	git -C super status --porcelain --ignore-submodules=none >output &&
 	diff output - <<-\EOF
 	 M sub1
 	 M sub2

@@ -357,8 +395,13 @@ test_expect_success 'status with untracked file in nested submodule (porcelain)'
 	EOF
 '
 
+test_expect_success 'status with untracked file in nested submodule (porcelain) (untracked ignored)' '
+	git -C super status --porcelain >output &&
+	test_must_be_empty output
+'
+
 test_expect_success 'status with untracked file in nested submodule (porcelain=2)' '
-	git -C super status --porcelain=2 >output &&
+	git -C super status --porcelain=2 --ignore-submodules=none >output &&
 	sanitize_output output &&
 	diff output - <<-\EOF
 	1 .M S..U 160000 160000 160000 HASH HASH sub1

@@ -367,8 +410,13 @@ test_expect_success 'status with untracked file in nested submodule (porcelain=2
 	EOF
 '
 
+test_expect_success 'status with untracked file in nested submodule (porcelain=2) (untracked ignored)' '
+	git -C super status --porcelain=2 >output &&
+	test_must_be_empty output
+'
+
 test_expect_success 'status with untracked file in nested submodule (short)' '
-	git -C super status --short >output &&
+	git -C super status --short --ignore-submodules=none >output &&
 	diff output - <<-\EOF
 	 ? sub1
 	 ? sub2

@@ -376,13 +424,18 @@ test_expect_success 'status with untracked file in nested submodule (short)' '
 	EOF
 '
 
+test_expect_success 'status with untracked file in nested submodule (short) (untracked ignored)' '
+	git -C super status --short >output &&
+	test_must_be_empty output
+'
+
 test_expect_success 'setup superproject with modified file in nested submodule' '
 	git -C super/sub1/sub2 add file &&
 	git -C super/sub2 add file
 '
 
 test_expect_success 'status with added file in nested submodule (porcelain)' '
-	git -C super status --porcelain >output &&
+	git -C super status --porcelain --ignore-submodules=none >output &&
 	diff output - <<-\EOF
 	 M sub1
 	 M sub2

@@ -390,8 +443,16 @@ test_expect_success 'status with added file in nested submodule (porcelain)' '
 	EOF
 '
 
+test_expect_success 'status with added file in nested submodule (porcelain) (untracked ignored)' '
+	git -C super status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub1
+	 M sub2
+	EOF
+'
+
 test_expect_success 'status with added file in nested submodule (porcelain=2)' '
-	git -C super status --porcelain=2 >output &&
+	git -C super status --porcelain=2 --ignore-submodules=none >output &&
 	sanitize_output output &&
 	diff output - <<-\EOF
 	1 .M S.M. 160000 160000 160000 HASH HASH sub1

@@ -400,8 +461,17 @@ test_expect_success 'status with added file in nested submodule (porcelain=2)' '
 	EOF
 '
 
+test_expect_success 'status with added file in nested submodule (porcelain=2) (untracked ignored)' '
+	git -C super status --porcelain=2 >output &&
+	sanitize_output output &&
+	diff output - <<-\EOF
+	1 .M S.M. 160000 160000 160000 HASH HASH sub1
+	1 .M S.M. 160000 160000 160000 HASH HASH sub2
+	EOF
+'
+
 test_expect_success 'status with added file in nested submodule (short)' '
-	git -C super status --short >output &&
+	git -C super status --short --ignore-submodules=none >output &&
 	diff output - <<-\EOF
 	 m sub1
 	 m sub2

@@ -409,4 +479,12 @@ test_expect_success 'status with added file in nested submodule (short)' '
 	EOF
 '
 
+test_expect_success 'status with added file in nested submodule (short) (untracked ignored)' '
+	git -C super status --short >output &&
+	diff output - <<-\EOF
+	 m sub1
+	 m sub2
+	EOF
+'
+
 test_done

Signature · 1 line

2.21.1 (Apple Git-122.3)

Quoted 8 lines

t/t3701-add-interactive.sh +0 -0

index ca04fac417..9104d1946d 100755
@@ -765,6 +765,11 @@ test_expect_success 'setup different kinds of dirty submodules' '
      cat >expected <<-\EOF &&
      dirty-both-ways
      dirty-head

t/t3701-add-interactive.sh test_expect_success 'setup different kinds of dirty submodules' ' · line 772

Quoted 5 lines

t/t3701-add-interactive.sh +5 -0

+     EOF
+     git -C for-submodules diff-files --name-only --ignore-submodules=none >actual &&
+     cat >expected <<-\EOF &&
+     dirty-both-ways
+     dirty-head

This will throw-away above change to "expected", I think this is not what you expected to write!

Quoted 8 lines

Quoted 8 lines

t/t3701-add-interactive.sh +0 -0

index ca04fac417..9104d1946d 100755
@@ -765,6 +765,11 @@ test_expect_success 'setup different kinds of dirty submodules' '
      cat >expected <<-\EOF &&
      dirty-both-ways
      dirty-head

Quoted 8 lines

t/t3701-add-interactive.sh test_expect_success 'setup different kinds of dirty submodules' ' · line 772

Quoted 5 lines

t/t3701-add-interactive.sh +5 -0

+     EOF
+     git -C for-submodules diff-files --name-only --ignore-submodules=none >actual &&
+     cat >expected <<-\EOF &&
+     dirty-both-ways
+     dirty-head

This will throw-away above change to "expected", I think this is not what you expected to write!

I am sorry I couldn't understand what you mean by that. I think that is what I expected to write. I want those changes to be in "expected".

Documentation/config/diff.txt +0 -0

 diff.mnemonicPrefix::
 	If set, 'git diff' uses a prefix pair that is different from the

diff.c +3 -0

index 2bb2f8f57e..5a80695da8 100644
@@ -4585,6 +4585,9 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)
 		DIFF_XDL_SET(options, INDENT_HEURISTIC);
 
 	options->orderfile = diff_order_file_cfg;
+	
+	if (!options->flags.ignore_submodule_set)
+		options->flags.ignore_untracked_in_submodules = 1;
 
 	if (diff_no_prefix) {
 		options->a_prefix = options->b_prefix = "";

diff.h +1 -0

index 11de52e9e9..1e18e6b1c3 100644
@@ -178,6 +178,7 @@ struct diff_flags {
 	unsigned diff_from_contents;
 	unsigned dirty_submodules;
 	unsigned ignore_untracked_in_submodules;
+	unsigned ignore_submodule_set;
 	unsigned ignore_dirty_submodules;
 	unsigned override_submodule_config;
 	unsigned dirstat_by_line;

submodule.c +1 -0

index b3bb59f066..8f6227c993 100644
@@ -420,6 +420,7 @@ const char *submodule_strategy_to_string(const struct submodule_update_strategy
 void handle_ignore_submodules_arg(struct diff_options *diffopt,
 				  const char *arg)
 {
+	diffopt->flags.ignore_submodule_set = 1;
 	diffopt->flags.ignore_submodules = 0;
 	diffopt->flags.ignore_untracked_in_submodules = 0;
 	diffopt->flags.ignore_dirty_submodules = 0;

t/t3701-add-interactive.sh +6 -0

index ca04fac417..9a2489cde0 100755
@@ -765,6 +765,12 @@ test_expect_success 'setup different kinds of dirty submodules' '
 	cat >expected <<-\EOF &&
 	dirty-both-ways
 	dirty-head
+	EOF
+	test_cmp expected actual &&
+	git -C for-submodules diff-files --name-only --ignore-submodules=none >actual &&
+	cat >expected <<-\EOF &&
+	dirty-both-ways
+	dirty-head
 	dirty-otherwise
 	EOF
 	test_cmp expected actual &&

t/t4027-diff-submodule.sh +10 -2

index d7145ccca4..894a11b224 100755
@@ -93,6 +93,14 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked)' '
 	) &&
 	git diff HEAD >actual &&
 	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subtip $subprev &&
+	test_cmp expect.body actual.body
+'
+
+test_expect_success 'git diff HEAD with dirty submodule (untracked) (none ignored)' '
+	test_config diff.ignoreSubmodules none &&
+	git diff HEAD >actual &&
+	sed -e "1,/^@@/d" actual >actual.body &&
 	expect_from_to >expect.body $subtip $subprev-dirty &&
 	test_cmp expect.body actual.body
 '

@@ -168,13 +176,13 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'
 		git clean -qfdx &&
 		>cruft
 	) &&
-	git diff HEAD >actual &&
+	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 diff --ignore-submodules=all HEAD >actual2 &&
 	test_must_be_empty actual2 &&
-	git diff --ignore-submodules=untracked HEAD >actual3 &&
+	git diff HEAD >actual3 &&
 	test_must_be_empty actual3 &&
 	git diff --ignore-submodules=dirty HEAD >actual4 &&
 	test_must_be_empty actual4

t/t4041-diff-submodule-option.sh +8 -8

index f852136585..b3a7b7acaa 100755
@@ -262,7 +262,7 @@ test_expect_success 'submodule is up to date' '
 
 test_expect_success 'submodule contains untracked content' '
 	echo new > sm1/new-file &&
-	git diff-index -p --submodule=log HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	EOF

@@ -270,7 +270,7 @@ test_expect_success 'submodule contains untracked content' '
 '
 
 test_expect_success 'submodule contains untracked content (untracked ignored)' '
-	git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD >actual &&
 	test_must_be_empty actual
 '
 

@@ -286,7 +286,7 @@ test_expect_success 'submodule contains untracked content (all ignored)' '
 
 test_expect_success 'submodule contains untracked and modified content' '
 	echo new > sm1/foo6 &&
-	git diff-index -p --submodule=log HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	Submodule sm1 contains modified content

@@ -296,7 +296,7 @@ test_expect_success 'submodule contains untracked and modified content' '
 
 test_expect_success 'submodule contains untracked and modified content (untracked ignored)' '
 	echo new > sm1/foo6 &&
-	git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains modified content
 	EOF

@@ -337,7 +337,7 @@ test_expect_success 'submodule is modified' '
 
 test_expect_success 'modified submodule contains untracked content' '
 	echo new > sm1/new-file &&
-	git diff-index -p --submodule=log HEAD >actual &&
+	git diff-index -p  --ignore-submodules=none --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	Submodule sm1 $head6..$head8:

@@ -347,7 +347,7 @@ test_expect_success 'modified submodule contains untracked content' '
 '
 
 test_expect_success 'modified submodule contains untracked content (untracked ignored)' '
-	git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head6..$head8:
 	  > change

@@ -371,7 +371,7 @@ test_expect_success 'modified submodule contains untracked content (all ignored)
 
 test_expect_success 'modified submodule contains untracked and modified content' '
 	echo modification >> sm1/foo6 &&
-	git diff-index -p --submodule=log HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	Submodule sm1 contains modified content

@@ -383,7 +383,7 @@ test_expect_success 'modified submodule contains untracked and modified content'
 
 test_expect_success 'modified submodule contains untracked and modified content (untracked ignored)' '
 	echo modification >> sm1/foo6 &&
-	git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains modified content
 	Submodule sm1 $head6..$head8:

t/t4060-diff-submodule-option-diff-format.sh +8 -8

index fc8229c726..dc7b242697 100755
@@ -409,7 +409,7 @@ test_expect_success 'submodule is up to date' '
 
 test_expect_success 'submodule contains untracked content' '
 	echo new > sm1/new-file &&
-	git diff-index -p --submodule=diff HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	EOF

@@ -417,7 +417,7 @@ test_expect_success 'submodule contains untracked content' '
 '
 
 test_expect_success 'submodule contains untracked content (untracked ignored)' '
-	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	git diff-index -p --submodule=diff HEAD >actual &&
 	test_must_be_empty actual
 '
 

@@ -433,7 +433,7 @@ test_expect_success 'submodule contains untracked content (all ignored)' '
 
 test_expect_success 'submodule contains untracked and modified content' '
 	echo new > sm1/foo6 &&
-	git diff-index -p --submodule=diff HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	Submodule sm1 contains modified content

@@ -451,7 +451,7 @@ test_expect_success 'submodule contains untracked and modified content' '
 # NOT OK
 test_expect_success 'submodule contains untracked and modified content (untracked ignored)' '
 	echo new > sm1/foo6 &&
-	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains modified content
 	diff --git a/sm1/foo6 b/sm1/foo6

@@ -512,7 +512,7 @@ test_expect_success 'submodule is modified' '
 
 test_expect_success 'modified submodule contains untracked content' '
 	echo new > sm1/new-file &&
-	git diff-index -p --submodule=diff HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	Submodule sm1 $head7..$head8:

@@ -528,7 +528,7 @@ test_expect_success 'modified submodule contains untracked content' '
 '
 
 test_expect_success 'modified submodule contains untracked content (untracked ignored)' '
-	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head7..$head8:
 	diff --git a/sm1/foo6 b/sm1/foo6

@@ -564,7 +564,7 @@ test_expect_success 'modified submodule contains untracked content (all ignored)
 
 test_expect_success 'modified submodule contains untracked and modified content' '
 	echo modification >> sm1/foo6 &&
-	git diff-index -p --submodule=diff HEAD >actual &&
+	git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains untracked content
 	Submodule sm1 contains modified content

@@ -583,7 +583,7 @@ test_expect_success 'modified submodule contains untracked and modified content'
 
 test_expect_success 'modified submodule contains untracked and modified content (untracked ignored)' '
 	echo modification >> sm1/foo6 &&
-	git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 contains modified content
 	Submodule sm1 $head7..$head8:

wt-status.c +3 -1

index 7074bbdd53..83d418647d 100644
@@ -606,7 +606,9 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
 	if (s->ignore_submodule_arg) {
 		rev.diffopt.flags.override_submodule_config = 1;
 		handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
-	}
+	} else if (!rev.diffopt.flags.ignore_submodule_set && 
+			s->show_untracked_files != SHOW_NO_UNTRACKED_FILES)
+		handle_ignore_submodules_arg(&rev.diffopt, "none");
 	rev.diffopt.format_callback = wt_status_collect_changed_cb;
 	rev.diffopt.format_callback_data = s;
 	rev.diffopt.detect_rename = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;

Signature · 1 line

2.21.1 (Apple Git-122.3)

Git diff reports a submodule directory as -dirty even when there are only untracked files in the submodule directory. This is inconsistent with what git describe --dirty says when run in the submodule directory in that state.

Make --ignore-submodules=untracked the default for git diff when there is no configuration variable or command line option, so that the command would not give '-dirty' suffix to a submodule whose working tree has untracked files, to make it consistent with git describe --dirty that is run in the submodule working tree.

And also make --ignore-submodules=none the default for git status so that the user doesn't end up deleting a submodule that has uncommitted (untracked) files.

Signed-off-by Sangeeta Jain <[email protected]>

---
 Documentation/config/diff.txt                |  2 ++
 diff.c                                       |  3 +++
 diff.h                                       |  1 +
 submodule.c                                  |  1 +
 t/t3701-add-interactive.sh                   |  6 ++++++
 t/t4027-diff-submodule.sh                    | 12 ++++++++++--
 t/t4041-diff-submodule-option.sh             | 16 ++++++++--------
 t/t4060-diff-submodule-option-diff-format.sh | 16 ++++++++--------
 wt-status.c                                  |  4 +++-
 9 files changed, 42 insertions(+), 19 deletions(-)

Documentation/config/diff.txt +2 -0

index c3ae136eba..2d3331f55c 100644
@@ -85,6 +85,8 @@ diff.ignoreSubmodules::
        and 'git status' when `status.submoduleSummary` is set unless it is
        overridden by using the --ignore-submodules command-line option.
        The 'git submodule' commands are not affected by this setting.
+       By default this is set to untracked so that any untracked
+       submodules are ignored.
 
 diff.mnemonicPrefix::
        If set, 'git diff' uses a prefix pair that is different from the

diff.c +3 -0

index 2bb2f8f57e..5a80695da8 100644
@@ -4585,6 +4585,9 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)
                DIFF_XDL_SET(options, INDENT_HEURISTIC);
 
        options->orderfile = diff_order_file_cfg;
+
+       if (!options->flags.ignore_submodule_set)
+               options->flags.ignore_untracked_in_submodules = 1;
 
        if (diff_no_prefix) {
                options->a_prefix = options->b_prefix = "";

diff.h +1 -0

index 11de52e9e9..1e18e6b1c3 100644
@@ -178,6 +178,7 @@ struct diff_flags {
        unsigned diff_from_contents;
        unsigned dirty_submodules;
        unsigned ignore_untracked_in_submodules;
+       unsigned ignore_submodule_set;
        unsigned ignore_dirty_submodules;
        unsigned override_submodule_config;
        unsigned dirstat_by_line;

submodule.c +1 -0

index b3bb59f066..8f6227c993 100644
@@ -420,6 +420,7 @@ const char *submodule_strategy_to_string(const struct submodule_update_strategy
 void handle_ignore_submodules_arg(struct diff_options *diffopt,
                                  const char *arg)
 {
+       diffopt->flags.ignore_submodule_set = 1;
        diffopt->flags.ignore_submodules = 0;
        diffopt->flags.ignore_untracked_in_submodules = 0;
        diffopt->flags.ignore_dirty_submodules = 0;

t/t3701-add-interactive.sh +6 -0

index ca04fac417..9a2489cde0 100755
@@ -765,6 +765,12 @@ test_expect_success 'setup different kinds of dirty submodules' '
        cat >expected <<-\EOF &&
        dirty-both-ways
        dirty-head
+       EOF
+       test_cmp expected actual &&
+       git -C for-submodules diff-files --name-only --ignore-submodules=none >actual &&
+       cat >expected <<-\EOF &&
+       dirty-both-ways
+       dirty-head
        dirty-otherwise
        EOF
        test_cmp expected actual &&

t/t4027-diff-submodule.sh +10 -2

index d7145ccca4..894a11b224 100755
@@ -93,6 +93,14 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked)' '
        ) &&
        git diff HEAD >actual &&
        sed -e "1,/^@@/d" actual >actual.body &&
+       expect_from_to >expect.body $subtip $subprev &&
+       test_cmp expect.body actual.body
+'
+
+test_expect_success 'git diff HEAD with dirty submodule (untracked) (none ignored)' '
+       test_config diff.ignoreSubmodules none &&
+       git diff HEAD >actual &&
+       sed -e "1,/^@@/d" actual >actual.body &&
        expect_from_to >expect.body $subtip $subprev-dirty &&
        test_cmp expect.body actual.body
 '

@@ -168,13 +176,13 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)'
                git clean -qfdx &&
                >cruft
        ) &&
-       git diff HEAD >actual &&
+       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 diff --ignore-submodules=all HEAD >actual2 &&
        test_must_be_empty actual2 &&
-       git diff --ignore-submodules=untracked HEAD >actual3 &&
+       git diff HEAD >actual3 &&
        test_must_be_empty actual3 &&
        git diff --ignore-submodules=dirty HEAD >actual4 &&
        test_must_be_empty actual4

t/t4041-diff-submodule-option.sh +8 -8

index f852136585..b3a7b7acaa 100755
@@ -262,7 +262,7 @@ test_expect_success 'submodule is up to date' '
 
 test_expect_success 'submodule contains untracked content' '
        echo new > sm1/new-file &&
-       git diff-index -p --submodule=log HEAD >actual &&
+       git diff-index -p --ignore-submodules=none --submodule=log HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 contains untracked content
        EOF

@@ -270,7 +270,7 @@ test_expect_success 'submodule contains untracked content' '
 '
 
 test_expect_success 'submodule contains untracked content (untracked ignored)' '
-       git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+       git diff-index -p --submodule=log HEAD >actual &&
        test_must_be_empty actual
 '
 

@@ -286,7 +286,7 @@ test_expect_success 'submodule contains untracked content (all ignored)' '
 
 test_expect_success 'submodule contains untracked and modified content' '
        echo new > sm1/foo6 &&
-       git diff-index -p --submodule=log HEAD >actual &&
+       git diff-index -p --ignore-submodules=none --submodule=log HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 contains untracked content
        Submodule sm1 contains modified content

@@ -296,7 +296,7 @@ test_expect_success 'submodule contains untracked and modified content' '
 
 test_expect_success 'submodule contains untracked and modified content (untracked ignored)' '
        echo new > sm1/foo6 &&
-       git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+       git diff-index -p --submodule=log HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 contains modified content
        EOF

@@ -337,7 +337,7 @@ test_expect_success 'submodule is modified' '
 
 test_expect_success 'modified submodule contains untracked content' '
        echo new > sm1/new-file &&
-       git diff-index -p --submodule=log HEAD >actual &&
+       git diff-index -p  --ignore-submodules=none --submodule=log HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 contains untracked content
        Submodule sm1 $head6..$head8:

@@ -347,7 +347,7 @@ test_expect_success 'modified submodule contains untracked content' '
 '
 
 test_expect_success 'modified submodule contains untracked content (untracked ignored)' '
-       git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+       git diff-index -p --submodule=log HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 $head6..$head8:
          > change

@@ -371,7 +371,7 @@ test_expect_success 'modified submodule contains untracked content (all ignored)
 
 test_expect_success 'modified submodule contains untracked and modified content' '
        echo modification >> sm1/foo6 &&
-       git diff-index -p --submodule=log HEAD >actual &&
+       git diff-index -p --ignore-submodules=none --submodule=log HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 contains untracked content
        Submodule sm1 contains modified content

@@ -383,7 +383,7 @@ test_expect_success 'modified submodule contains untracked and modified content'
 
 test_expect_success 'modified submodule contains untracked and modified content (untracked ignored)' '
        echo modification >> sm1/foo6 &&
-       git diff-index -p --ignore-submodules=untracked --submodule=log HEAD >actual &&
+       git diff-index -p --submodule=log HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 contains modified content
        Submodule sm1 $head6..$head8:

t/t4060-diff-submodule-option-diff-format.sh +8 -8

index fc8229c726..dc7b242697 100755
@@ -409,7 +409,7 @@ test_expect_success 'submodule is up to date' '
 
 test_expect_success 'submodule contains untracked content' '
        echo new > sm1/new-file &&
-       git diff-index -p --submodule=diff HEAD >actual &&
+       git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 contains untracked content
        EOF

@@ -417,7 +417,7 @@ test_expect_success 'submodule contains untracked content' '
 '
 
 test_expect_success 'submodule contains untracked content (untracked ignored)' '
-       git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+       git diff-index -p --submodule=diff HEAD >actual &&
        test_must_be_empty actual
 '
 

@@ -433,7 +433,7 @@ test_expect_success 'submodule contains untracked content (all ignored)' '
 
 test_expect_success 'submodule contains untracked and modified content' '
        echo new > sm1/foo6 &&
-       git diff-index -p --submodule=diff HEAD >actual &&
+       git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 contains untracked content
        Submodule sm1 contains modified content

@@ -451,7 +451,7 @@ test_expect_success 'submodule contains untracked and modified content' '
 # NOT OK
 test_expect_success 'submodule contains untracked and modified content (untracked ignored)' '
        echo new > sm1/foo6 &&
-       git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+       git diff-index -p --submodule=diff HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 contains modified content
        diff --git a/sm1/foo6 b/sm1/foo6

@@ -512,7 +512,7 @@ test_expect_success 'submodule is modified' '
 
 test_expect_success 'modified submodule contains untracked content' '
        echo new > sm1/new-file &&
-       git diff-index -p --submodule=diff HEAD >actual &&
+       git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 contains untracked content
        Submodule sm1 $head7..$head8:

@@ -528,7 +528,7 @@ test_expect_success 'modified submodule contains untracked content' '
 '
 
 test_expect_success 'modified submodule contains untracked content (untracked ignored)' '
-       git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+       git diff-index -p --submodule=diff HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 $head7..$head8:
        diff --git a/sm1/foo6 b/sm1/foo6

@@ -564,7 +564,7 @@ test_expect_success 'modified submodule contains untracked content (all ignored)
 
 test_expect_success 'modified submodule contains untracked and modified content' '
        echo modification >> sm1/foo6 &&
-       git diff-index -p --submodule=diff HEAD >actual &&
+       git diff-index -p --ignore-submodules=none --submodule=diff HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 contains untracked content
        Submodule sm1 contains modified content

@@ -583,7 +583,7 @@ test_expect_success 'modified submodule contains untracked and modified content'
 
 test_expect_success 'modified submodule contains untracked and modified content (untracked ignored)' '
        echo modification >> sm1/foo6 &&
-       git diff-index -p --ignore-submodules=untracked --submodule=diff HEAD >actual &&
+       git diff-index -p --submodule=diff HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 contains modified content
        Submodule sm1 $head7..$head8:

wt-status.c +3 -2

index 7074bbdd53..83d418647d 100644
@@ -606,7 +606,9 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
        if (s->ignore_submodule_arg) {
                rev.diffopt.flags.override_submodule_config = 1;
                handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
-       }
+       } else if (!rev.diffopt.flags.ignore_submodule_set &&
+                       s->show_untracked_files != SHOW_NO_UNTRACKED_FILES)
+               handle_ignore_submodules_arg(&rev.diffopt, "none");
        rev.diffopt.format_callback = wt_status_collect_changed_cb;
        rev.diffopt.format_callback_data = s;
        rev.diffopt.detect_rename = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;
--

2.21.1 (Apple Git-122.3)

Hey,

I worked on passing --ignore-submodules=none as the default behavior of git status so that the user doesn't end up deleting a submodule that has uncommitted (untracked) files.

The following changes make git status pass the ignoreSubmodules none argument as default.

(quoted diff) +1 -1

@@ -4587,7 +4587,7 @@ void repo_diff_setup(struct repository *r,
 struct diff_options *options)
        options->orderfile = diff_order_file_cfg;
 
        if (!options->flags.ignore_submodule_set)
-               handle_ignore_submodules_arg(options, "untracked");
+               options->flags.ignore_untracked_in_submodules = 1;
 
        if (diff_no_prefix) {

                options->a_prefix = options->b_prefix = "";

(quoted diff) +3 -0

@@ -607,6 +607,9 @@ static void
 wt_status_collect_changes_worktree(struct wt_status *s)
                rev.diffopt.flags.override_submodule_config = 1;
                handle_ignore_submodules_arg(&rev.diffopt,
 s->ignore_submodule_arg);
        }
+       else if(!rev.diffopt.flags.ignore_submodule_set){
+               handle_ignore_submodules_arg(&rev.diffopt, "none");
+       }
 

I have had to set the flag manually in diff.c because when we call handle_ignore_submodules_arg() with "untracked" arg, options->flags.ignore_submodule_set is set to 1 and therefore when we check for it in wt-status.c it appears that user has already set some config and therefore we shouldn't add "none" as ignoreSubmodules arg.

Another way to do that is to have one more flag in diff_options that can let us know whether options->flags.ignore_submodule_set was set by the user or by diff passing untracked as the default argument.

Can someone please help me with what might be the right way to proceed?

Hey Junio,

In the "What's cooking in git.git" you mentioned that this patch is left on doc update and some adjustments in "git status".