| Message ID | 0e38da05-efd6-451e-bd8a-b2b3457c0c75@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add: use advise_if_enabled | expand |
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. > We don't have a test for this. Add one. > > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > builtin/add.c | 6 +++--- > t/t3700-add.sh | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 35 insertions(+), 3 deletions(-) > > diff --git a/builtin/add.c b/builtin/add.c > index 289adaaecf..e97699d6b9 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -310,9 +310,9 @@ static void check_embedded_repo(const char *path) > strbuf_strip_suffix(&name, "/"); > > warning(_("adding embedded git repository: %s"), name.buf); > - if (!adviced_on_embedded_repo && > - advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) { > - advise(embedded_advice, name.buf, name.buf); > + if (!adviced_on_embedded_repo) { > + advise_if_enabled(ADVICE_ADD_EMBEDDED_REPO, > + embedded_advice, name.buf, name.buf); > adviced_on_embedded_repo = 1; > } This uses a static variable "adviced_on_embedded_repo" to skip giving the advice messages over and over. The patch preserves that feature of this code while updating it to use the "if_enabled" variant. > diff --git a/t/t3700-add.sh b/t/t3700-add.sh > index 681081e0d5..2b92f3eb5b 100755 > --- a/t/t3700-add.sh > +++ b/t/t3700-add.sh > @@ -349,6 +349,38 @@ test_expect_success '"git add ." in empty repo' ' > ) > ' > > +test_expect_success '"git add" a nested repository' ' "nested" -> "embedded", as the warning, advice_type and the message contents all use "embedded" consistently. > + 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. > + cat >expect <<-EOF && > + warning: adding embedded git repository: empty > + hint: You${SQ}ve added another git repository inside your current repository. > + hint: Clones of the outer repository will not contain the contents of > + hint: the embedded repository and will not know how to obtain it. > + hint: If you meant to add a submodule, use: > + hint: > + hint: git submodule add <url> empty > + hint: > + hint: If you added this path by mistake, you can remove it from the > + hint: index with: > + hint: > + hint: git rm --cached empty > + hint: > + hint: See "git help submodule" for more information. > + hint: Disable this message with "git config advice.addEmbeddedRepo false" > + EOF > + test_cmp expect actual > + ) > +' > + > test_expect_success 'error on a repository with no commits' ' > rm -fr empty && > git init empty &&
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.
Rubén Justo <rjusto@gmail.com> writes: >> 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"? That is merely shifting justification around. You'd now need to answer: Why do you consider it preferred? A few obvious advantages of using if_enabled() form are: - you do not have to give "here is how to disable this piece of advice" yourself. - the code can become shorter. and they may make it preferred to use it, when appropriate. > Good point about the naming. I'm not so sure about the "only once" > part, but I do not have any strong objection. I am not sure what you are not sure about ;-). If you are adding a test for a feature because it is not covered by existing tests, and if that feature consists of two parts, it is naturally expected of you to cover both parts with the new test, unless there is a strong reason not to. No? I would understand if you said because of such and such reasons, we should not cover the "two instances of violation, only one advice" half of the feature, though.
On Fri, Mar 29, 2024 at 12:31:30PM -0700, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > >> 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"? > > That is merely shifting justification around. You'd now need to > answer: Why do you consider it preferred? Because it's newer ;-D Maybe I'll point to the commit where advise_if_enabled() was introduced, b3b18d1621 (advice: revamp advise API, 2020-03-02). We have some arguments there. I'll sleep on it.
Rubén Justo <rjusto@gmail.com> writes: > On Fri, Mar 29, 2024 at 12:31:30PM -0700, Junio C Hamano wrote: >> Rubén Justo <rjusto@gmail.com> writes: >> >> >> 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"? >> >> That is merely shifting justification around. You'd now need to >> answer: Why do you consider it preferred? > > Because it's newer ;-D A newer thing is not necessarily better, though. > Maybe I'll point to the commit where advise_if_enabled() was introduced, > b3b18d1621 (advice: revamp advise API, 2020-03-02). We have some > arguments there. I'll sleep on it. I think I've already given my verison of justification in the message you are responding to. I'll stop spending time on this topic while you are sleeping on it ;-)
On Fri, Mar 29, 2024 at 12:31:30PM -0700, Junio C Hamano wrote: > > Good point about the naming. I'm not so sure about the "only once" > > part, but I do not have any strong objection. > > I am not sure what you are not sure about ;-). > > If you are adding a test for a feature because it is not covered by > existing tests, and if that feature consists of two parts, it is > naturally expected of you to cover both parts with the new test, > unless there is a strong reason not to. No? Sure. However, I was not seeing that a whole. Only testing the advice message seemed sensible to me. However, your proposed test covers the whole feature, and it shouldn't be too challenging to modify the advice's text if we need to. So, it is better. Thanks.
diff --git a/builtin/add.c b/builtin/add.c index 289adaaecf..e97699d6b9 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -310,9 +310,9 @@ static void check_embedded_repo(const char *path) strbuf_strip_suffix(&name, "/"); warning(_("adding embedded git repository: %s"), name.buf); - if (!adviced_on_embedded_repo && - advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) { - advise(embedded_advice, name.buf, name.buf); + if (!adviced_on_embedded_repo) { + advise_if_enabled(ADVICE_ADD_EMBEDDED_REPO, + embedded_advice, name.buf, name.buf); adviced_on_embedded_repo = 1; } diff --git a/t/t3700-add.sh b/t/t3700-add.sh index 681081e0d5..2b92f3eb5b 100755 --- a/t/t3700-add.sh +++ b/t/t3700-add.sh @@ -349,6 +349,38 @@ test_expect_success '"git add ." in empty repo' ' ) ' +test_expect_success '"git add" a nested repository' ' + rm -fr empty && + git init empty && + ( + cd empty && + git init empty && + ( + cd empty && + git commit --allow-empty -m "foo" + ) && + git add empty 2>actual && + cat >expect <<-EOF && + warning: adding embedded git repository: empty + hint: You${SQ}ve added another git repository inside your current repository. + hint: Clones of the outer repository will not contain the contents of + hint: the embedded repository and will not know how to obtain it. + hint: If you meant to add a submodule, use: + hint: + hint: git submodule add <url> empty + hint: + hint: If you added this path by mistake, you can remove it from the + hint: index with: + hint: + hint: git rm --cached empty + hint: + hint: See "git help submodule" for more information. + hint: Disable this message with "git config advice.addEmbeddedRepo false" + EOF + test_cmp expect actual + ) +' + test_expect_success 'error on a repository with no commits' ' rm -fr empty && git init empty &&
Use the newer advise_if_enabled() machinery to show the advice. We don't have a test for this. Add one. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- builtin/add.c | 6 +++--- t/t3700-add.sh | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-)