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

[Junio C Hamano <gitster@pobox.com>]

Sangeeta NB <sangunb09@gmail.com> writes:

>> > -     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?

OK, I re-read the changes to the tests in v5 (an excerpt at the end
of this message), and I think this one makes sort-of sense.

We want to see that a plain-vanilla invocation of "git diff" without
custom "--ignore-submodules" option would stop saying -dirty when
untracked cruft is in the submodule's working tree, and this test,
as it says in its title, is exactly testing that case (i.e. "how is
a submodule without change relative to the gitlink in the
superproject shown when there is untracked cruft there?").  It is a
perfect opportunity to give the answer to the question and how that
answer will be different from the current system if we took this
patch.  We used to expect to see that submodule has changed from the
commit subprev to the same commit subprev with dirty indicator.
What should we expect under the new world order?  We should stop
claiming that submodule has any change.

So I would have expected that would be shown by the first step in
first "what happens without custom --ignore-submodules invocation?"
test by changing the expect_from_to that corresponds to it, and then
a *new* test added to show that with "--ignore-submodules=none" it
is still possible to get "changed from the same commit to the same
commit with the -dirty indicator" as a second test.

As long as all cases (i.e. all possible values to --ignore-submodules=<what>,
plus the case where --ignore-submodules=<what> is not given at all) are
covered, it is OK, but I think it is the best to show the case without
an option, especially when the original does so.

Thanks.

@@ -168,13 +177,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