| Message ID | 20240514011437.3779151-3-gitster@pobox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix use of uninitialized hash algorithms | expand |
On Mon, May 13, 2024 at 06:14:34PM -0700, Junio C Hamano wrote: > A few commands, like "git apply" and "git patch-id", have been > broken with a recent change to stop setting the default hash > algorithm to SHA-1. Test them and fix them in later commits. Is there a specific reason why this needs a whole patch suite, as opposed to adding the tests to the respective test suites of the commands? > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > t/t1517-outside-repo.sh | 61 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > create mode 100755 t/t1517-outside-repo.sh > > diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh > new file mode 100755 > index 0000000000..e0fd495ec1 > --- /dev/null > +++ b/t/t1517-outside-repo.sh > @@ -0,0 +1,61 @@ > +#!/bin/sh > + > +test_description='check random commands outside repo' > + > +TEST_PASSES_SANITIZE_LEAK=true > +. ./test-lib.sh > + > +test_expect_success 'set up a non-repo directory and test file' ' > + GIT_CEILING_DIRECTORIES=$(pwd) && > + export GIT_CEILING_DIRECTORIES && > + mkdir non-repo && > + ( > + cd non-repo && > + # confirm that git does not find a repo > + test_must_fail git rev-parse --git-dir > + ) && > + test_write_lines one two three four >nums && > + git add nums && > + cp nums nums.old && > + test_write_lines five >>nums && > + git diff >sample.patch > +' We have the "nongit" command that does most of this for us. Patrick
Patrick Steinhardt <ps@pks.im> writes: > Is there a specific reason why this needs a whole patch suite, as > opposed to adding the tests to the respective test suites of the > commands? Yes, testing out-of-repository operations needs certain trick and people forget to write such tests using the GIT_CEILING_DIRECTORIES mechanism. Having one place where we have an enumeration of commands that are designed to be usable outside repository is a handy way to make sure that we have enough test coverage. It would make it easy to control how GIT_DEFAULT_HASH environment is set during these tests to have them in all one place.
On Tue, May 14, 2024 at 08:08:19AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > Is there a specific reason why this needs a whole patch suite, as > > opposed to adding the tests to the respective test suites of the > > commands? > > Yes, testing out-of-repository operations needs certain trick and > people forget to write such tests using the GIT_CEILING_DIRECTORIES > mechanism. Having one place where we have an enumeration of > commands that are designed to be usable outside repository is a > handy way to make sure that we have enough test coverage. It would > make it easy to control how GIT_DEFAULT_HASH environment is set > during these tests to have them in all one place. We already have the "nogit" command that neatly encapsulates all of this logic, so the trickery is contained in a single spot in practice. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Tue, May 14, 2024 at 08:08:19AM -0700, Junio C Hamano wrote: >> Patrick Steinhardt <ps@pks.im> writes: >> >> > Is there a specific reason why this needs a whole patch suite, as >> > opposed to adding the tests to the respective test suites of the >> > commands? >> >> Yes, testing out-of-repository operations needs certain trick and >> people forget to write such tests using the GIT_CEILING_DIRECTORIES >> mechanism. Having one place where we have an enumeration of >> commands that are designed to be usable outside repository is a >> handy way to make sure that we have enough test coverage. It would >> make it easy to control how GIT_DEFAULT_HASH environment is set >> during these tests to have them in all one place. > > We already have the "nogit" command that neatly encapsulates all of this > logic, so the trickery is contained in a single spot in practice. Heh, you asked for "a" specific reason, and I listed three. The tests that are spread across many scripts make it harder to see if we have enough coverage for the out-of-repository operations, and the use of "nongit" helper does not change the equation, does it?
On Wed, May 15, 2024 at 07:15:49AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > On Tue, May 14, 2024 at 08:08:19AM -0700, Junio C Hamano wrote: > >> Patrick Steinhardt <ps@pks.im> writes: > >> > >> > Is there a specific reason why this needs a whole patch suite, as > >> > opposed to adding the tests to the respective test suites of the > >> > commands? > >> > >> Yes, testing out-of-repository operations needs certain trick and > >> people forget to write such tests using the GIT_CEILING_DIRECTORIES > >> mechanism. Having one place where we have an enumeration of > >> commands that are designed to be usable outside repository is a > >> handy way to make sure that we have enough test coverage. It would > >> make it easy to control how GIT_DEFAULT_HASH environment is set > >> during these tests to have them in all one place. > > > > We already have the "nogit" command that neatly encapsulates all of this > > logic, so the trickery is contained in a single spot in practice. > > Heh, you asked for "a" specific reason, and I listed three. Hm. I guess I got thrown off a bit as this was written in a single paragraph, so my mind didn't process it as different reasons :) > The tests that are spread across many scripts make it harder to see if > we have enough coverage for the out-of-repository operations, Put this way I in fact agree with you. > and the use of "nongit" helper does not change the equation, does it? No, it doesn't really. Thanks! Patrick
Patrick Steinhardt <ps@pks.im> writes: >> The tests that are spread across many scripts make it harder to see if >> we have enough coverage for the out-of-repository operations, > > Put this way I in fact agree with you. > >> and the use of "nongit" helper does not change the equation, does it? > > No, it doesn't really. Of course the other side of the coin is that having all tests about a single command (say, "git mailmap") in the same script for both in-repo and out-of-repo operations is also handy in a different way. I haven't found a good way to balance the two. Obviously duplicating the same test in two scripts (one collection for the same command, the other collection out-of repo operation of various commands) is something I really do want to avoid, so...
diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh new file mode 100755 index 0000000000..e0fd495ec1 --- /dev/null +++ b/t/t1517-outside-repo.sh @@ -0,0 +1,61 @@ +#!/bin/sh + +test_description='check random commands outside repo' + +TEST_PASSES_SANITIZE_LEAK=true +. ./test-lib.sh + +test_expect_success 'set up a non-repo directory and test file' ' + GIT_CEILING_DIRECTORIES=$(pwd) && + export GIT_CEILING_DIRECTORIES && + mkdir non-repo && + ( + cd non-repo && + # confirm that git does not find a repo + test_must_fail git rev-parse --git-dir + ) && + test_write_lines one two three four >nums && + git add nums && + cp nums nums.old && + test_write_lines five >>nums && + git diff >sample.patch +' + +test_expect_failure 'compute a patch-id outside repository' ' + git patch-id <sample.patch >patch-id.expect && + ( + cd non-repo && + git patch-id <../sample.patch >../patch-id.actual + ) && + test_cmp patch-id.expect patch-id.actual +' + +test_expect_failure 'hash-object outside repository' ' + git hash-object --stdin <sample.patch >hash.expect && + ( + cd non-repo && + git hash-object --stdin <../sample.patch >../hash.actual + ) && + test_cmp hash.expect hash.actual +' + +test_expect_failure 'apply a patch outside repository' ' + ( + cd non-repo && + cp ../nums.old nums && + git apply ../sample.patch + ) && + test_cmp nums non-repo/nums +' + +test_expect_success 'grep outside repository' ' + git grep --cached two >expect && + ( + cd non-repo && + cp ../nums.old nums && + git grep --no-index two >../actual + ) && + test_cmp expect actual +' + +test_done
A few commands, like "git apply" and "git patch-id", have been broken with a recent change to stop setting the default hash algorithm to SHA-1. Test them and fix them in later commits. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- t/t1517-outside-repo.sh | 61 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100755 t/t1517-outside-repo.sh