Re: [PATCH v2 2/4] t1517: test commands that are designed to be run outside repository

From: Kyle Lippincott <spectral@google.com>
Date: May 13, 2024, 19:57 UTC
Message-ID: <CAO_smVg7mdQHp+sg4-vVUEOVRBEoZjqXZu=Kk8PuYc9mtYeMXg@mail.gmail.com>
In-Reply-To: <20240513192112.866021-3-gitster@pobox.com>

On Mon, May 13, 2024 at 12:21 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 41 quoted lines

>
> 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.

Show 41 quoted lines

> +       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
>
>