| Message ID | 20200805174921.16000-5-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t7401: modernize, cleanup and warn | expand |
On Wed, Aug 05, 2020 at 11:19:21PM +0530, Shourya Shukla wrote: > Add a WARNING regarding the usage of 'git add' instead of 'git > submodule add' to add submodules to the superproject. Also add a > NEEDSWORK regarding the outdated syntax and working of the test, which > may need to be improved to obtain better and desired results. > > While at it, change the word 'test' to 'test script' in the test > description to avoid ambiguity. > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > --- > t/t7401-submodule-summary.sh | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh > index 145914cd5a..2db4cf5cbf 100755 > --- a/t/t7401-submodule-summary.sh > +++ b/t/t7401-submodule-summary.sh > @@ -5,8 +5,13 @@ > > test_description='Summary support for submodules > > -This test tries to verify the sanity of summary subcommand of git submodule. > +This test script tries to verify the sanity of summary subcommand of git submodule. > ' > +# WARNING: This test script uses 'git add' instead of 'git submodule add' to add > +# submodules to the superproject. Some submodule subcommands such as init and > +# deinit might not work as expected in this script. > + > +# NEEDSWORK: This test script is old fashioned and may need a big cleanup. It would be worth saying why, especially if you're re-rolling anyway. Even something as simple as: "there are lots of commands taking place outside of a 'test_expect_{success,failure}' block, which is no longer in good-style". I also wouldn't be upset to see some of those fixed up in this series, but I realize that may be a bigger endeavor than you are willing to take on for now. > > . ./test-lib.sh > > -- > 2.27.0 Thanks, Taylor
Shourya Shukla <shouryashukla.oo@gmail.com> writes: > Add a WARNING regarding the usage of 'git add' instead of 'git > submodule add' to add submodules to the superproject. Is that a warning worthy thing? As far as I know, using "git add" to register a gitlink is perfectly fine and a supported way to start a new submodule. It may have to be followed by other steps like "git config -f .gitmodules" (e.g. when operations that needs to use the contents recorded in the .gitmodules file are to be tested), but writing tests using lower-level ingredients for finer grained tests is not all that unusual, is it? I dunno. > NEEDSWORK regarding the outdated syntax and working of the test, which > may need to be improved to obtain better and desired results. Sounds good. > While at it, change the word 'test' to 'test script' in the test > description to avoid ambiguity. Sounds good. I often search for a pair of phrases to differentiate a single tXXXX-name.sh file as a whole and an individual test piece in it. "This test script", especially when written near the beginning of the file, is a good way to clearly convey that you want to refer to the former. > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > --- > t/t7401-submodule-summary.sh | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh > index 145914cd5a..2db4cf5cbf 100755 > --- a/t/t7401-submodule-summary.sh > +++ b/t/t7401-submodule-summary.sh > @@ -5,8 +5,13 @@ > > test_description='Summary support for submodules > > -This test tries to verify the sanity of summary subcommand of git submodule. > +This test script tries to verify the sanity of summary subcommand of git submodule. > ' > +# WARNING: This test script uses 'git add' instead of 'git submodule add' to add > +# submodules to the superproject. Some submodule subcommands such as init and > +# deinit might not work as expected in this script. > + > +# NEEDSWORK: This test script is old fashioned and may need a big cleanup. > > . ./test-lib.sh
On 05/08 02:36, Junio C Hamano wrote: > Shourya Shukla <shouryashukla.oo@gmail.com> writes: > > > Add a WARNING regarding the usage of 'git add' instead of 'git > > submodule add' to add submodules to the superproject. > > Is that a warning worthy thing? As far as I know, using "git add" > to register a gitlink is perfectly fine and a supported way to start > a new submodule. It may have to be followed by other steps like > "git config -f .gitmodules" (e.g. when operations that needs to use > the contents recorded in the .gitmodules file are to be tested), but > writing tests using lower-level ingredients for finer grained tests > is not all that unusual, is it? I dunno. The thing is that 'git submodule {init,deinit}' fail when there is no .gitmodules. I can initiliase the .gitmodules separately using 'git config -f .gitmodules' but I think it will be better to use 'git submodule add' throughout the script rather than worry about it all the time. But again, if the warning seems unnecessary, then I can obviously use 'git config' to initilaise the submodules and change this commit. What do you reckon? > > NEEDSWORK regarding the outdated syntax and working of the test, which > > may need to be improved to obtain better and desired results. > > Sounds good. > > > While at it, change the word 'test' to 'test script' in the test > > description to avoid ambiguity. > > Sounds good. I often search for a pair of phrases to differentiate > a single tXXXX-name.sh file as a whole and an individual test piece > in it. "This test script", especially when written near the > beginning of the file, is a good way to clearly convey that you want > to refer to the former. > > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > > --- > > t/t7401-submodule-summary.sh | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh > > index 145914cd5a..2db4cf5cbf 100755 > > --- a/t/t7401-submodule-summary.sh > > +++ b/t/t7401-submodule-summary.sh > > @@ -5,8 +5,13 @@ > > > > test_description='Summary support for submodules > > > > -This test tries to verify the sanity of summary subcommand of git submodule. > > +This test script tries to verify the sanity of summary subcommand of git submodule. > > ' > > +# WARNING: This test script uses 'git add' instead of 'git submodule add' to add > > +# submodules to the superproject. Some submodule subcommands such as init and > > +# deinit might not work as expected in this script. > > + > > +# NEEDSWORK: This test script is old fashioned and may need a big cleanup. > > > > . ./test-lib.sh
Shourya Shukla <shouryashukla.oo@gmail.com> writes: > On 05/08 02:36, Junio C Hamano wrote: >> Shourya Shukla <shouryashukla.oo@gmail.com> writes: >> >> > Add a WARNING regarding the usage of 'git add' instead of 'git >> > submodule add' to add submodules to the superproject. >> >> Is that a warning worthy thing? As far as I know, using "git add" >> to register a gitlink is perfectly fine and a supported way to start >> a new submodule. It may have to be followed by other steps like >> "git config -f .gitmodules" (e.g. when operations that needs to use >> the contents recorded in the .gitmodules file are to be tested), but >> writing tests using lower-level ingredients for finer grained tests >> is not all that unusual, is it? I dunno. > > The thing is that 'git submodule {init,deinit}' fail when there is no > .gitmodules. I can initiliase the .gitmodules separately using 'git > config -f .gitmodules' but I think it will be better to use 'git > submodule add' throughout the script rather than worry about it all the > time. On the other hand, we do want to make sure that the workflow using lower-level tools continues to work, so that is a balancing act. > But again, if the warning seems unnecessary, then I can obviously use > 'git config' to initilaise the submodules and change this commit. What > do you reckon? If you need "git submodule init" etc. to work in this test, yes, you can change the test to either use "git submodule add" instead, or "git config -f .gitmodules" in addition. If you don't, there is no need to change anything, no?
On Thu, Aug 6, 2020 at 11:11 PM Junio C Hamano <gitster@pobox.com> wrote: > > Shourya Shukla <shouryashukla.oo@gmail.com> writes: > > > On 05/08 02:36, Junio C Hamano wrote: > >> Shourya Shukla <shouryashukla.oo@gmail.com> writes: > >> > >> > Add a WARNING regarding the usage of 'git add' instead of 'git > >> > submodule add' to add submodules to the superproject. > >> > >> Is that a warning worthy thing? As far as I know, using "git add" > >> to register a gitlink is perfectly fine and a supported way to start > >> a new submodule. It may have to be followed by other steps like > >> "git config -f .gitmodules" (e.g. when operations that needs to use > >> the contents recorded in the .gitmodules file are to be tested), but > >> writing tests using lower-level ingredients for finer grained tests > >> is not all that unusual, is it? I dunno. > > > > The thing is that 'git submodule {init,deinit}' fail when there is no > > .gitmodules. I can initiliase the .gitmodules separately using 'git > > config -f .gitmodules' but I think it will be better to use 'git > > submodule add' throughout the script rather than worry about it all the > > time. > > On the other hand, we do want to make sure that the workflow using > lower-level tools continues to work, so that is a balancing act. Yeah, that's the reason why we suggested to add the new tests in a new test script t7421: https://lore.kernel.org/git/20200806164102.6707-5-shouryashukla.oo@gmail.com/ > > But again, if the warning seems unnecessary, then I can obviously use > > 'git config' to initilaise the submodules and change this commit. What > > do you reckon? > > If you need "git submodule init" etc. to work in this test, yes, you > can change the test to either use "git submodule add" instead, or > "git config -f .gitmodules" in addition. If you don't, there is no > need to change anything, no? In t7421 "git submodule add" is used, so that we can perform the new tests we want there. The purpose of this patch adding a WARNING and a NEEDSWORK was to tell explicitly that this test script (t7401) is not necessarily the best test script for "git submodule summary" tests because it is very old fashioned as it has a lot of boilerplate stuff outside test_expect_{success, failure} and it uses "git add" instead of "git submodule add". I think we might want to just add the NEEDSWORK in this patch series and add the WARNING, or perhaps just a NOTE, about "git add" vs "git submodule add" in the other patch series when we introduce t7421.
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh index 145914cd5a..2db4cf5cbf 100755 --- a/t/t7401-submodule-summary.sh +++ b/t/t7401-submodule-summary.sh @@ -5,8 +5,13 @@ test_description='Summary support for submodules -This test tries to verify the sanity of summary subcommand of git submodule. +This test script tries to verify the sanity of summary subcommand of git submodule. ' +# WARNING: This test script uses 'git add' instead of 'git submodule add' to add +# submodules to the superproject. Some submodule subcommands such as init and +# deinit might not work as expected in this script. + +# NEEDSWORK: This test script is old fashioned and may need a big cleanup. . ./test-lib.sh
Add a WARNING regarding the usage of 'git add' instead of 'git submodule add' to add submodules to the superproject. Also add a NEEDSWORK regarding the outdated syntax and working of the test, which may need to be improved to obtain better and desired results. While at it, change the word 'test' to 'test script' in the test description to avoid ambiguity. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t7401-submodule-summary.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)