| Message ID | 20240513192112.866021-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 12:21 PM Junio C Hamano <gitster@pobox.com> 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. > > 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 > +' > + > +test_expect_failure 'compute a patch-id outside repository' ' Do we only expect failure because of a temporary condition (the bug that is mentioned in the commit message)? If so, we should probably add a TODO, FIXME, or some other similar style of comment that describes that this should be fixed. This way, if the patch series to fix the issue doesn't materialize, people don't read the test file and think that these commands aren't supported outside of a repository. Do we have a way of catching the specific failure mode? i.e. if it crashes, is there a test_expect_crash? I'm thinking that it might be nice to be more specific about what kind of failure we expect, this way if it fails in a different way before we convert these to test_expect_success, the test fails (due to the unexpected change). I'm assuming that for crashes of this type there's no good/portable way of verifying that it's a specific crash, but having the test check for a difference between an exit code that indicates a signal was raised and an exit code that indicates that the process returned "naturally" after an unsuccessful execution might be feasible, if we already have such a mechanism. Adding one just for this series doesn't seem justified. > + 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 > -- > 2.45.0-145-g3e4a232f6e > >
Kyle Lippincott <spectral@google.com> writes: > Do we only expect failure because of a temporary condition (the bug > that is mentioned in the commit message)? If so, we should probably > add a TODO, FIXME, or some other similar style of comment that > describes that this should be fixed. test_expect_failure is description enough for that purpose. If a command should NOT work outside the project we will write a test more like so: test_expect_success 'foo does not work outside' ' ... prepare that $cwd is outside ... test_must_fail git foo '
Junio C Hamano <gitster@pobox.com> writes: > Kyle Lippincott <spectral@google.com> writes: > >> Do we only expect failure because of a temporary condition (the bug >> that is mentioned in the commit message)? If so, we should probably >> add a TODO, FIXME, or some other similar style of comment that >> describes that this should be fixed. > > test_expect_failure is description enough for that purpose. We say this in t/README: - test_expect_failure [<prereq>] <message> <script> This is NOT the opposite of test_expect_success, but is used to mark a test that demonstrates a known breakage. Unlike the usual test_expect_success tests, which say "ok" on success and "FAIL" on failure, this will say "FIXED" on success and "still broken" on failure. Failures from these tests won't cause -i (immediate) to stop. Which means that when somebody rans out of things to do, grepping for test_expect_failure may give them a good place to start ;-). Note that there were a few very rare occasions that what was marked as "known breakage" with test_expect_failure turned out to be what was working as intended. Thanks.
On Mon, May 13, 2024 at 2:00 PM Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > Kyle Lippincott <spectral@google.com> writes: > > > >> Do we only expect failure because of a temporary condition (the bug > >> that is mentioned in the commit message)? If so, we should probably > >> add a TODO, FIXME, or some other similar style of comment that > >> describes that this should be fixed. > > > > test_expect_failure is description enough for that purpose. > > We say this in t/README: > > - test_expect_failure [<prereq>] <message> <script> > > This is NOT the opposite of test_expect_success, but is used > to mark a test that demonstrates a known breakage. Unlike > the usual test_expect_success tests, which say "ok" on > success and "FAIL" on failure, this will say "FIXED" on > success and "still broken" on failure. Failures from these > tests won't cause -i (immediate) to stop. Got it, thanks for explaining. With that, this change looks good to me. > > Which means that when somebody rans out of things to do, grepping > for test_expect_failure may give them a good place to start ;-). > > Note that there were a few very rare occasions that what was marked > as "known breakage" with test_expect_failure turned out to be what > was working as intended. > > Thanks.
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