Re: [PATCH 1/2] t7501: Add tests for various index usages, -i and -o, of commit command.

From: Christian Couder <christian.couder@gmail.com>
Date: Jan 9, 2024, 09:20 UTC
Message-ID: <CAP8UFD0GJf5+eOTxy6s+zCzpDmCU_FY4BjwtjTE7RvZ5mKettA@mail.gmail.com>
In-Reply-To: <20240109060417.1144647-3-shyamthakkar001@gmail.com>

First about the commit subject:

"t7501: Add tests for various index usages, -i and -o, of commit command."

it should be shorter, shouldn't end with a "." and "Add" should be "add".

On Tue, Jan 9, 2024 at 7:10 AM Ghanshyam Thakkar <shyamthakkar001@gmail.com> wrote:

>
> This commit adds tests for -i and -o flags of the commit command. It
> includes testing -i with and without staged changes, testing -o with and
> without staged changes, and testing -i and -o together.

A few suggestions to make things a bit more clear:

  - tell that -i is a synonymous for --include and -o for --only
  - tell if there are already tests for these options
  - tell why the tests you add are worth it if tests for an option already exist

Show 19 quoted lines

> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
>  t/t7501-commit-basic-functionality.sh | 90 +++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>
> diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
> index 3d8500a52e..71dc52ce3a 100755
> --- a/t/t7501-commit-basic-functionality.sh
> +++ b/t/t7501-commit-basic-functionality.sh
> @@ -76,6 +76,96 @@ test_expect_success 'nothing to commit' '
>         test_must_fail git commit -m initial
>  '
>
> +test_expect_success 'commit with -i fails with untracked files' '
> +       test_when_finished "rm -rf testdir" &&
> +       git init testdir &&
> +       echo content >testdir/file.txt &&
> +       test_must_fail git -C testdir commit -i file.txt -m initial
> +'

Existing tests didn't need a repo, so I am not sure it's worth creating a testdir repo just for this test.

Also I am not sure this is the best place in the test script to add -i and -o related tests. Here we are between a 'nothing to commit' test and a '--dry-run fails with nothing to commit' and then more 'nothing to commit' related tests. I think it would be better if all those 'nothing to commit' related tests could stay together.

> +test_expect_success 'commit with -i' '
> +       echo content >bar &&
> +       git add bar &&
> +       git commit -m "bar" &&

Why is the above needed for this test?

Show 11 quoted lines

> +       echo content2 >bar &&
> +       git commit -i bar -m "bar second"
> +'
> +
> +test_expect_success 'commit with -i multiple files' '
> +       test_when_finished "git reset --hard" &&
> +       echo content >bar &&
> +       echo content >baz &&
> +       echo content >saz &&
> +       git add bar baz saz &&
> +       git commit -m "bar baz saz" &&

Not sure why the above is needed here too.

Show 40 quoted lines

> +       echo content2 >bar &&
> +       echo content2 >baz &&
> +       echo content2 >saz &&
> +       git commit -i bar saz -m "bar" &&
> +       git diff --name-only >remaining &&
> +       test_grep "baz" remaining
> +'
> +
> +test_expect_success 'commit with -i includes staged changes' '
> +       test_when_finished "git reset --hard" &&
> +       echo content >bar &&
> +       git add bar &&
> +       git commit -m "bar" &&
> +
> +       echo content2 >bar &&
> +       echo content2 >baz &&
> +       git add baz &&
> +       git commit -i bar -m "bar baz" &&
> +       git diff --name-only >remaining &&
> +       test_must_be_empty remaining &&
> +       git diff --name-only --staged >remaining &&
> +       test_must_be_empty remaining
> +'
> +
> +test_expect_success 'commit with -o' '
> +       echo content >bar &&
> +       git add bar &&
> +       git commit -m "bar" &&
> +       echo content2 >bar &&
> +       git commit -o bar -m "bar again"
> +'
> +
> +test_expect_success 'commit with -o fails with untracked files' '
> +       test_when_finished "rm -rf testdir" &&
> +       git init testdir &&
> +       echo content >testdir/bar &&
> +       test_must_fail git -C testdir commit -o bar -m "bar"
> +'
> +
> +test_expect_success 'commit with -o exludes staged changes' '

s/exludes/excludes/

Show 13 quoted lines

> +       test_when_finished "git reset --hard" &&
> +       echo content >bar &&
> +       echo content >baz &&
> +       git add . &&
> +       git commit -m "bar baz" &&
> +
> +       echo content2 >bar &&
> +       echo content2 >baz &&
> +       git add baz &&
> +       git commit -o bar -m "bar" &&
> +       git diff --name-only --staged >actual &&
> +       test_grep "baz" actual
> +'

Thanks.