[PATCH 0/2][GSOC] t7501: Add tests for various index usages, -i and -o, of commit command and amending commit to add signoff.

[PATCH 0/2][GSOC] t7501: Add tests for various index usages, -i and -o, of commit command and amending commit to add signoff.

[Ghanshyam Thakkar]

This patch series adds tests for various index usages, -i and -o, of commit
command and amending commit to add Signed-of-by trailer. This is in
reference to the comment added in commit 12ace0b2 which reads:

  # FIXME: Test the various index usages, -i and -o, test reflog,
  # signoff, hooks

Ghanshyam Thakkar (2):
  t7501: Add tests for various index usages, -i and -o, of commit
    command.
  t7501: Add test for amending commit to add signoff.

 t/t7501-commit-basic-functionality.sh | 108 ++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

-- 
2.43.0

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

[Ghanshyam Thakkar]

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.

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
+'
+
+test_expect_success 'commit with -i' '
+	echo content >bar &&
+	git add bar &&
+	git commit -m "bar" &&
+
+	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" &&
+
+	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' '
+	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
+'
+
+test_expect_success 'commit with both -i and -o fails' '
+	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 &&
+	test_must_fail git commit -i baz -o bar -m "bar"
+'
+
 test_expect_success '--dry-run fails with nothing to commit' '
 	test_must_fail git commit -m initial --dry-run
 '
-- 
2.43.0

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

[Christian Couder]

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

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

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

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

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

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

[Ghanshyam Thakkar]

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!

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

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

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

Overly long subject that has an unusual capitalization after
"t7501:" (see "git log --no-merges --format=%s -20 v2.43.0" for
example and try to write something that blends better).

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

In addition to "why a new repository???" comment raised already, I
do not want to see the last command spelled like so.  Always write
dashed options (and their parameters) before non-option arguments,
i.e.

	git commit -i -m initial file.txt
	git -C testdir  commit -i -m initial file.txt
	test_must_fail git -C testdir commit -i -m initial file.txt

The command line parser does rotate the unrecognized arguments to
the end and keeps looking for recognisable option (possibly followed
by its parameter), but that is purely to help lazy writers (i.e.,
interactive command users).  When writers know "-i" does not take
any parameter, it may be convenient if the writer who forgot to say
"-m" can just append "-m initial" to what has already be written.

When writing source (be it the production code or test), however, we
write for readers.  What you wrote at a first glance, especially
given that "-i" (or "-o" for that matter) is a relatively less
commonly used option, would confuse less experienced readers by
making them wonder what "-i file.txt" means (e.g., "is that taking
input from the contents of file.txt?").

[PATCH 2/2] t7501: Add test for amending commit to add signoff.

[Ghanshyam Thakkar]

This commit adds test for amending the latest commit to add
Signed-off-by trailer at the end of commit message.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t7501-commit-basic-functionality.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 71dc52ce3a..35c69c3ddd 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -464,6 +464,24 @@ test_expect_success 'amend commit to fix author' '
 
 '
 
+test_expect_success 'amend commit to add signoff' '
+
+	test_when_finished "rm -rf testdir" &&
+	git init testdir &&
+	echo content >testdir/file &&
+	git -C testdir add file &&
+	git -C testdir commit -m "file" &&
+	git -C testdir commit --amend --signoff &&
+	git -C testdir log -1 --pretty=format:%B >actual &&
+	(
+		echo file &&
+		echo &&
+		git -C testdir var GIT_COMMITTER_IDENT >ident &&
+		sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /" ident
+	) >expected &&
+	test_cmp expected actual
+'
+
 test_expect_success 'amend commit to fix date' '
 
 	test_tick &&
-- 
2.43.0

Re: [PATCH 2/2] t7501: Add test for amending commit to add signoff.

[Phillip Wood]

Hi Ghanshyam

On 09/01/2024 06:04, Ghanshyam Thakkar wrote:
> This commit adds test for amending the latest commit to add
> Signed-off-by trailer at the end of commit message.

If we're not already testing this then it seems like a useful addition, 
thanks for working on it. It would also be helpful to check that "git 
commit --amend --signoff" does not add a Signed-off-by: trailer if it 
already exists.

> +test_expect_success 'amend commit to add signoff' '
> +
> +	test_when_finished "rm -rf testdir" &&
> +	git init testdir &&

As Christian said about the other patch in this series I don't think we 
need a new repository here. In our test files we use the same repository 
for the whole file unless there is a compelling reason not to.

> +	echo content >testdir/file &&
> +	git -C testdir add file &&
> +	git -C testdir commit -m "file" &&

I think these three lines can be replaced by

	test_commit --no-tag file file content

> +	git -C testdir commit --amend --signoff &&

> +	git -C testdir log -1 --pretty=format:%B >actual &&
> +	(
> +		echo file &&
> +		echo &&
> +		git -C testdir var GIT_COMMITTER_IDENT >ident &&
> +		sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /" ident
> +	) >expected &&
> +	test_cmp expected actual

This section of the test can be improved by using test_commit_message

	test_commit_message HEAD <<-EOF
	file
	
	Signed-off-by: $GIT_COMMITER_NAME <$GIT_COMMITTER_EMAIL>
	EOF

Best Wishes

Phillip

Re: [PATCH 2/2] t7501: Add test for amending commit to add signoff.

[Ghanshyam Thakkar]

On Tue Jan 9, 2024 at 4:14 PM IST, Phillip Wood wrote:
> Hi Ghanshyam
>
> On 09/01/2024 06:04, Ghanshyam Thakkar wrote:
> > This commit adds test for amending the latest commit to add
> > Signed-off-by trailer at the end of commit message.
>
> If we're not already testing this then it seems like a useful addition, 
> thanks for working on it. It would also be helpful to check that "git 
> commit --amend --signoff" does not add a Signed-off-by: trailer if it 
> already exists.

I hadn't thought of that. I have hastily sent the v2 without seeing this
comment. I will add this test in v3.
>
> > +test_expect_success 'amend commit to add signoff' '
> > +
> > +	test_when_finished "rm -rf testdir" &&
> > +	git init testdir &&
>
> As Christian said about the other patch in this series I don't think we 
> need a new repository here. In our test files we use the same repository 
> for the whole file unless there is a compelling reason not to.

Updated from v2 onwards.
>
> > +	echo content >testdir/file &&
> > +	git -C testdir add file &&
> > +	git -C testdir commit -m "file" &&
>
> I think these three lines can be replaced by
>
> 	test_commit --no-tag file file content

Thank you for the suggestion. I have updated the test to use test_commit.
>
> > +	git -C testdir commit --amend --signoff &&
>
> > +	git -C testdir log -1 --pretty=format:%B >actual &&
> > +	(
> > +		echo file &&
> > +		echo &&
> > +		git -C testdir var GIT_COMMITTER_IDENT >ident &&
> > +		sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /" ident
> > +	) >expected &&
> > +	test_cmp expected actual
>
> This section of the test can be improved by using test_commit_message
>
> 	test_commit_message HEAD <<-EOF
> 	file
> 	
> 	Signed-off-by: $GIT_COMMITER_NAME <$GIT_COMMITTER_EMAIL>
> 	EOF
I have updated the test to use above approach.

Thank you for the review!

Re: [PATCH 2/2] t7501: Add test for amending commit to add signoff.

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> Subject: Re: [PATCH 2/2] t7501: Add test for amending commit to add signoff.

The title is with unusual capitalization and final full-stop (again,
check "git log --no-merges --format=%s -20 v2.43.0" and try to blend
in).

> This commit adds test for amending the latest commit to add
> Signed-off-by trailer at the end of commit message.

"This commit adds ..." -> "Add ..."

Also what the patch does can be read from the patch text below, but
it cannot be read _why_ the patch author thought it was a good idea
to make such a change.  The proposed commit log message is a place
to describe the reason behind the patch.  Why do we want a new test?
Why do we want that new test in this particular file?  etc.

> +test_expect_success 'amend commit to add signoff' '
> +
> +	test_when_finished "rm -rf testdir" &&
> +	git init testdir &&

The same "why a new repository for just this test???" applies here.

> +	echo content >testdir/file &&
> +	git -C testdir add file &&
> +	git -C testdir commit -m "file" &&
> +	git -C testdir commit --amend --signoff &&
> +	git -C testdir log -1 --pretty=format:%B >actual &&

If you are doing many things in a separate directory, the usual
pattern is

	# create a directory DIR (usuall "mkdir", not "git init")
	mkdir DIR &&
	(
		cd DIR &&
		git do this &&
		git do that &&
		inspect the result of this >actual &&
		prepare the expected outcome >expect &&
		test_cmp expect actual
	) &&

Thanks.

Re: [PATCH 0/2][GSOC] t7501: Add tests for various index usages, -i and -o, of commit command and amending commit to add signoff.

[Christian Couder]

On Tue, Jan 9, 2024 at 7:10 AM Ghanshyam Thakkar
<shyamthakkar001@gmail.com> wrote:
>
> This patch series adds tests for various index usages, -i and -o, of commit
> command and amending commit to add Signed-of-by trailer. This is in
> reference to the comment added in commit 12ace0b2 which reads:
>
>   # FIXME: Test the various index usages, -i and -o, test reflog,
>   # signoff, hooks

It seems to me that the patch series should remove or change that
FIXME if it resolves it, or some parts of it.

For example I would expect the patch that adds -i and -o related tests
to remove at least "-i and -o, " from that FIXME comment.

> Ghanshyam Thakkar (2):
>   t7501: Add tests for various index usages, -i and -o, of commit
>     command.
>   t7501: Add test for amending commit to add signoff.

I commented on the first patch, and I took a look at the second one.
It seems to me that a number of comments I made on the first one are
valid for the second one as well. For a micro-project I would suggest
focussing on only one patch though. As we say in
https://git.github.io/General-Microproject-Information/, we want
quality, not quantity! You can always work on other possible bigger
things after your micro-project is merged.

Thanks!

[PATCH v2 0/2] t7501: add tests for --include, --only,

[Ghanshyam Thakkar]

This patch series adds tests for --include, --only of commit and test
for amending the commit with --signoff. And it also addresses the
reviews of Christian Couder and Phillip Wood.

The v2 of this patch removes unnecessary initialization of git repos
and reuses existing files instead of creating new files. Also, it 
removes some redundant code, namely committing everytime for tracking new
files, instead of just staging the files.

The second patch of this series refactors the test with a better
approach as suggested by Phillip Wood.

Ghanshyam Thakkar (2):
  t7501: add tests for --include, --only of commit
  t7501: add test for --amend with --signoff

 t/t7501-commit-basic-functionality.sh | 68 ++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v3 0/2] t7501: add tests for --include, --only and

[Ghanshyam Thakkar]

The v3 of the patch series addresses the comments from Junio and adds
the test to check --amend --signoff does not add signoff if it
already exists as suggested by Phillip Wood.

Also I have removed two tests from v2 which tested the same thing as
other tests in the series, namely:

- removed 'commit --include'. This pattern is tested in '--include with 
staged changes'.
- removed 'commit --only'. This pattern is tested in '--only excludes 
staged changes'.

Ghanshyam Thakkar (2):
  t7501: add tests for --include and --only
  t7501: add tests for --amend --signoff

 t/t7501-commit-basic-functionality.sh | 68 ++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v4 0/2] t7501: add tests for --include, --only, --signoff

[Ghanshyam Thakkar]

The v4 of this series fixes the issue reported by Junio in v3, in
which one of the files was not being added to the index before
committing. This is fixed in v4.

Also 'untracked files' tests are moved after the 'nothing to commit
tests', and also show that with -i and -o, behavior remains same as 
when not using any flag.

The v4 also adds extra step to check that certain files are being
committed. Specifically like,

 'git diff --name-only HEAD^ HEAD'

as per Junio's suggestion.

The '-i and -o do not mix' test is updated according to Junio's
suggestion.

Apart from that, certain instances of 'test_grep' are replaced with
'test_cmp' as suggested by Phillip.

The --signoff tests remain the same apart from removal of empty line
as pointed out by Phillip.

Ghanshyam Thakkar (2):
  t7501: add tests for --include and --only
  t7501: add tests for --amend --signoff

 t/t7501-commit-basic-functionality.sh | 102 +++++++++++++++++++++++++-
 1 file changed, 100 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v4 1/2] t7501: add tests for --include and --only

[Ghanshyam Thakkar]

Add tests for --only (-o) and --include (-i). This include testing
with or without staged changes for both -i and -o. Also to test
for committing untracked files with -i, -o and without -i/-o.

Some tests already exist in t7501 for testing --only, however,
it is tested in combination with --amend and --allow-empty and on
to-be-born branch. The addition of these tests check, when the
pathspec is provided, that only the files matching the pathspec
get committed. This behavior is same when we provide --only
(as --only is the default mode of operation when pathspec is
provided.)

As for --include, there is no prior test for checking if --include
also commits staged changes.

Therefore, these tests belong in t7501 with other similar existing
tests, as described in the case of --only.

And also add test for checking incompatibilty when using -o and -i
together.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t7501-commit-basic-functionality.sh | 79 ++++++++++++++++++++++++++-
 1 file changed, 78 insertions(+), 1 deletion(-)

diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 3d8500a52e..e4633b4af5 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
 #
 
-# FIXME: Test the various index usages, -i and -o, test reflog,
+# FIXME: Test the various index usages, test reflog,
 # signoff
 
 test_description='git commit'
@@ -92,6 +92,19 @@ test_expect_success '--long fails with nothing to commit' '
 	test_must_fail git commit -m initial --long
 '
 
+test_expect_success 'fail to commit untracked file' '
+	echo content >baz &&
+	test_must_fail git commit -m "baz" baz
+'
+
+test_expect_success '--only also fail to commit untracked file' '
+	test_must_fail git commit --only -m "baz" baz
+'
+
+test_expect_success '--include also fail to commit untracked file' '
+	test_must_fail git commit --include -m "baz" baz
+'
+
 test_expect_success 'setup: non-initial commit' '
 	echo bongo bongo bongo >file &&
 	git commit -m next -a
@@ -117,6 +130,70 @@ test_expect_success '--long with stuff to commit returns ok' '
 	git commit -m next -a --long
 '
 
+test_expect_success 'only commit given path (also excluding additional staged changes)' '
+	echo content >file &&
+	echo content >baz &&
+	git add baz &&
+	git commit -m "file" file &&
+
+	git diff --name-only >actual &&
+	test_must_be_empty actual &&
+
+	git diff --name-only --staged >actual &&
+	test_cmp - actual <<-EOF &&
+	baz
+	EOF
+
+	git diff --name-only HEAD^ HEAD >actual &&
+	test_cmp - actual <<-EOF
+	file
+	EOF
+'
+
+test_expect_success 'same as above with -o/--only' '
+	echo change >file &&
+	echo change >baz &&
+	git add baz &&
+	git commit --only -m "file" file &&
+
+	git diff --name-only >actual &&
+	test_must_be_empty actual &&
+
+	git diff --name-only --staged >actual &&
+	test_cmp - actual <<-EOF &&
+	baz
+	EOF
+
+	git diff --name-only HEAD^ HEAD >actual &&
+	test_cmp - actual <<-EOF
+	file
+	EOF
+'
+
+test_expect_success '-i/--include includes staged changes' '
+	echo newcontent >file &&
+	echo newcontent >baz &&
+	git add file &&
+	git commit --include -m "file baz" baz  &&
+
+	git diff --name-only HEAD >remaining &&
+	test_must_be_empty remaining &&
+
+	git diff --name-only HEAD^ HEAD >changes &&
+	test_cmp - changes <<-EOF
+	baz
+	file
+	EOF
+'
+
+test_expect_success '--include and --only do not mix' '
+	test_when_finished "git reset --hard" &&
+	echo new >file &&
+	echo new >baz &&
+	test_must_fail git commit --include --only -m "file baz" file baz 2>actual &&
+	test_grep -e "fatal: options .-i/--include. and .-o/--only. cannot be used together" actual
+'
+
 test_expect_success 'commit message from non-existing file' '
 	echo more bongo: bongo bongo bongo bongo >file &&
 	test_must_fail git commit -F gah -a
-- 
2.43.0

Re: [PATCH v4 1/2] t7501: add tests for --include and --only

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> @@ -92,6 +92,19 @@ test_expect_success '--long fails with nothing to commit' '
>  	test_must_fail git commit -m initial --long
>  '
>  
> +test_expect_success 'fail to commit untracked file' '
> +	echo content >baz &&
> +	test_must_fail git commit -m "baz" baz
> +'
> +
> +test_expect_success '--only also fail to commit untracked file' '
> +	test_must_fail git commit --only -m "baz" baz
> +'
> +
> +test_expect_success '--include also fail to commit untracked file' '
> +	test_must_fail git commit --include -m "baz" baz
> +'

As the latter two depends on the first one's side effect of leaving
an untracked 'baz' file in the working tree, I do not know if it is
sensible to split these into three tests.  An obvious alternative is
to have a single test

	test_expect_success 'pathspec that do not match any tracked path' '
		echo content >baz &&
		test_must_fail git commit -m baz baz &&
		test_must_fail git commit -o -m baz baz &&
		test_must_fail git commit -i -m baz baz
	'

By the way, I do not think presence of 'baz' in the working tree
matters in the failures from these tests all that much, as the
reason they fail is because the pathspec does not match any tracked
file, whose contents in the index to be updated before made into a
commit.

> @@ -117,6 +130,70 @@ test_expect_success '--long with stuff to commit returns ok' '
>  	git commit -m next -a --long
>  '
>  
> +test_expect_success 'only commit given path (also excluding additional staged changes)' '
> +	echo content >file &&
> +	echo content >baz &&
> +	git add baz &&
> +	git commit -m "file" file &&
> +
> +	git diff --name-only >actual &&
> +	test_must_be_empty actual &&
> +
> +	git diff --name-only --staged >actual &&
> +	test_cmp - actual <<-EOF &&
> +	baz
> +	EOF
> +
> +	git diff --name-only HEAD^ HEAD >actual &&
> +	test_cmp - actual <<-EOF
> +	file
> +	EOF
> +'

OK.  The change to baz is already in the index, but "commit file"
will skip over it and record only the changes to file in the commit.
Very much makes sense.

> +test_expect_success 'same as above with -o/--only' '
> +	echo change >file &&
> +	echo change >baz &&
> +	git add baz &&
> +	git commit --only -m "file" file &&
> +
> +	git diff --name-only >actual &&
> +	test_must_be_empty actual &&
> +
> +	git diff --name-only --staged >actual &&
> +	test_cmp - actual <<-EOF &&
> +	baz
> +	EOF
> +
> +	git diff --name-only HEAD^ HEAD >actual &&
> +	test_cmp - actual <<-EOF
> +	file
> +	EOF
> +'

Likewise.  An obvious thing to notice is that this cannot use the
same "contents" text as before, even though it claims to be "same as
above".  If the final contents of "file" and "baz" does not matter,
but it matters more that these files have been changed, it often is
a good idea to append to the file.  That way, you can ensure that
you will be making them different, no matter what the initial
condition was, i.e.,

	for opt in "" "-o" "--only"
	do
		test_expect_success 'skip over already added change' '
			echo more >>file &&
			echo more >>baz &&
			git add baz &&
			git commit $opt -m "file" file &&

			... ensure that changes to file are committed
			... and changes to baz is only in the index
		'
	done

let's you test all three combinations.

Thanks.

Re: [PATCH v4 1/2] t7501: add tests for --include and --only

[Ghanshyam Thakkar]

On Sat Jan 13, 2024 at 4:40 AM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>
> > @@ -92,6 +92,19 @@ test_expect_success '--long fails with nothing to commit' '
> >  	test_must_fail git commit -m initial --long
> >  '
> >  
> > +test_expect_success 'fail to commit untracked file' '
> > +	echo content >baz &&
> > +	test_must_fail git commit -m "baz" baz
> > +'
> > +
> > +test_expect_success '--only also fail to commit untracked file' '
> > +	test_must_fail git commit --only -m "baz" baz
> > +'
> > +
> > +test_expect_success '--include also fail to commit untracked file' '
> > +	test_must_fail git commit --include -m "baz" baz
> > +'
>
> As the latter two depends on the first one's side effect of leaving
> an untracked 'baz' file in the working tree, I do not know if it is
> sensible to split these into three tests.  An obvious alternative is
> to have a single test
>
> 	test_expect_success 'pathspec that do not match any tracked path' '
> 		echo content >baz &&
> 		test_must_fail git commit -m baz baz &&
> 		test_must_fail git commit -o -m baz baz &&
> 		test_must_fail git commit -i -m baz baz
> 	'
>
> By the way, I do not think presence of 'baz' in the working tree
> matters in the failures from these tests all that much, as the
> reason they fail is because the pathspec does not match any tracked
> file, whose contents in the index to be updated before made into a
> commit.

Yes, that is true. However, as per your prior email in which you stated
about --include:

    "Now which behaviour between "error out because there is no path in
    the index that matches pathspec 'baz'" and "add baz to the index and
    commit that addition, together with what is already in the index" we
    would want to take is probably open for discussion.  Such a discussion
    may decide that the current behaviour is fine.  Until then..."

If in such a discussion it is decided that -i should add to index and
commit, then by not having 'baz' in the working tree, the -i test
would still error out regardless of whether its behaviour is to [add
to the index and commit] or [error out]. Therefore, by having 'baz'
we can detect the change between [-i adds to index and commits] or
[errors out].

> Likewise.  An obvious thing to notice is that this cannot use the
> same "contents" text as before, even though it claims to be "same as
> above".  If the final contents of "file" and "baz" does not matter,
> but it matters more that these files have been changed, it often is
> a good idea to append to the file.  That way, you can ensure that
> you will be making them different, no matter what the initial
> condition was, i.e.,
>
> 	for opt in "" "-o" "--only"
> 	do
> 		test_expect_success 'skip over already added change' '
> 			echo more >>file &&
> 			echo more >>baz &&
> 			git add baz &&
> 			git commit $opt -m "file" file &&
>
> 			... ensure that changes to file are committed
> 			... and changes to baz is only in the index
> 		'
> 	done
>
> let's you test all three combinations.

Yeah, that is a more effective approach. I will change it and reroll
quickly.
>
> Thanks.

Thank you for all the help!

Re: [PATCH v4 1/2] t7501: add tests for --include and --only

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> +test_expect_success '-i/--include includes staged changes' '
> +	echo newcontent >file &&
> +	echo newcontent >baz &&
> +	git add file &&
> +	git commit --include -m "file baz" baz  &&

I may have said this already, but the command invocation that does
not result in an error smells like a bug, and I doubt that we want
to etch the current behaviour into stone, which may make it harder
to fix [*].

Another related behaviour that I suspect is a bug is that if you did

    git add -u baz

instead of this "git commit -i baz", I think the command will
silently succeed without doing anything.  They may be the same bug,
because "git commit -i <pathspec>" is an equivalent to "git add -u
<pathspec>" followed by "git commit" after all.  Both should say
"there is no such path to update that matches the pathspec <baz>"
and error out, I suspect.

Thanks.

[Footnote]

 * A reasonable way out to unblock this particular patch may be to
   clarify that this test is only documenting the current behaviour
   without necessarily endorsing it.  Perhaps

	echo more >>file &&
	echo more >>baz &&
	git add file &&

	# Note: "git commit -i baz" is like "git add -u baz"
	# followed by "git commit" but because baz is untracked,
	# only "file" is committed.
	# This test only documents this current behaviour, which we
	# may want to fix, and when it happens, this needs to be
	# adjusted to the new behaviour.
	git commit -i -m "file and baz" baz &&

   or something, probably.

Re: [PATCH v4 1/2] t7501: add tests for --include and --only

[Ghanshyam Thakkar]

On Sat Jan 13, 2024 at 6:46 AM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>
> > +test_expect_success '-i/--include includes staged changes' '
> > +	echo newcontent >file &&
> > +	echo newcontent >baz &&
> > +	git add file &&
> > +	git commit --include -m "file baz" baz  &&
>
> I may have said this already, but the command invocation that does
> not result in an error smells like a bug, and I doubt that we want
> to etch the current behaviour into stone, which may make it harder
> to fix [*].

Yeah, that is why baz is added in the index in the test before the one
you quoted. And as I understand it, when everything is in the index, it
works as intended. Therefore to not get in the way of amending this
behaviour, no untracked files are being (attempted to be) committed in
these tests (except 'fail to commit untracked files' test, but that is
not what you quoted above).

> Another related behaviour that I suspect is a bug is that if you did
>
>     git add -u baz
>
> instead of this "git commit -i baz", I think the command will
> silently succeed without doing anything.  They may be the same bug,
> because "git commit -i <pathspec>" is an equivalent to "git add -u
> <pathspec>" followed by "git commit" after all.  Both should say
> "there is no such path to update that matches the pathspec <baz>"
> and error out, I suspect.

Yeah, just checked, that also succeeds silently.

> Thanks.
>
> [Footnote]
>
>  * A reasonable way out to unblock this particular patch may be to
>    clarify that this test is only documenting the current behaviour
>    without necessarily endorsing it.  Perhaps
>
> 	echo more >>file &&
> 	echo more >>baz &&
> 	git add file &&
>
> 	# Note: "git commit -i baz" is like "git add -u baz"
> 	# followed by "git commit" but because baz is untracked,
> 	# only "file" is committed.
> 	# This test only documents this current behaviour, which we
> 	# may want to fix, and when it happens, this needs to be
> 	# adjusted to the new behaviour.
> 	git commit -i -m "file and baz" baz &&
>
>    or something, probably.

as stated above, baz is tracked from the previous test. In any case,
I will add a note just to document that untracked files also do not
give any error (when there are staged changes) but are not committed.

Thanks.

[PATCH v4 2/2] t7501: add tests for --amend --signoff

[Ghanshyam Thakkar]

Add tests for amending the commit to add Signed-off-by trailer. And
also to check if it does not add another trailer if one already exists.

Currently, there are tests for --signoff separately in t7501, however,
they are not tested with --amend.

Therefore, these tests belong with other similar tests of --amend in
t7501-commit-basic-functionality.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t7501-commit-basic-functionality.sh | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index e4633b4af5..33a9895e72 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,8 +3,7 @@
 # Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
 #
 
-# FIXME: Test the various index usages, test reflog,
-# signoff
+# FIXME: Test the various index usages, test reflog
 
 test_description='git commit'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
@@ -466,6 +465,28 @@ test_expect_success 'amend commit to fix date' '
 
 '
 
+test_expect_success 'amend commit to add signoff' '
+
+	test_commit "msg" file content &&
+	git commit --amend --signoff &&
+	test_commit_message HEAD <<-EOF
+	msg
+
+	Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+'
+
+test_expect_success 'amend does not add signoff if it already exists' '
+
+	test_commit --signoff "tenor" file newcontent &&
+	git commit --amend --signoff &&
+	test_commit_message HEAD <<-EOF
+	tenor
+
+	Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+'
+
 test_expect_success 'commit mentions forced date in output' '
 	git commit --amend --date=2010-01-02T03:04:05 >output &&
 	grep "Date: *Sat Jan 2 03:04:05 2010" output
-- 
2.43.0

[PATCH v5 0/2] t7501: add tests for --include, --only, --signoff

[Ghanshyam Thakkar]

The v5 of this series updates the '--only' test to use the for loop
and >> (append) operator to test all three cases (-o/--only/none) via a
single test.

This version also documents a bug found via the previous iteration of
this series, which does not report error when --include is used with
invalid pathspec.

Signoff tests remain unchanged.

Ghanshyam Thakkar (2):
  t7501: add tests for --include and --only
  t7501: add tests for --amend --signoff

 t/t7501-commit-basic-functionality.sh | 100 +++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v5 1/2] t7501: add tests for --include and --only

[Ghanshyam Thakkar]

Add tests for --only (-o) and --include (-i). This include testing
with or without staged changes for both -i and -o. Also to test
for committing untracked files with -i, -o and without -i/-o.

Some tests already exist in t7501 for testing --only, however,
it is tested in combination with --amend and --allow-empty and on
to-be-born branch. The addition of these tests check, when the
pathspec is provided, that only the files matching the pathspec
get committed. This behavior is same when we provide --only and
checked by the tests.
(as --only is the default mode of operation when pathspec is
provided.)

As for --include, there is no prior test for checking if --include
also commits staged changes, thus add test for that. Along with 
the test also document a potential bug, in which, when provided
with -i and an untracked pathspec, commit does not fail if there
are staged changes. And when there are no staged changes commit
fails. However, no error is returned to stderr in either of the
cases.

Thus, these tests belong in t7501 with other similar existing tests,
as described in the case of --only.

And also add a test for checking incompatibilty when using -o and
-i together.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t7501-commit-basic-functionality.sh | 77 ++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 3d8500a52e..3e18b879b5 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
 #
 
-# FIXME: Test the various index usages, -i and -o, test reflog,
+# FIXME: Test the various index usages, test reflog,
 # signoff
 
 test_description='git commit'
@@ -92,6 +92,32 @@ test_expect_success '--long fails with nothing to commit' '
 	test_must_fail git commit -m initial --long
 '
 
+test_expect_success 'fail to commit untracked pathspec (even with --include/--only)' '
+	echo content >baz &&
+	error="error: pathspec .baz. did not match any file(s) known to git" &&
+	
+	test_must_fail git commit -m "baz" baz 2>err &&
+	test_grep -e "$error" err &&
+
+	test_must_fail git commit --only -m "baz" baz 2>err &&
+	test_grep -e "$error" err &&
+
+	# TODO: as for --include, the below command will fail because nothing is
+	# staged. If something was staged, it would not fail even if the
+	# pathspec was untracked (however, pathspec will remain untracked and
+	# staged changes would be committed.) In either cases, no error is
+	# returned to stderr like in (-o and without -o/-i) cases. In a
+	# similar manner, "git add -u baz" also does not error out.
+	# 
+	# Therefore, the below test is just to document the current behavior
+	# and is not an endorsement to the current behavior, and we may 
+	# want to fix this. And when that happens, this test should be
+	# updated accordingly.
+
+	test_must_fail git commit --include -m "baz" baz 2>err &&
+	test_must_be_empty err
+'
+
 test_expect_success 'setup: non-initial commit' '
 	echo bongo bongo bongo >file &&
 	git commit -m next -a
@@ -117,6 +143,55 @@ test_expect_success '--long with stuff to commit returns ok' '
 	git commit -m next -a --long
 '
 
+for opt in "" "-o" "--only"
+do 
+	test_expect_success 'exclude additional staged changes when given pathspec' '
+		echo content >>file &&
+		echo content >>baz &&
+		git add baz &&
+		git commit $opt -m "file" file &&
+
+		git diff --name-only >actual &&
+		test_must_be_empty actual &&
+
+		git diff --name-only --staged >actual &&
+		test_cmp - actual <<-EOF &&
+		baz
+		EOF
+
+		git diff --name-only HEAD^ HEAD >actual &&
+		test_cmp - actual <<-EOF
+		file
+		EOF
+	'
+done
+
+test_expect_success '-i/--include includes staged changes' '
+	echo content >>file &&
+	echo content >>baz &&
+	git add file &&
+	
+	# baz is in the index, therefore, it will be committed
+	git commit --include -m "file and baz" baz  &&
+
+	git diff --name-only HEAD >remaining &&
+	test_must_be_empty remaining &&
+
+	git diff --name-only HEAD^ HEAD >changes &&
+	test_cmp - changes <<-EOF
+	baz
+	file
+	EOF
+'
+
+test_expect_success '--include and --only do not mix' '
+	test_when_finished "git reset --hard" &&
+	echo content >>file &&
+	echo content >>baz &&
+	test_must_fail git commit --include --only -m "file baz" file baz 2>actual &&
+	test_grep -e "fatal: options .-i/--include. and .-o/--only. cannot be used together" actual
+'
+
 test_expect_success 'commit message from non-existing file' '
 	echo more bongo: bongo bongo bongo bongo >file &&
 	test_must_fail git commit -F gah -a
-- 
2.43.0

Re: [PATCH v5 1/2] t7501: add tests for --include and --only

[Junio C Hamano]

Thanks.

"git am" auto-fixed these whitespace breakage.

.git/rebase-apply/patch:25: trailing whitespace.
	
.git/rebase-apply/patch:38: trailing whitespace.
	# 
.git/rebase-apply/patch:40: trailing whitespace.
	# and is not an endorsement to the current behavior, and we may 
.git/rebase-apply/patch:56: trailing whitespace.
do 
.git/rebase-apply/patch:82: trailing whitespace.
	
warning: 5 lines applied after fixing whitespace errors.
Applying: t7501: add tests for --include and --only

Re: [PATCH v5 1/2] t7501: add tests for --include and --only

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> +test_expect_success 'fail to commit untracked pathspec (even with --include/--only)' '

"untracked pathspec" is a nonsense word.  We do not track pathspec;
you may use pathspec to specify which path(s) to work on.  I think
this is more about untracked files being ignored for the purpose of
"-i" and "-o".

You used to call this "untracked file", which probably makes more
sense.

> +	# TODO: as for --include, the below command will fail because nothing is
> +	# staged. If something was staged, it would not fail even if the
> +	# pathspec was untracked (however, pathspec will remain untracked and
> +	# staged changes would be committed.) In either cases, no error is

Both uses of "pathspec" here are nonsense.  Saying "even though the
pathspec does not match any tracked path" is OK.  "(however, the
untracked path that match the pathspec are not added and only the
changes in the index gets commit)" is also OK.

> +	# returned to stderr like in (-o and without -o/-i) cases. In a
> +	# similar manner, "git add -u baz" also does not error out.
> +	# 
> +	# Therefore, the below test is just to document the current behavior
> +	# and is not an endorsement to the current behavior, and we may 
> +	# want to fix this. And when that happens, this test should be
> +	# updated accordingly.

OK.

> @@ -117,6 +143,55 @@ test_expect_success '--long with stuff to commit returns ok' '
>  	git commit -m next -a --long
>  '
>  
> +for opt in "" "-o" "--only"
> +do 
> +	test_expect_success 'exclude additional staged changes when given pathspec' '
> +		echo content >>file &&
> +		echo content >>baz &&
> +		git add baz &&
> +		git commit $opt -m "file" file &&
> +
> +		git diff --name-only >actual &&
> +		test_must_be_empty actual &&
> +
> +		git diff --name-only --staged >actual &&
> +		test_cmp - actual <<-EOF &&
> +		baz
> +		EOF

It is probably more common to say

		test_write_lines baz >expect &&
		git diff --name-only --cached >actual &&
		test_cmp expect actual

especially when the expected pattern is a file with short lines,
instead of here-text.  It makes it easier to debug tests, too,
because after "sh t7501-*.sh -i" fails, you can go to the trash
directory and the expectation as well as the actual output are
there in files.

> +		git diff --name-only HEAD^ HEAD >actual &&
> +		test_cmp - actual <<-EOF
> +		file
> +		EOF

Ditto.

This tests the most important aspect of "-o"; even when the index
has other changes relative to HEAD already, they are skipped over
and only the changes to paths that match the given pathspec are
committed, and this test demonstrates it.  Well done.

> +test_expect_success '-i/--include includes staged changes' '
> +	echo content >>file &&
> +	echo content >>baz &&
> +	git add file &&
> +	
> +	# baz is in the index, therefore, it will be committed
> +	git commit --include -m "file and baz" baz  &&
> +
> +	git diff --name-only HEAD >remaining &&
> +	test_must_be_empty remaining &&
> +
> +	git diff --name-only HEAD^ HEAD >changes &&
> +	test_cmp - changes <<-EOF
> +	baz
> +	file
> +	EOF

Again with "test_write_lines baz file >expect", this test becomes a
bit shorter.

This tests the most important aspect of "-i"; changes to the paths
that match the given pathspec are added to the index, and recorded
together with changes added to the index already to the commit.
Well done.

> +'
> +
> +test_expect_success '--include and --only do not mix' '
> +	test_when_finished "git reset --hard" &&
> +	echo content >>file &&
> +	echo content >>baz &&
> +	test_must_fail git commit --include --only -m "file baz" file baz 2>actual &&
> +	test_grep -e "fatal: options .-i/--include. and .-o/--only. cannot be used together" actual
> +'
> +
>  test_expect_success 'commit message from non-existing file' '
>  	echo more bongo: bongo bongo bongo bongo >file &&
>  	test_must_fail git commit -F gah -a

Very nicely done.

Thanks.

[PATCH v5 2/2] t7501: add tests for --amend --signoff

[Ghanshyam Thakkar]

Add tests for amending the commit to add Signed-off-by trailer. And
also to check if it does not add another trailer if one already exists.

Currently, there are tests for --signoff separately in t7501, however,
they are not tested with --amend.

Therefore, these tests belong with other similar tests of --amend in
t7501-commit-basic-functionality.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t7501-commit-basic-functionality.sh | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 3e18b879b5..db37e9051b 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,8 +3,7 @@
 # Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
 #
 
-# FIXME: Test the various index usages, test reflog,
-# signoff
+# FIXME: Test the various index usages, test reflog
 
 test_description='git commit'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
@@ -464,6 +463,28 @@ test_expect_success 'amend commit to fix date' '
 
 '
 
+test_expect_success 'amend commit to add signoff' '
+
+	test_commit "msg" file content &&
+	git commit --amend --signoff &&
+	test_commit_message HEAD <<-EOF
+	msg
+
+	Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+'
+
+test_expect_success 'amend does not add signoff if it already exists' '
+
+	test_commit --signoff "tenor" file newcontent &&
+	git commit --amend --signoff &&
+	test_commit_message HEAD <<-EOF
+	tenor
+
+	Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+'
+
 test_expect_success 'commit mentions forced date in output' '
 	git commit --amend --date=2010-01-02T03:04:05 >output &&
 	grep "Date: *Sat Jan 2 03:04:05 2010" output
-- 
2.43.0

[PATCH v6 0/2] t7501: add tests for --include, --only, --signoff

[Ghanshyam Thakkar]

The v6 addresses the comments from Junio, which suggested to improve the
TODO comment describing the potential bug for --include. Also, replace
here-doc with expected output file for better debugging, as suggested by
Junio.

--signoff tests remain unchanged.

Ghanshyam Thakkar (2):
  t7501: add tests for --include and --only
  t7501: add tests for --amend --signoff

 t/t7501-commit-basic-functionality.sh | 98 ++++++++++++++++++++++++++-
 1 file changed, 96 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v6 1/2] t7501: add tests for --include and --only

[Ghanshyam Thakkar]

Add tests for --only (-o) and --include (-i). This include testing
with or without staged changes for both -i and -o. Also to test
for committing untracked files with -i, -o and without -i/-o.

Some tests already exist in t7501 for testing --only, however,
it is only tested in combination with --amend and --allow-empty
and on to-be-born branch. The addition of these tests check, when
the pathspec is provided without using -only, that only the files
matching the pathspec get committed. This behavior is same when
we provide --only and it is checked by the tests.
(as --only is the default mode of operation when pathspec is
provided.)

As for --include, there is no prior test for checking if --include
also commits staged changes, thus add test for that. Along with
the tests also document a potential bug, in which, when provided
with -i and a pathspec that does not match any tracked path,
commit does not fail if there are staged changes. And when there
are no staged changes commit fails. However, no error is returned
to stderr in either of the cases. This is described in the TODO
comment before the relevent testcase.

And also add a test for checking incompatibilty when using -o and
-i together.

Thus, these tests belong in t7501 with other similar existing tests,
as described in the case of --only.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t7501-commit-basic-functionality.sh | 75 ++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 1 deletion(-)

diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 3d8500a52e..c169f10458 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
 #
 
-# FIXME: Test the various index usages, -i and -o, test reflog,
+# FIXME: Test the various index usages, test reflog,
 # signoff
 
 test_description='git commit'
@@ -92,6 +92,34 @@ test_expect_success '--long fails with nothing to commit' '
 	test_must_fail git commit -m initial --long
 '
 
+test_expect_success 'fail to commit untracked file (even with --include/--only)' '
+	echo content >baz &&
+	error="error: pathspec .baz. did not match any file(s) known to git" &&
+
+	test_must_fail git commit -m "baz" baz 2>err &&
+	test_grep -e "$error" err &&
+
+	test_must_fail git commit --only -m "baz" baz 2>err &&
+	test_grep -e "$error" err &&
+
+	# TODO: as for --include, the below command will fail because
+	# nothing is staged. If something was staged, it would not fail
+	# even though the provided pathspec does not match any tracked
+	# path. (However, the untracked paths that match the pathspec are
+	# not committed and only the staged changes get committed.)
+	# In either cases, no error is returned to stderr like in (--only
+	# and without --only/--include) cases. In a similar manner,
+	# "git add -u baz" also does not error out.
+	#
+	# Therefore, the below test is just to document the current behavior
+	# and is not an endorsement to the current behavior, and we may
+	# want to fix this. And when that happens, this test should be
+	# updated accordingly.
+
+	test_must_fail git commit --include -m "baz" baz 2>err &&
+	test_must_be_empty err
+'
+
 test_expect_success 'setup: non-initial commit' '
 	echo bongo bongo bongo >file &&
 	git commit -m next -a
@@ -117,6 +145,51 @@ test_expect_success '--long with stuff to commit returns ok' '
 	git commit -m next -a --long
 '
 
+for opt in "" "-o" "--only"
+do
+	test_expect_success 'exclude additional staged changes when given pathspec' '
+		echo content >>file &&
+		echo content >>baz &&
+		git add baz &&
+		git commit $opt -m "file" file &&
+
+		git diff --name-only >actual &&
+		test_must_be_empty actual &&
+
+		test_write_lines baz >expect &&
+		git diff --name-only --cached >actual &&
+		test_cmp expect actual &&
+
+		test_write_lines file >expect &&
+		git diff --name-only HEAD^ HEAD >actual &&
+		test_cmp expect actual
+	'
+done
+
+test_expect_success '-i/--include includes staged changes' '
+	echo content >>file &&
+	echo content >>baz &&
+	git add file &&
+
+	# baz is in the index, therefore, it will be committed
+	git commit --include -m "file and baz" baz  &&
+
+	git diff --name-only HEAD >remaining &&
+	test_must_be_empty remaining &&
+
+	test_write_lines baz file >expect &&
+	git diff --name-only HEAD^ HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--include and --only do not mix' '
+	test_when_finished "git reset --hard" &&
+	echo content >>file &&
+	echo content >>baz &&
+	test_must_fail git commit --include --only -m "file baz" file baz 2>actual &&
+	test_grep -e "fatal: options .-i/--include. and .-o/--only. cannot be used together" actual
+'
+
 test_expect_success 'commit message from non-existing file' '
 	echo more bongo: bongo bongo bongo bongo >file &&
 	test_must_fail git commit -F gah -a
-- 
2.43.0

[PATCH v6 2/2] t7501: add tests for --amend --signoff

[Ghanshyam Thakkar]

Add tests for amending the commit to add Signed-off-by trailer. And
also to check if it does not add another trailer if one already exists.

Currently, there are tests for --signoff separately in t7501, however,
they are not tested with --amend.

Therefore, these tests belong with other similar tests of --amend in
t7501-commit-basic-functionality.

Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t7501-commit-basic-functionality.sh | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index c169f10458..bced44a0fc 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,8 +3,7 @@
 # Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
 #
 
-# FIXME: Test the various index usages, test reflog,
-# signoff
+# FIXME: Test the various index usages, test reflog
 
 test_description='git commit'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
@@ -462,6 +461,28 @@ test_expect_success 'amend commit to fix date' '
 
 '
 
+test_expect_success 'amend commit to add signoff' '
+
+	test_commit "msg" file content &&
+	git commit --amend --signoff &&
+	test_commit_message HEAD <<-EOF
+	msg
+
+	Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+'
+
+test_expect_success 'amend does not add signoff if it already exists' '
+
+	test_commit --signoff "tenor" file newcontent &&
+	git commit --amend --signoff &&
+	test_commit_message HEAD <<-EOF
+	tenor
+
+	Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+'
+
 test_expect_success 'commit mentions forced date in output' '
 	git commit --amend --date=2010-01-02T03:04:05 >output &&
 	grep "Date: *Sat Jan 2 03:04:05 2010" output
-- 
2.43.0

Re: [PATCH v6 0/2] t7501: add tests for --include, --only, --signoff

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> The v6 addresses the comments from Junio, which suggested to improve the
> TODO comment describing the potential bug for --include. Also, replace
> here-doc with expected output file for better debugging, as suggested by
> Junio.
>
> --signoff tests remain unchanged.
>
> Ghanshyam Thakkar (2):
>   t7501: add tests for --include and --only
>   t7501: add tests for --amend --signoff
>
>  t/t7501-commit-basic-functionality.sh | 98 ++++++++++++++++++++++++++-
>  1 file changed, 96 insertions(+), 2 deletions(-)

Looks quite good.  Let's declare victory and merge it down to 'next'
soonish.

Thanks.

[PATCH v3 1/2] t7501: add tests for --include and --only

[Ghanshyam Thakkar]

Add tests for --include (-i) and --only (-o) of commit. This includes
testing --include with and without staged changes, testing --only with
and without staged changes and testing --only and --include together.

Some tests already exist in t7501 for testing --only, however, it is
only tested in combination with --amend, --allow-empty and to-be-born
branch, and not for testing --only separately.

Similarly there are no separate tests for --include.

These tests are an addition to other similar tests in t7501,
as described above in the case of --only, therefore, they belong
in t7501-commit-basic-functionality.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t7501-commit-basic-functionality.sh | 43 ++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 3d8500a52e..e005175d0b 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
 #
 
-# FIXME: Test the various index usages, -i and -o, test reflog,
+# fixme: test the various index usages, test reflog,
 # signoff
 
 test_description='git commit'
@@ -150,6 +150,47 @@ test_expect_success 'setup: commit message from file' '
 	git commit -F msg -a
 '
 
+test_expect_success '--include fails with untracked files' '
+	echo content >baz &&
+	test_must_fail git commit --include -m "initial" baz
+'
+
+test_expect_success '--include with staged changes' '
+	echo newcontent >baz &&
+	echo newcontent >file &&
+	git add file &&
+	git commit --include -m "file baz" 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 '--only fails with untracked files' '
+	echo content >newfile &&
+	test_must_fail git commit --only -m "newfile" newfile
+'
+
+test_expect_success '--only excludes staged changes' '
+	echo content >file &&
+	echo content >baz &&
+	git add baz &&
+	git commit --only -m "file" file &&
+
+	git diff --name-only >actual &&
+	test_must_be_empty actual &&
+	git diff --name-only --staged >actual &&
+	test_grep "baz" actual
+'
+
+test_expect_success '--include and --only do not mix' '
+	test_when_finished "git reset --hard" &&
+	echo new >file &&
+	echo new >baz &&
+	test_must_fail git commit --include --only -m "bar" bar baz
+'
+
 test_expect_success 'amend commit' '
 	cat >editor <<-\EOF &&
 	#!/bin/sh
-- 
2.43.0

Re: [PATCH v3 1/2] t7501: add tests for --include and --only

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> -# FIXME: Test the various index usages, -i and -o, test reflog,
> +# fixme: test the various index usages, test reflog,
>  # signoff

Why the sudden downcasing?  If this were to turn it to "TODO:"
(110+) or "NEEDSWORK:" (110+) from less often used "FIXME:" (40-),
such a change might be defensible, but there is only one instance
of downcased "fixme:", so I am curious how this happened.

> +test_expect_success '--include fails with untracked files' '
> +	echo content >baz &&
> +	test_must_fail git commit --include -m "initial" baz
> +'

OK, this is because "--allow-empty" is not passed.  This should fail
without -i/-o (which should be the same as passing -o), with -i, or
with -o.

Calling this commit "initial" is highly misleading.  There are bunch
of commits already, but path "baz" has never been used.

> +test_expect_success '--include with staged changes' '
> +	echo newcontent >baz &&
> +	echo newcontent >file &&
> +	git add file &&
> +	git commit --include -m "file baz" baz  &&

I suspect that you found a bug whose behaviour we do not want to
carve into stone with this test.  When this test begins, because the
previous step failed to create the initial commit, we are creating
the root commit, without --allow-empty, with contents in the index
for path "file".  At this point

    $ git commit -m "file baz" baz

(or with "-o", which is the same thing) does error out with

    error: pathspec 'baz' did not match any file(s) known to git

because the "only" thing is to take the changes between HEAD and the
index and limit them further to those paths that match "baz", but
there is no path that matches "baz".

This command

    $ git commit -m "file baz" -i baz

should either error out the same way, or behave more or less[*] like

    $ git add baz && git commit -m "file baz"

and record changes to both "file" and "baz".

    Side note: The "more or less" is we should do "git rm baz"
               instead, if you removed the path.

But it seems that the current code simply ignores the unmatching
pathspec "baz" that is on the command line, hence recording only the
change to the contents of "file".

> +	git diff --name-only >remaining &&
> +	test_must_be_empty remaining &&
> +	git diff --name-only --staged >remaining &&
> +	test_must_be_empty remaining

These two tests to see if the working tree is clean and if the index
is clean are not wrong per-se, but is insufficient.  Judging from
the commit message you used, I think you expected this commit to
contain both changes to 'file' and 'baz'.  That needs to be also
checked with something like "git diff --name-only HEAD^ HEAD".

Now which behaviour between "error out because there is no path in
the index that matches pathspec 'baz'" and "add baz to the index and
commit that addition, together with what is already in the index" we
would want to take is probably open for discussion.  Such a discussion
may decide that the current behaviour is fine.  Until then...

> +test_expect_success '--only fails with untracked files' '
> +	echo content >newfile &&
> +	test_must_fail git commit --only -m "newfile" newfile
> +'

And this should fail the same way without "-o".  Don't we have such
a test that we can just sneak "with -o the same thing happens" test
next to it?

> +test_expect_success '--only excludes staged changes' '
> +	echo content >file &&
> +	echo content >baz &&
> +	git add baz &&
> +	git commit --only -m "file" file &&

This should behave exactly the same way without "-o".

> +	git diff --name-only >actual &&
> +	test_must_be_empty actual &&
> +	git diff --name-only --staged >actual &&
> +	test_grep "baz" actual
> +'
> +
> +test_expect_success '--include and --only do not mix' '
> +	test_when_finished "git reset --hard" &&
> +	echo new >file &&
> +	echo new >baz &&
> +	test_must_fail git commit --include --only -m "bar" bar baz

OK.  If you grep for 'cannot be used together' in t/ directory,
there are many tests that make sure how an invocation like this
should fail, i.e. with an error message that mentions incompatible
options.  Don't we want to do the same?

Thanks.

Re: [PATCH v3 1/2] t7501: add tests for --include and --only

[Ghanshyam Thakkar]

On Thu Jan 11, 2024 at 12:07 AM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>
> > -# FIXME: Test the various index usages, -i and -o, test reflog,
> > +# fixme: test the various index usages, test reflog,
> >  # signoff
>
> Why the sudden downcasing?  If this were to turn it to "TODO:"
> (110+) or "NEEDSWORK:" (110+) from less often used "FIXME:" (40-),
> such a change might be defensible, but there is only one instance
> of downcased "fixme:", so I am curious how this happened.

Wow, I must have done it mistakenly. I guess everything on that line
became lowercase. But, I will fix it.

> > +test_expect_success '--include fails with untracked files' '
> > +	echo content >baz &&
> > +	test_must_fail git commit --include -m "initial" baz
> > +'
>
> OK, this is because "--allow-empty" is not passed.  This should fail
> without -i/-o (which should be the same as passing -o), with -i, or
> with -o.
>
> Calling this commit "initial" is highly misleading.  There are bunch
> of commits already, but path "baz" has never been used.

I will fix this.

> > +test_expect_success '--include with staged changes' '
> > +	echo newcontent >baz &&
> > +	echo newcontent >file &&
> > +	git add file &&
> > +	git commit --include -m "file baz" baz  &&
>
> I suspect that you found a bug whose behaviour we do not want to
> carve into stone with this test.  When this test begins, because the
> previous step failed to create the initial commit, we are creating
> the root commit, without --allow-empty, with contents in the index
> for path "file".  At this point
>
>     $ git commit -m "file baz" baz
>
> (or with "-o", which is the same thing) does error out with
>
>     error: pathspec 'baz' did not match any file(s) known to git
>
> because the "only" thing is to take the changes between HEAD and the
> index and limit them further to those paths that match "baz", but
> there is no path that matches "baz".


> This command
>
>     $ git commit -m "file baz" -i baz
>
> should either error out the same way, or behave more or less[*] like
>
>     $ git add baz && git commit -m "file baz"
>
> and record changes to both "file" and "baz".
>
>     Side note: The "more or less" is we should do "git rm baz"
>                instead, if you removed the path.
>
> But it seems that the current code simply ignores the unmatching
> pathspec "baz" that is on the command line, hence recording only the
> change to the contents of "file".
>
yeah, it seems like if there is anything staged, even if we provide
--include with untracked files, it will commit the staged files
(excluding the untracked files) and will not error out like --only.

Also in v1 there was another test before this one which added the
baz file to the index. That test was removed due to duplication and
while cleaning up the v1 for v2, due to --include not giving an error,
I did not notice that 'baz' was being left out. Will fix the tests.
Apologies for such mistakes.

> > +	git diff --name-only >remaining &&
> > +	test_must_be_empty remaining &&
> > +	git diff --name-only --staged >remaining &&
> > +	test_must_be_empty remaining
>
> These two tests to see if the working tree is clean and if the index
> is clean are not wrong per-se, but is insufficient.  Judging from
> the commit message you used, I think you expected this commit to
> contain both changes to 'file' and 'baz'.  That needs to be also
> checked with something like "git diff --name-only HEAD^ HEAD".

Yeah, the "git diff --name-only HEAD^ HEAD" is more definitive.
I will add that in v4.

> Now which behaviour between "error out because there is no path in
> the index that matches pathspec 'baz'" and "add baz to the index and
> commit that addition, together with what is already in the index" we
> would want to take is probably open for discussion.  Such a discussion
> may decide that the current behaviour is fine.  Until then...
>
> > +test_expect_success '--only fails with untracked files' '
> > +	echo content >newfile &&
> > +	test_must_fail git commit --only -m "newfile" newfile
> > +'
>
> And this should fail the same way without "-o".  Don't we have such
> a test that we can just sneak "with -o the same thing happens" test
> next to it?

Well, I could not find any test which specifically for untracked
files. I will keep looking for it and if not found, perhaps, I can add
both "-i and -o with untracked files" tests after "nothing to commit"
tests in t7501 which are similar in nature.

> > +	git commit --only -m "file" file &&
>
> This should behave exactly the same way without "-o".

That is true, however, I could not find any tests in the t75-- series
that test without -o and which provide pathspec in the commit command
also. So, should I drop -o option in this test? or add a test without
-o?

>
> > +	git diff --name-only >actual &&
> > +	test_must_be_empty actual &&
> > +	git diff --name-only --staged >actual &&
> > +	test_grep "baz" actual
> > +'
> > +
> > +test_expect_success '--include and --only do not mix' '
> > +	test_when_finished "git reset --hard" &&
> > +	echo new >file &&
> > +	echo new >baz &&
> > +	test_must_fail git commit --include --only -m "bar" bar baz
>
> OK.  If you grep for 'cannot be used together' in t/ directory,
> there are many tests that make sure how an invocation like this
> should fail, i.e. with an error message that mentions incompatible
> options.  Don't we want to do the same?

I did it like this because in t7501, there are couple of existing 
"do not mix" tests similar to this one, which only check if the command
fails. However, the approach you mentioned is obviously better, so, I
will update it in v4.

> Thanks.

Thank you for the review!

Re: [PATCH v3 1/2] t7501: add tests for --include and --only

[phillip.wood123]

On 10/01/2024 16:35, Ghanshyam Thakkar wrote:

I don't have much to add to what Junio said, I've just left one comment 
below

> +test_expect_success '--only excludes staged changes' '
> +	echo content >file &&
> +	echo content >baz &&
> +	git add baz &&
> +	git commit --only -m "file" file &&
> +
> +	git diff --name-only >actual &&
> +	test_must_be_empty actual &&
> +	git diff --name-only --staged >actual &&
> +	test_grep "baz" actual

using test_grep feels a bit weak here, I think it would be better to use 
test_cmp to ensure that there are no other staged changes.

Best Wishes

Phillip

[PATCH v3 2/2] t7501: add tests for --amend --signoff

[Ghanshyam Thakkar]

Add tests for amending the commit to add Signed-off-by trailer. And
also to check if it does not add another trailer if one already exists.

Currently, there are tests for --signoff separately in t7501, however,
they are not tested with --amend.

Therefore, these tests belong with other similar tests of --amend in
t7501-commit-basic-functionality.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t7501-commit-basic-functionality.sh | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index e005175d0b..546d60d7fc 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,8 +3,7 @@
 # Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
 #
 
-# fixme: test the various index usages, test reflog,
-# signoff
+# fixme: test the various index usages, test reflog
 
 test_description='git commit'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
@@ -430,6 +429,30 @@ test_expect_success 'amend commit to fix date' '
 
 '
 
+test_expect_success 'amend commit to add signoff' '
+
+	test_commit "msg" file content &&
+	git commit --amend --signoff &&
+	test_commit_message HEAD <<-EOF
+	msg
+
+	Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+
+'
+
+test_expect_success 'amend does not add signoff if it already exists' '
+
+	test_commit --signoff "tenor" file newcontent &&
+	git commit --amend --signoff &&
+	test_commit_message HEAD <<-EOF
+	tenor
+
+	Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+
+'
+
 test_expect_success 'commit mentions forced date in output' '
 	git commit --amend --date=2010-01-02T03:04:05 >output &&
 	grep "Date: *Sat Jan 2 03:04:05 2010" output
-- 
2.43.0

Re: [PATCH v3 2/2] t7501: add tests for --amend --signoff

[phillip.wood123]

On 10/01/2024 16:35, Ghanshyam Thakkar wrote:
> Add tests for amending the commit to add Signed-off-by trailer. And
> also to check if it does not add another trailer if one already exists.
> 
> Currently, there are tests for --signoff separately in t7501, however,
> they are not tested with --amend.
> 
> Therefore, these tests belong with other similar tests of --amend in
> t7501-commit-basic-functionality.
> 
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>

This version looks good, thanks for re-rolling. I have left one comment 
below but it is not worth re-rolling just for that.

> +test_expect_success 'amend commit to add signoff' '
> +
> +	test_commit "msg" file content &&
> +	git commit --amend --signoff &&
> +	test_commit_message HEAD <<-EOF
> +	msg
> +
> +	Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
> +	EOF
> +
> +'

If you do happen re-roll I think we could happily lose the empty line 
before the closing "'" in this test and the next.

Best Wishes

Phillip

[PATCH v2 1/2] t7501: add tests for --include, --only of commit

[Ghanshyam Thakkar]

This commit adds tests for testing --include (-o) and --only (-o). It
include testing --include with and without staged changes, testing
--only with and without staged changes and testing --only and --include
together.

Some tests already exist in t7501 for testing --only, however, they
are only tested in combination with --amend --allow-empty and to-be-born
branch, and not for testing --only separately.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
A single test also exists in t1092-sparse-checkout-compatibility.sh
named "commit including unstaged changes" for --include, however, that
compares the results of sparse-checkout with full-checkout when using
--include and is not necessarily for testing --include itself. Otherthan
that, I could not find any other test for --include.

 t/t7501-commit-basic-functionality.sh | 55 ++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 3d8500a52e..9325db1f66 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
 #
 
-# FIXME: Test the various index usages, -i and -o, test reflog,
+# FIXME: Test the various index usages, test reflog,
 # signoff
 
 test_description='git commit'
@@ -150,6 +150,59 @@ test_expect_success 'setup: commit message from file' '
 	git commit -F msg -a
 '
 
+test_expect_success '--include fails with untracked files' '
+	echo content >baz &&
+	test_must_fail git commit --include baz -m initial
+'
+
+test_expect_success 'commit --include' '
+	test_when_finished "rm -rf remaining" &&
+	echo content >file &&
+	git commit --include file -m "file" &&
+	git diff --name-only >remaining &&
+	test_must_be_empty remaining
+'
+
+test_expect_success '--include with staged changes' '
+	echo newcontent >baz &&
+	echo newcontent >file &&
+	git add file &&
+	git commit --include baz -m "file 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 --only' '
+	echo change >file &&
+	git commit --only file -m "file" &&
+	git diff --name-only >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success '--only fails with untracked files' '
+	echo content >newfile &&
+	test_must_fail git commit --only newfile -m "newfile"
+'
+
+test_expect_success '--only excludes staged changes' '
+	echo content >file &&
+	echo content >baz &&
+	git add baz &&
+	git commit --only file -m "file" &&
+	git diff --name-only --staged >actual &&
+	test_grep "baz" actual
+'
+
+test_expect_success '--include and --only together fails' '
+	test_when_finished "git reset --hard" &&
+	echo new >file &&
+	echo new >baz &&
+	test_must_fail git commit --include baz --only bar -m "bar"
+'
+
 test_expect_success 'amend commit' '
 	cat >editor <<-\EOF &&
 	#!/bin/sh
-- 
2.43.0

Re: [PATCH v2 1/2] t7501: add tests for --include, --only of commit

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> This commit adds tests for testing --include (-o) and --only (-o). It

"-i" not "-o" stands for "--include".

Please write in imperative mood.

> +test_expect_success '--include fails with untracked files' '
> +	echo content >baz &&
> +	test_must_fail git commit --include baz -m initial
> +'

My comment on argument order applies to this iteration, too.  Please
write your code to help readers.

> +test_expect_success 'commit --include' '
> +	test_when_finished "rm -rf remaining" &&

Why recursive removal when you _know_ what you create is a plan
file?

[PATCH v2 2/2] t7501: add test for --amend with --signoff

[Ghanshyam Thakkar]

This commit adds test for amending commit to add Signed-off-by
trailer.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
I believe there are many existing tests which already cover almost
all of the scenarios. So in addition to this test, I have also
updated the FIXME comment to remove signoff.

 t/t7501-commit-basic-functionality.sh | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 9325db1f66..376a7d59cc 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,8 +3,7 @@
 # Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
 #
 
-# FIXME: Test the various index usages, test reflog,
-# signoff
+# FIXME: Test the various index usages, test reflog
 
 test_description='git commit'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
@@ -442,6 +441,18 @@ test_expect_success 'amend commit to fix date' '
 
 '
 
+test_expect_success 'amend commit to add signoff' '
+
+	test_commit file file content &&
+	git commit --amend --signoff &&
+	test_commit_message HEAD <<-EOF
+	file
+
+	Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+
+'
+
 test_expect_success 'commit mentions forced date in output' '
 	git commit --amend --date=2010-01-02T03:04:05 >output &&
 	grep "Date: *Sat Jan 2 03:04:05 2010" output
-- 
2.43.0