Re: [PATCH 3/3] add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO

["Rubén Justo" <rjusto@gmail.com>]

On Fri, Mar 29, 2024 at 10:55:56AM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > 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"?

> > +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.

> > +	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.