On Fri, Mar 29, 2024 at 10:55:56AM -0700, Junio C Hamano wrote:

Rubén Justo [off-list ref] writes:

quoted

Use the newer advise_if_enabled() machinery to show the advice.

Common to the other two patches, but "Newer" is not a good enough
excuse if the existing code is working well for us and not being
maintenance burden.  The previous two patches were helped by use of
advise_if_enabled() in a concrete way (or perhaps two ways), and
that should be explained when selling them.

This one also needs a similar justification, but with a twist.

May I ask what you would find a good justification?

Perhaps "newer" -> "now preferred"?

quoted

+test_expect_success '"git add" a nested repository' '

"nested" -> "embedded", as the warning, advice_type and the message
contents all use "embedded" consistently.

Makes sense.

quoted

+	rm -fr empty &&
+	git init empty &&
+	(
+		cd empty &&
+		git init empty &&
+		(
+			cd empty &&
+			git commit --allow-empty -m "foo"
+		) &&
+		git add empty 2>actual &&

It is very good to add a test for a feature that we failed to cover
so far.  But the feature, as we seen above, is twofold.  We see an
advice, and we it see only once even when we have multiple.

So we should add two such embedded repositories for the test, no?
Also, the shell repository is not meant to stay empty as the user
will make a mistaken attempt to "add" something to it.

Perhaps the above part would become more like:

	rm -rf outer && git init outer &&
	(
		cd outer &&
		for i in 1 2
		do
			name=inner$i &&
			git init $name &&
                        git -C $name --allow-empty -m $name ||
				return 1
		done &&
                git add . 2>actual &&

to use a more descriptive name that shows the point of the test (it
is not interesting that they are empty---they are in "outer contains
innner repositories" relationship and that is what the test wants to
make), and ensure "only once" part of the feature we are testing.

Good point about the naming.  I'm not so sure about the "only once"
part, but I do not have any strong objection.

Thanks.