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

From: "Ghanshyam Thakkar" <shyamthakkar001@gmail.com>
To: "Christian Couder" <christian.couder@gmail.com>
Cc: <git@vger.kernel.org>
Subject: Re: [PATCH 1/2] t7501: Add tests for various index usages, -i and -o, of commit command.
Date: Tue, 09 Jan 2024 22:40:20 +0530	[thread overview]
Message-ID: <CYACBX8Y10KJ.3QVLHQN8LLZ86@gmail.com> (raw)
In-Reply-To: <CAP8UFD0GJf5+eOTxy6s+zCzpDmCU_FY4BjwtjTE7RvZ5mKettA@mail.gmail.com>

On Tue Jan 9, 2024 at 2:50 PM IST, Christian Couder wrote:
> 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".
Updated in v2.

> 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

I have updated the commit messages in v2 to address these points.

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

Yes, I just wanted to make sure the files were not tracked. However, I
have updated these instaces to use the existing repo and use unique
non-generic names for generating untracked files.

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

I have moved these tests above the "--amend" related tests, which do not
break the flow of the tests.

> > +test_expect_success 'commit with -i' '
> > +       echo content >bar &&
> > +       git add bar &&
> > +       git commit -m "bar" &&
>
> Why is the above needed for this test?
This was to make sure that the file is tracked, however, I realised that
committing is not necessary, so I have updated this test to use existing
files in repo and to not generate a new one.
>
> > +       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.
Similar to above, I have updated this test to use existing files in repo
and to not generate a new one.
>
> > +       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/
Done.
>
> > +       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.

Thanks for the review!