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