{"thread":{"id":"60703","subject":"[PATCH 0/2][GSOC] t7501: Add tests for various index usages, -i and -o, of commit command and amending commit to add signoff.","startedAt":"2024-01-09T06:09:49Z","lastAt":"2024-01-17T21:33:48Z","messageCount":37,"participants":["Ghanshyam Thakkar","Christian Couder","Phillip Wood","Junio C Hamano","phillip.wood123@gmail.com"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"486418","messageId":"20240109060417.1144647-2-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":null,"subject":"[PATCH 0/2][GSOC] t7501: Add tests for various index usages, -i and -o, of commit command and amending commit to add signoff.","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-09T06:04:12Z","receivedAt":"2024-01-09T06:09:49Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"This patch series adds tests for various index usages, -i and -o, of commit\ncommand and amending commit to add Signed-of-by trailer. This is in\nreference to the comment added in commit 12ace0b2 which reads:\n\n # FIXME: Test the various index usages, -i and -o, test reflog,\n # signoff, hooks\n\nGhanshyam Thakkar (2):\n t7501: Add tests for various index usages, -i and -o, of commit\n command.\n t7501: Add test for amending commit to add signoff.\n\n t/t7501-commit-basic-functionality.sh | 108 ++++++++++++++++++++++++++\n 1 file changed, 108 insertions(+)\n\n-- \n2.43.0\n\n"},{"id":"486419","messageId":"20240109060417.1144647-3-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240109060417.1144647-2-shyamthakkar001@gmail.com","subject":"[PATCH 1/2] t7501: Add tests for various index usages, -i and -o, of commit command.","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-09T06:04:13Z","receivedAt":"2024-01-09T06:10:00Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"This commit adds tests for -i and -o flags of the commit command. It\nincludes testing -i with and without staged changes, testing -o with and\nwithout staged changes, and testing -i and -o together.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n t/t7501-commit-basic-functionality.sh | 90 +++++++++++++++++++++++++++\n 1 file changed, 90 insertions(+)\n\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex 3d8500a52e..71dc52ce3a 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -76,6 +76,96 @@ test_expect_success 'nothing to commit' '\n \ttest_must_fail git commit -m initial\n '\n \n+test_expect_success 'commit with -i fails with untracked files' '\n+\ttest_when_finished \"rm -rf testdir\" &&\n+\tgit init testdir &&\n+\techo content >testdir/file.txt &&\n+\ttest_must_fail git -C testdir commit -i file.txt -m initial\n+'\n+\n+test_expect_success 'commit with -i' '\n+\techo content >bar &&\n+\tgit add bar &&\n+\tgit commit -m \"bar\" &&\n+\n+\techo content2 >bar &&\n+\tgit commit -i bar -m \"bar second\"\n+'\n+\n+test_expect_success 'commit with -i multiple files' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo content >bar &&\n+\techo content >baz &&\n+\techo content >saz &&\n+\tgit add bar baz saz &&\n+\tgit commit -m \"bar baz saz\" &&\n+\n+\techo content2 >bar &&\n+\techo content2 >baz &&\n+\techo content2 >saz &&\n+\tgit commit -i bar saz -m \"bar\" &&\n+\tgit diff --name-only >remaining &&\n+\ttest_grep \"baz\" remaining\n+'\n+\n+test_expect_success 'commit with -i includes staged changes' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo content >bar &&\n+\tgit add bar &&\n+\tgit commit -m \"bar\" &&\n+\n+\techo content2 >bar &&\n+\techo content2 >baz &&\n+\tgit add baz &&\n+\tgit commit -i bar -m \"bar baz\" &&\n+\tgit diff --name-only >remaining &&\n+\ttest_must_be_empty remaining &&\n+\tgit diff --name-only --staged >remaining &&\n+\ttest_must_be_empty remaining\n+'\n+\n+test_expect_success 'commit with -o' '\n+\techo content >bar &&\n+\tgit add bar &&\n+\tgit commit -m \"bar\" &&\n+\techo content2 >bar &&\n+\tgit commit -o bar -m \"bar again\"\n+'\n+\n+test_expect_success 'commit with -o fails with untracked files' '\n+\ttest_when_finished \"rm -rf testdir\" &&\n+\tgit init testdir &&\n+\techo content >testdir/bar &&\n+\ttest_must_fail git -C testdir commit -o bar -m \"bar\"\n+'\n+\n+test_expect_success 'commit with -o exludes staged changes' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo content >bar &&\n+\techo content >baz &&\n+\tgit add . &&\n+\tgit commit -m \"bar baz\" &&\n+\n+\techo content2 >bar &&\n+\techo content2 >baz &&\n+\tgit add baz &&\n+\tgit commit -o bar -m \"bar\" &&\n+\tgit diff --name-only --staged >actual &&\n+\ttest_grep \"baz\" actual\n+'\n+\n+test_expect_success 'commit with both -i and -o fails' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo content >bar &&\n+\techo content >baz &&\n+\tgit add . &&\n+\tgit commit -m \"bar baz\" &&\n+\n+\techo content2 >bar &&\n+\techo content2 >baz &&\n+\ttest_must_fail git commit -i baz -o bar -m \"bar\"\n+'\n+\n test_expect_success '--dry-run fails with nothing to commit' '\n \ttest_must_fail git commit -m initial --dry-run\n '\n-- \n2.43.0\n\n"},{"id":"486420","messageId":"20240109060417.1144647-4-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240109060417.1144647-2-shyamthakkar001@gmail.com","subject":"[PATCH 2/2] t7501: Add test for amending commit to add signoff.","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-09T06:04:14Z","receivedAt":"2024-01-09T06:10:03Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"This commit adds test for amending the latest commit to add\nSigned-off-by trailer at the end of commit message.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n t/t7501-commit-basic-functionality.sh | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex 71dc52ce3a..35c69c3ddd 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -464,6 +464,24 @@ test_expect_success 'amend commit to fix author' '\n \n '\n \n+test_expect_success 'amend commit to add signoff' '\n+\n+\ttest_when_finished \"rm -rf testdir\" &&\n+\tgit init testdir &&\n+\techo content >testdir/file &&\n+\tgit -C testdir add file &&\n+\tgit -C testdir commit -m \"file\" &&\n+\tgit -C testdir commit --amend --signoff &&\n+\tgit -C testdir log -1 --pretty=format:%B >actual &&\n+\t(\n+\t\techo file &&\n+\t\techo &&\n+\t\tgit -C testdir var GIT_COMMITTER_IDENT >ident &&\n+\t\tsed -e \"s/>.*/>/\" -e \"s/^/Signed-off-by: /\" ident\n+\t) >expected &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'amend commit to fix date' '\n \n \ttest_tick &&\n-- \n2.43.0\n\n"},{"id":"486421","messageId":"CAP8UFD0GJf5+eOTxy6s+zCzpDmCU_FY4BjwtjTE7RvZ5mKettA@mail.gmail.com","threadId":"60703","inReplyTo":"20240109060417.1144647-3-shyamthakkar001@gmail.com","subject":"Re: [PATCH 1/2] t7501: Add tests for various index usages, -i and -o, of commit command.","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-09T09:20:55Z","receivedAt":"2024-01-09T09:21:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"First about the commit subject:\n\n\"t7501: Add tests for various index usages, -i and -o, of commit command.\"\n\nit should be shorter, shouldn't end with a \".\" and \"Add\" should be \"add\".\n\nOn Tue, Jan 9, 2024 at 7:10 AM Ghanshyam Thakkar\n wrote:\n>\n> This commit adds tests for -i and -o flags of the commit command. It\n> includes testing -i with and without staged changes, testing -o with and\n> without staged changes, and testing -i and -o together.\n\nA few suggestions to make things a bit more clear:\n\n - tell that -i is a synonymous for --include and -o for --only\n - tell if there are already tests for these options\n - tell why the tests you add are worth it if tests for an option already exist\n\n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> t/t7501-commit-basic-functionality.sh | 90 +++++++++++++++++++++++++++\n> 1 file changed, 90 insertions(+)\n>\n> diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\n> index 3d8500a52e..71dc52ce3a 100755\n> --- a/t/t7501-commit-basic-functionality.sh\n> +++ b/t/t7501-commit-basic-functionality.sh\n> @@ -76,6 +76,96 @@ test_expect_success 'nothing to commit' '\n> test_must_fail git commit -m initial\n> '\n>\n> +test_expect_success 'commit with -i fails with untracked files' '\n> + test_when_finished \"rm -rf testdir\" &&\n> + git init testdir &&\n> + echo content >testdir/file.txt &&\n> + test_must_fail git -C testdir commit -i file.txt -m initial\n> +'\n\nExisting tests didn't need a repo, so I am not sure it's worth\ncreating a testdir repo just for this test.\n\nAlso I am not sure this is the best place in the test script to add -i\nand -o related tests. Here we are between a 'nothing to commit' test\nand a '--dry-run fails with nothing to commit' and then more 'nothing\nto commit' related tests. I think it would be better if all those\n'nothing to commit' related tests could stay together.\n\n> +test_expect_success 'commit with -i' '\n> + echo content >bar &&\n> + git add bar &&\n> + git commit -m \"bar\" &&\n\nWhy is the above needed for this test?\n\n> + echo content2 >bar &&\n> + git commit -i bar -m \"bar second\"\n> +'\n> +\n> +test_expect_success 'commit with -i multiple files' '\n> + test_when_finished \"git reset --hard\" &&\n> + echo content >bar &&\n> + echo content >baz &&\n> + echo content >saz &&\n> + git add bar baz saz &&\n> + git commit -m \"bar baz saz\" &&\n\nNot sure why the above is needed here too.\n\n> + echo content2 >bar &&\n> + echo content2 >baz &&\n> + echo content2 >saz &&\n> + git commit -i bar saz -m \"bar\" &&\n> + git diff --name-only >remaining &&\n> + test_grep \"baz\" remaining\n> +'\n> +\n> +test_expect_success 'commit with -i includes staged changes' '\n> + test_when_finished \"git reset --hard\" &&\n> + echo content >bar &&\n> + git add bar &&\n> + git commit -m \"bar\" &&\n> +\n> + echo content2 >bar &&\n> + echo content2 >baz &&\n> + git add baz &&\n> + git commit -i bar -m \"bar baz\" &&\n> + git diff --name-only >remaining &&\n> + test_must_be_empty remaining &&\n> + git diff --name-only --staged >remaining &&\n> + test_must_be_empty remaining\n> +'\n> +\n> +test_expect_success 'commit with -o' '\n> + echo content >bar &&\n> + git add bar &&\n> + git commit -m \"bar\" &&\n> + echo content2 >bar &&\n> + git commit -o bar -m \"bar again\"\n> +'\n> +\n> +test_expect_success 'commit with -o fails with untracked files' '\n> + test_when_finished \"rm -rf testdir\" &&\n> + git init testdir &&\n> + echo content >testdir/bar &&\n> + test_must_fail git -C testdir commit -o bar -m \"bar\"\n> +'\n> +\n> +test_expect_success 'commit with -o exludes staged changes' '\n\ns/exludes/excludes/\n\n> + test_when_finished \"git reset --hard\" &&\n> + echo content >bar &&\n> + echo content >baz &&\n> + git add . &&\n> + git commit -m \"bar baz\" &&\n> +\n> + echo content2 >bar &&\n> + echo content2 >baz &&\n> + git add baz &&\n> + git commit -o bar -m \"bar\" &&\n> + git diff --name-only --staged >actual &&\n> + test_grep \"baz\" actual\n> +'\n\nThanks.\n"},{"id":"486422","messageId":"CAP8UFD2W3JCXQCRV9ekzMbcPHW+LmWBxm_WOKkEZDD=Tnjj_mg@mail.gmail.com","threadId":"60703","inReplyTo":"20240109060417.1144647-2-shyamthakkar001@gmail.com","subject":"Re: [PATCH 0/2][GSOC] t7501: Add tests for various index usages, -i and -o, of commit command and amending commit to add signoff.","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-09T09:32:06Z","receivedAt":"2024-01-09T09:32:20Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Jan 9, 2024 at 7:10 AM Ghanshyam Thakkar\n wrote:\n>\n> This patch series adds tests for various index usages, -i and -o, of commit\n> command and amending commit to add Signed-of-by trailer. This is in\n> reference to the comment added in commit 12ace0b2 which reads:\n>\n> # FIXME: Test the various index usages, -i and -o, test reflog,\n> # signoff, hooks\n\nIt seems to me that the patch series should remove or change that\nFIXME if it resolves it, or some parts of it.\n\nFor example I would expect the patch that adds -i and -o related tests\nto remove at least \"-i and -o, \" from that FIXME comment.\n\n> Ghanshyam Thakkar (2):\n> t7501: Add tests for various index usages, -i and -o, of commit\n> command.\n> t7501: Add test for amending commit to add signoff.\n\nI commented on the first patch, and I took a look at the second one.\nIt seems to me that a number of comments I made on the first one are\nvalid for the second one as well. For a micro-project I would suggest\nfocussing on only one patch though. As we say in\nhttps://git.github.io/General-Microproject-Information/, we want\nquality, not quantity! You can always work on other possible bigger\nthings after your micro-project is merged.\n\nThanks!\n"},{"id":"486425","messageId":"2113e178-149b-49aa-9d78-ff1c480f754c@gmail.com","threadId":"60703","inReplyTo":"20240109060417.1144647-4-shyamthakkar001@gmail.com","subject":"Re: [PATCH 2/2] t7501: Add test for amending commit to add signoff.","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-01-09T10:44:23Z","receivedAt":"2024-01-09T10:44:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ghanshyam\n\nOn 09/01/2024 06:04, Ghanshyam Thakkar wrote:\n> This commit adds test for amending the latest commit to add\n> Signed-off-by trailer at the end of commit message.\n\nIf we're not already testing this then it seems like a useful addition, \nthanks for working on it. It would also be helpful to check that \"git \ncommit --amend --signoff\" does not add a Signed-off-by: trailer if it \nalready exists.\n\n> +test_expect_success 'amend commit to add signoff' '\n> +\n> +\ttest_when_finished \"rm -rf testdir\" &&\n> +\tgit init testdir &&\n\nAs Christian said about the other patch in this series I don't think we \nneed a new repository here. In our test files we use the same repository \nfor the whole file unless there is a compelling reason not to.\n\n> +\techo content >testdir/file &&\n> +\tgit -C testdir add file &&\n> +\tgit -C testdir commit -m \"file\" &&\n\nI think these three lines can be replaced by\n\n\ttest_commit --no-tag file file content\n\n> +\tgit -C testdir commit --amend --signoff &&\n\n> +\tgit -C testdir log -1 --pretty=format:%B >actual &&\n> +\t(\n> +\t\techo file &&\n> +\t\techo &&\n> +\t\tgit -C testdir var GIT_COMMITTER_IDENT >ident &&\n> +\t\tsed -e \"s/>.*/>/\" -e \"s/^/Signed-off-by: /\" ident\n> +\t) >expected &&\n> +\ttest_cmp expected actual\n\nThis section of the test can be improved by using test_commit_message\n\n\ttest_commit_message HEAD <<-EOF\n\tfile\n\t\n\tSigned-off-by: $GIT_COMMITER_NAME <$GIT_COMMITTER_EMAIL>\n\tEOF\n\nBest Wishes\n\nPhillip\n"},{"id":"486441","messageId":"20240109165304.8027-2-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240109060417.1144647-2-shyamthakkar001@gmail.com","subject":"[PATCH v2 0/2] t7501: add tests for --include, --only,","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-09T16:51:19Z","receivedAt":"2024-01-09T16:55:22Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"This patch series adds tests for --include, --only of commit and test\nfor amending the commit with --signoff. And it also addresses the\nreviews of Christian Couder and Phillip Wood.\n\nThe v2 of this patch removes unnecessary initialization of git repos\nand reuses existing files instead of creating new files. Also, it \nremoves some redundant code, namely committing everytime for tracking new\nfiles, instead of just staging the files.\n\nThe second patch of this series refactors the test with a better\napproach as suggested by Phillip Wood.\n\nGhanshyam Thakkar (2):\n t7501: add tests for --include, --only of commit\n t7501: add test for --amend with --signoff\n\n t/t7501-commit-basic-functionality.sh | 68 ++++++++++++++++++++++++++-\n 1 file changed, 66 insertions(+), 2 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"486442","messageId":"20240109165304.8027-4-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240109060417.1144647-2-shyamthakkar001@gmail.com","subject":"[PATCH v2 1/2] t7501: add tests for --include, --only of commit","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-09T16:51:21Z","receivedAt":"2024-01-09T17:01:06Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"This commit adds tests for testing --include (-o) and --only (-o). It\ninclude testing --include with and without staged changes, testing\n--only with and without staged changes and testing --only and --include\ntogether.\n\nSome tests already exist in t7501 for testing --only, however, they\nare only tested in combination with --amend --allow-empty and to-be-born\nbranch, and not for testing --only separately.\n\nSigned-off-by: Ghanshyam Thakkar \n---\nA single test also exists in t1092-sparse-checkout-compatibility.sh\nnamed \"commit including unstaged changes\" for --include, however, that\ncompares the results of sparse-checkout with full-checkout when using\n--include and is not necessarily for testing --include itself. Otherthan\nthat, I could not find any other test for --include.\n\n t/t7501-commit-basic-functionality.sh | 55 ++++++++++++++++++++++++++-\n 1 file changed, 54 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex 3d8500a52e..9325db1f66 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -3,7 +3,7 @@\n # Copyright (c) 2007 Kristian Høgsberg \n #\n \n-# FIXME: Test the various index usages, -i and -o, test reflog,\n+# FIXME: Test the various index usages, test reflog,\n # signoff\n \n test_description='git commit'\n@@ -150,6 +150,59 @@ test_expect_success 'setup: commit message from file' '\n \tgit commit -F msg -a\n '\n \n+test_expect_success '--include fails with untracked files' '\n+\techo content >baz &&\n+\ttest_must_fail git commit --include baz -m initial\n+'\n+\n+test_expect_success 'commit --include' '\n+\ttest_when_finished \"rm -rf remaining\" &&\n+\techo content >file &&\n+\tgit commit --include file -m \"file\" &&\n+\tgit diff --name-only >remaining &&\n+\ttest_must_be_empty remaining\n+'\n+\n+test_expect_success '--include with staged changes' '\n+\techo newcontent >baz &&\n+\techo newcontent >file &&\n+\tgit add file &&\n+\tgit commit --include baz -m \"file baz\" &&\n+\n+\tgit diff --name-only >remaining &&\n+\ttest_must_be_empty remaining &&\n+\tgit diff --name-only --staged >remaining &&\n+\ttest_must_be_empty remaining\n+'\n+\n+test_expect_success 'commit --only' '\n+\techo change >file &&\n+\tgit commit --only file -m \"file\" &&\n+\tgit diff --name-only >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success '--only fails with untracked files' '\n+\techo content >newfile &&\n+\ttest_must_fail git commit --only newfile -m \"newfile\"\n+'\n+\n+test_expect_success '--only excludes staged changes' '\n+\techo content >file &&\n+\techo content >baz &&\n+\tgit add baz &&\n+\tgit commit --only file -m \"file\" &&\n+\tgit diff --name-only --staged >actual &&\n+\ttest_grep \"baz\" actual\n+'\n+\n+test_expect_success '--include and --only together fails' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo new >file &&\n+\techo new >baz &&\n+\ttest_must_fail git commit --include baz --only bar -m \"bar\"\n+'\n+\n test_expect_success 'amend commit' '\n \tcat >editor <<-\\EOF &&\n \t#!/bin/sh\n-- \n2.43.0\n\n"},{"id":"486443","messageId":"20240109165304.8027-6-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240109060417.1144647-2-shyamthakkar001@gmail.com","subject":"[PATCH v2 2/2] t7501: add test for --amend with --signoff","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-09T16:51:23Z","receivedAt":"2024-01-09T17:01:24Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"This commit adds test for amending commit to add Signed-off-by\ntrailer.\n\nSigned-off-by: Ghanshyam Thakkar \n---\nI believe there are many existing tests which already cover almost\nall of the scenarios. So in addition to this test, I have also\nupdated the FIXME comment to remove signoff.\n\n t/t7501-commit-basic-functionality.sh | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex 9325db1f66..376a7d59cc 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -3,8 +3,7 @@\n # Copyright (c) 2007 Kristian Høgsberg \n #\n \n-# FIXME: Test the various index usages, test reflog,\n-# signoff\n+# FIXME: Test the various index usages, test reflog\n \n test_description='git commit'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n@@ -442,6 +441,18 @@ test_expect_success 'amend commit to fix date' '\n \n '\n \n+test_expect_success 'amend commit to add signoff' '\n+\n+\ttest_commit file file content &&\n+\tgit commit --amend --signoff &&\n+\ttest_commit_message HEAD <<-EOF\n+\tfile\n+\n+\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\n+'\n+\n test_expect_success 'commit mentions forced date in output' '\n \tgit commit --amend --date=2010-01-02T03:04:05 >output &&\n \tgrep \"Date: *Sat Jan 2 03:04:05 2010\" output\n-- \n2.43.0\n\n"},{"id":"486444","messageId":"CYACBX8Y10KJ.3QVLHQN8LLZ86@gmail.com","threadId":"60703","inReplyTo":"CAP8UFD0GJf5+eOTxy6s+zCzpDmCU_FY4BjwtjTE7RvZ5mKettA@mail.gmail.com","subject":"Re: [PATCH 1/2] t7501: Add tests for various index usages, -i and -o, of commit command.","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-09T17:10:20Z","receivedAt":"2024-01-09T17:10:24Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Tue Jan 9, 2024 at 2:50 PM IST, Christian Couder wrote:\n> First about the commit subject:\n>\n> \"t7501: Add tests for various index usages, -i and -o, of commit command.\"\n>\n> it should be shorter, shouldn't end with a \".\" and \"Add\" should be \"add\".\nUpdated in v2.\n\n> On Tue, Jan 9, 2024 at 7:10 AM Ghanshyam Thakkar\n> wrote:\n> >\n> > This commit adds tests for -i and -o flags of the commit command. It\n> > includes testing -i with and without staged changes, testing -o with and\n> > without staged changes, and testing -i and -o together.\n>\n> A few suggestions to make things a bit more clear:\n>\n> - tell that -i is a synonymous for --include and -o for --only\n> - tell if there are already tests for these options\n> - tell why the tests you add are worth it if tests for an option already exist\n\nI have updated the commit messages in v2 to address these points.\n\n> > Signed-off-by: Ghanshyam Thakkar \n> > ---\n> > t/t7501-commit-basic-functionality.sh | 90 +++++++++++++++++++++++++++\n> > 1 file changed, 90 insertions(+)\n> >\n> > diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\n> > index 3d8500a52e..71dc52ce3a 100755\n> > --- a/t/t7501-commit-basic-functionality.sh\n> > +++ b/t/t7501-commit-basic-functionality.sh\n> > @@ -76,6 +76,96 @@ test_expect_success 'nothing to commit' '\n> > test_must_fail git commit -m initial\n> > '\n> >\n> > +test_expect_success 'commit with -i fails with untracked files' '\n> > + test_when_finished \"rm -rf testdir\" &&\n> > + git init testdir &&\n> > + echo content >testdir/file.txt &&\n> > + test_must_fail git -C testdir commit -i file.txt -m initial\n> > +'\n>\n> Existing tests didn't need a repo, so I am not sure it's worth\n> creating a testdir repo just for this test.\n\nYes, I just wanted to make sure the files were not tracked. However, I\nhave updated these instaces to use the existing repo and use unique\nnon-generic names for generating untracked files.\n\n> Also I am not sure this is the best place in the test script to add -i\n> and -o related tests. Here we are between a 'nothing to commit' test\n> and a '--dry-run fails with nothing to commit' and then more 'nothing\n> to commit' related tests. I think it would be better if all those\n> 'nothing to commit' related tests could stay together.\n\nI have moved these tests above the \"--amend\" related tests, which do not\nbreak the flow of the tests.\n\n> > +test_expect_success 'commit with -i' '\n> > + echo content >bar &&\n> > + git add bar &&\n> > + git commit -m \"bar\" &&\n>\n> Why is the above needed for this test?\nThis was to make sure that the file is tracked, however, I realised that\ncommitting is not necessary, so I have updated this test to use existing\nfiles in repo and to not generate a new one.\n>\n> > + echo content2 >bar &&\n> > + git commit -i bar -m \"bar second\"\n> > +'\n> > +\n> > +test_expect_success 'commit with -i multiple files' '\n> > + test_when_finished \"git reset --hard\" &&\n> > + echo content >bar &&\n> > + echo content >baz &&\n> > + echo content >saz &&\n> > + git add bar baz saz &&\n> > + git commit -m \"bar baz saz\" &&\n>\n> Not sure why the above is needed here too.\nSimilar to above, I have updated this test to use existing files in repo\nand to not generate a new one.\n>\n> > + echo content2 >bar &&\n> > + echo content2 >baz &&\n> > + echo content2 >saz &&\n> > + git commit -i bar saz -m \"bar\" &&\n> > + git diff --name-only >remaining &&\n> > + test_grep \"baz\" remaining\n> > +'\n> > +\n> > +test_expect_success 'commit with -i includes staged changes' '\n> > + test_when_finished \"git reset --hard\" &&\n> > + echo content >bar &&\n> > + git add bar &&\n> > + git commit -m \"bar\" &&\n> > +\n> > + echo content2 >bar &&\n> > + echo content2 >baz &&\n> > + git add baz &&\n> > + git commit -i bar -m \"bar baz\" &&\n> > + git diff --name-only >remaining &&\n> > + test_must_be_empty remaining &&\n> > + git diff --name-only --staged >remaining &&\n> > + test_must_be_empty remaining\n> > +'\n> > +\n> > +test_expect_success 'commit with -o' '\n> > + echo content >bar &&\n> > + git add bar &&\n> > + git commit -m \"bar\" &&\n> > + echo content2 >bar &&\n> > + git commit -o bar -m \"bar again\"\n> > +'\n> > +\n> > +test_expect_success 'commit with -o fails with untracked files' '\n> > + test_when_finished \"rm -rf testdir\" &&\n> > + git init testdir &&\n> > + echo content >testdir/bar &&\n> > + test_must_fail git -C testdir commit -o bar -m \"bar\"\n> > +'\n> > +\n> > +test_expect_success 'commit with -o exludes staged changes' '\n>\n> s/exludes/excludes/\nDone.\n>\n> > + test_when_finished \"git reset --hard\" &&\n> > + echo content >bar &&\n> > + echo content >baz &&\n> > + git add . &&\n> > + git commit -m \"bar baz\" &&\n> > +\n> > + echo content2 >bar &&\n> > + echo content2 >baz &&\n> > + git add baz &&\n> > + git commit -o bar -m \"bar\" &&\n> > + git diff --name-only --staged >actual &&\n> > + test_grep \"baz\" actual\n> > +'\n>\n> Thanks.\n\nThanks for the review!\n"},{"id":"486447","messageId":"CYACMZ0L7W54.1VITNQ1ZHLZDP@gmail.com","threadId":"60703","inReplyTo":"2113e178-149b-49aa-9d78-ff1c480f754c@gmail.com","subject":"Re: [PATCH 2/2] t7501: Add test for amending commit to add signoff.","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-09T17:24:46Z","receivedAt":"2024-01-09T17:24:50Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Tue Jan 9, 2024 at 4:14 PM IST, Phillip Wood wrote:\n> Hi Ghanshyam\n>\n> On 09/01/2024 06:04, Ghanshyam Thakkar wrote:\n> > This commit adds test for amending the latest commit to add\n> > Signed-off-by trailer at the end of commit message.\n>\n> If we're not already testing this then it seems like a useful addition, \n> thanks for working on it. It would also be helpful to check that \"git \n> commit --amend --signoff\" does not add a Signed-off-by: trailer if it \n> already exists.\n\nI hadn't thought of that. I have hastily sent the v2 without seeing this\ncomment. I will add this test in v3.\n>\n> > +test_expect_success 'amend commit to add signoff' '\n> > +\n> > +\ttest_when_finished \"rm -rf testdir\" &&\n> > +\tgit init testdir &&\n>\n> As Christian said about the other patch in this series I don't think we \n> need a new repository here. In our test files we use the same repository \n> for the whole file unless there is a compelling reason not to.\n\nUpdated from v2 onwards.\n>\n> > +\techo content >testdir/file &&\n> > +\tgit -C testdir add file &&\n> > +\tgit -C testdir commit -m \"file\" &&\n>\n> I think these three lines can be replaced by\n>\n> \ttest_commit --no-tag file file content\n\nThank you for the suggestion. I have updated the test to use test_commit.\n>\n> > +\tgit -C testdir commit --amend --signoff &&\n>\n> > +\tgit -C testdir log -1 --pretty=format:%B >actual &&\n> > +\t(\n> > +\t\techo file &&\n> > +\t\techo &&\n> > +\t\tgit -C testdir var GIT_COMMITTER_IDENT >ident &&\n> > +\t\tsed -e \"s/>.*/>/\" -e \"s/^/Signed-off-by: /\" ident\n> > +\t) >expected &&\n> > +\ttest_cmp expected actual\n>\n> This section of the test can be improved by using test_commit_message\n>\n> \ttest_commit_message HEAD <<-EOF\n> \tfile\n> \t\n> \tSigned-off-by: $GIT_COMMITER_NAME <$GIT_COMMITTER_EMAIL>\n> \tEOF\nI have updated the test to use above approach.\n\nThank you for the review!\n"},{"id":"486448","messageId":"xmqqplyaif4l.fsf@gitster.g","threadId":"60703","inReplyTo":"20240109060417.1144647-3-shyamthakkar001@gmail.com","subject":"Re: [PATCH 1/2] t7501: Add tests for various index usages, -i and -o, of commit command.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-09T17:35:38Z","receivedAt":"2024-01-09T17:35:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> Subject: Re: [PATCH 1/2] t7501: Add tests for various index usages, -i and -o, of commit command.\n\nOverly long subject that has an unusual capitalization after\n\"t7501:\" (see \"git log --no-merges --format=%s -20 v2.43.0\" for\nexample and try to write something that blends better).\n\n> +test_expect_success 'commit with -i fails with untracked files' '\n> +\ttest_when_finished \"rm -rf testdir\" &&\n> +\tgit init testdir &&\n> +\techo content >testdir/file.txt &&\n> +\ttest_must_fail git -C testdir commit -i file.txt -m initial\n> +'\n\nIn addition to \"why a new repository???\" comment raised already, I\ndo not want to see the last command spelled like so. Always write\ndashed options (and their parameters) before non-option arguments,\ni.e.\n\n\tgit commit -i -m initial file.txt\n\tgit -C testdir commit -i -m initial file.txt\n\ttest_must_fail git -C testdir commit -i -m initial file.txt\n\nThe command line parser does rotate the unrecognized arguments to\nthe end and keeps looking for recognisable option (possibly followed\nby its parameter), but that is purely to help lazy writers (i.e.,\ninteractive command users). When writers know \"-i\" does not take\nany parameter, it may be convenient if the writer who forgot to say\n\"-m\" can just append \"-m initial\" to what has already be written.\n\nWhen writing source (be it the production code or test), however, we\nwrite for readers. What you wrote at a first glance, especially\ngiven that \"-i\" (or \"-o\" for that matter) is a relatively less\ncommonly used option, would confuse less experienced readers by\nmaking them wonder what \"-i file.txt\" means (e.g., \"is that taking\ninput from the contents of file.txt?\").\n\n"},{"id":"486449","messageId":"xmqqedeqienh.fsf@gitster.g","threadId":"60703","inReplyTo":"20240109060417.1144647-4-shyamthakkar001@gmail.com","subject":"Re: [PATCH 2/2] t7501: Add test for amending commit to add signoff.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-09T17:45:54Z","receivedAt":"2024-01-09T17:46:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> Subject: Re: [PATCH 2/2] t7501: Add test for amending commit to add signoff.\n\nThe title is with unusual capitalization and final full-stop (again,\ncheck \"git log --no-merges --format=%s -20 v2.43.0\" and try to blend\nin).\n\n> This commit adds test for amending the latest commit to add\n> Signed-off-by trailer at the end of commit message.\n\n\"This commit adds ...\" -> \"Add ...\"\n\nAlso what the patch does can be read from the patch text below, but\nit cannot be read _why_ the patch author thought it was a good idea\nto make such a change. The proposed commit log message is a place\nto describe the reason behind the patch. Why do we want a new test?\nWhy do we want that new test in this particular file? etc.\n\n> +test_expect_success 'amend commit to add signoff' '\n> +\n> +\ttest_when_finished \"rm -rf testdir\" &&\n> +\tgit init testdir &&\n\nThe same \"why a new repository for just this test???\" applies here.\n\n> +\techo content >testdir/file &&\n> +\tgit -C testdir add file &&\n> +\tgit -C testdir commit -m \"file\" &&\n> +\tgit -C testdir commit --amend --signoff &&\n> +\tgit -C testdir log -1 --pretty=format:%B >actual &&\n\nIf you are doing many things in a separate directory, the usual\npattern is\n\n\t# create a directory DIR (usuall \"mkdir\", not \"git init\")\n\tmkdir DIR &&\n\t(\n\t\tcd DIR &&\n\t\tgit do this &&\n\t\tgit do that &&\n\t\tinspect the result of this >actual &&\n\t\tprepare the expected outcome >expect &&\n\t\ttest_cmp expect actual\n\t) &&\n\nThanks.\n"},{"id":"486450","messageId":"xmqq5y02iegj.fsf@gitster.g","threadId":"60703","inReplyTo":"20240109165304.8027-4-shyamthakkar001@gmail.com","subject":"Re: [PATCH v2 1/2] t7501: add tests for --include, --only of commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-09T17:50:04Z","receivedAt":"2024-01-09T17:50:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> This commit adds tests for testing --include (-o) and --only (-o). It\n\n\"-i\" not \"-o\" stands for \"--include\".\n\nPlease write in imperative mood.\n\n> +test_expect_success '--include fails with untracked files' '\n> +\techo content >baz &&\n> +\ttest_must_fail git commit --include baz -m initial\n> +'\n\nMy comment on argument order applies to this iteration, too. Please\nwrite your code to help readers.\n\n> +test_expect_success 'commit --include' '\n> +\ttest_when_finished \"rm -rf remaining\" &&\n\nWhy recursive removal when you _know_ what you create is a plan\nfile?\n"},{"id":"486537","messageId":"20240110163622.51182-2-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240109165304.8027-2-shyamthakkar001@gmail.com","subject":"[PATCH v3 0/2] t7501: add tests for --include, --only and","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-10T16:35:16Z","receivedAt":"2024-01-10T16:37:07Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"The v3 of the patch series addresses the comments from Junio and adds\nthe test to check --amend --signoff does not add signoff if it\nalready exists as suggested by Phillip Wood.\n\nAlso I have removed two tests from v2 which tested the same thing as\nother tests in the series, namely:\n\n- removed 'commit --include'. This pattern is tested in '--include with \nstaged changes'.\n- removed 'commit --only'. This pattern is tested in '--only excludes \nstaged changes'.\n\nGhanshyam Thakkar (2):\n t7501: add tests for --include and --only\n t7501: add tests for --amend --signoff\n\n t/t7501-commit-basic-functionality.sh | 68 ++++++++++++++++++++++++++-\n 1 file changed, 66 insertions(+), 2 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"486538","messageId":"20240110163622.51182-4-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240109165304.8027-2-shyamthakkar001@gmail.com","subject":"[PATCH v3 1/2] t7501: add tests for --include and --only","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-10T16:35:18Z","receivedAt":"2024-01-10T16:37:29Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Add tests for --include (-i) and --only (-o) of commit. This includes\ntesting --include with and without staged changes, testing --only with\nand without staged changes and testing --only and --include together.\n\nSome tests already exist in t7501 for testing --only, however, it is\nonly tested in combination with --amend, --allow-empty and to-be-born\nbranch, and not for testing --only separately.\n\nSimilarly there are no separate tests for --include.\n\nThese tests are an addition to other similar tests in t7501,\nas described above in the case of --only, therefore, they belong\nin t7501-commit-basic-functionality.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n t/t7501-commit-basic-functionality.sh | 43 ++++++++++++++++++++++++++-\n 1 file changed, 42 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex 3d8500a52e..e005175d0b 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -3,7 +3,7 @@\n # Copyright (c) 2007 Kristian Høgsberg \n #\n \n-# FIXME: Test the various index usages, -i and -o, test reflog,\n+# fixme: test the various index usages, test reflog,\n # signoff\n \n test_description='git commit'\n@@ -150,6 +150,47 @@ test_expect_success 'setup: commit message from file' '\n \tgit commit -F msg -a\n '\n \n+test_expect_success '--include fails with untracked files' '\n+\techo content >baz &&\n+\ttest_must_fail git commit --include -m \"initial\" baz\n+'\n+\n+test_expect_success '--include with staged changes' '\n+\techo newcontent >baz &&\n+\techo newcontent >file &&\n+\tgit add file &&\n+\tgit commit --include -m \"file baz\" baz &&\n+\n+\tgit diff --name-only >remaining &&\n+\ttest_must_be_empty remaining &&\n+\tgit diff --name-only --staged >remaining &&\n+\ttest_must_be_empty remaining\n+'\n+\n+test_expect_success '--only fails with untracked files' '\n+\techo content >newfile &&\n+\ttest_must_fail git commit --only -m \"newfile\" newfile\n+'\n+\n+test_expect_success '--only excludes staged changes' '\n+\techo content >file &&\n+\techo content >baz &&\n+\tgit add baz &&\n+\tgit commit --only -m \"file\" file &&\n+\n+\tgit diff --name-only >actual &&\n+\ttest_must_be_empty actual &&\n+\tgit diff --name-only --staged >actual &&\n+\ttest_grep \"baz\" actual\n+'\n+\n+test_expect_success '--include and --only do not mix' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo new >file &&\n+\techo new >baz &&\n+\ttest_must_fail git commit --include --only -m \"bar\" bar baz\n+'\n+\n test_expect_success 'amend commit' '\n \tcat >editor <<-\\EOF &&\n \t#!/bin/sh\n-- \n2.43.0\n\n"},{"id":"486539","messageId":"20240110163622.51182-6-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240109165304.8027-2-shyamthakkar001@gmail.com","subject":"[PATCH v3 2/2] t7501: add tests for --amend --signoff","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-10T16:35:20Z","receivedAt":"2024-01-10T16:37:41Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Add tests for amending the commit to add Signed-off-by trailer. And\nalso to check if it does not add another trailer if one already exists.\n\nCurrently, there are tests for --signoff separately in t7501, however,\nthey are not tested with --amend.\n\nTherefore, these tests belong with other similar tests of --amend in\nt7501-commit-basic-functionality.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n t/t7501-commit-basic-functionality.sh | 27 +++++++++++++++++++++++++--\n 1 file changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex e005175d0b..546d60d7fc 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -3,8 +3,7 @@\n # Copyright (c) 2007 Kristian Høgsberg \n #\n \n-# fixme: test the various index usages, test reflog,\n-# signoff\n+# fixme: test the various index usages, test reflog\n \n test_description='git commit'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n@@ -430,6 +429,30 @@ test_expect_success 'amend commit to fix date' '\n \n '\n \n+test_expect_success 'amend commit to add signoff' '\n+\n+\ttest_commit \"msg\" file content &&\n+\tgit commit --amend --signoff &&\n+\ttest_commit_message HEAD <<-EOF\n+\tmsg\n+\n+\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\n+'\n+\n+test_expect_success 'amend does not add signoff if it already exists' '\n+\n+\ttest_commit --signoff \"tenor\" file newcontent &&\n+\tgit commit --amend --signoff &&\n+\ttest_commit_message HEAD <<-EOF\n+\ttenor\n+\n+\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\n+'\n+\n test_expect_success 'commit mentions forced date in output' '\n \tgit commit --amend --date=2010-01-02T03:04:05 >output &&\n \tgrep \"Date: *Sat Jan 2 03:04:05 2010\" output\n-- \n2.43.0\n\n"},{"id":"486553","messageId":"xmqqil41avcc.fsf@gitster.g","threadId":"60703","inReplyTo":"20240110163622.51182-4-shyamthakkar001@gmail.com","subject":"Re: [PATCH v3 1/2] t7501: add tests for --include and --only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-10T18:37:07Z","receivedAt":"2024-01-10T18:37:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> -# FIXME: Test the various index usages, -i and -o, test reflog,\n> +# fixme: test the various index usages, test reflog,\n> # signoff\n\nWhy the sudden downcasing? If this were to turn it to \"TODO:\"\n(110+) or \"NEEDSWORK:\" (110+) from less often used \"FIXME:\" (40-),\nsuch a change might be defensible, but there is only one instance\nof downcased \"fixme:\", so I am curious how this happened.\n\n> +test_expect_success '--include fails with untracked files' '\n> +\techo content >baz &&\n> +\ttest_must_fail git commit --include -m \"initial\" baz\n> +'\n\nOK, this is because \"--allow-empty\" is not passed. This should fail\nwithout -i/-o (which should be the same as passing -o), with -i, or\nwith -o.\n\nCalling this commit \"initial\" is highly misleading. There are bunch\nof commits already, but path \"baz\" has never been used.\n\n> +test_expect_success '--include with staged changes' '\n> +\techo newcontent >baz &&\n> +\techo newcontent >file &&\n> +\tgit add file &&\n> +\tgit commit --include -m \"file baz\" baz &&\n\nI suspect that you found a bug whose behaviour we do not want to\ncarve into stone with this test. When this test begins, because the\nprevious step failed to create the initial commit, we are creating\nthe root commit, without --allow-empty, with contents in the index\nfor path \"file\". At this point\n\n $ git commit -m \"file baz\" baz\n\n(or with \"-o\", which is the same thing) does error out with\n\n error: pathspec 'baz' did not match any file(s) known to git\n\nbecause the \"only\" thing is to take the changes between HEAD and the\nindex and limit them further to those paths that match \"baz\", but\nthere is no path that matches \"baz\".\n\nThis command\n\n $ git commit -m \"file baz\" -i baz\n\nshould either error out the same way, or behave more or less[*] like\n\n $ git add baz && git commit -m \"file baz\"\n\nand record changes to both \"file\" and \"baz\".\n\n Side note: The \"more or less\" is we should do \"git rm baz\"\n instead, if you removed the path.\n\nBut it seems that the current code simply ignores the unmatching\npathspec \"baz\" that is on the command line, hence recording only the\nchange to the contents of \"file\".\n\n> +\tgit diff --name-only >remaining &&\n> +\ttest_must_be_empty remaining &&\n> +\tgit diff --name-only --staged >remaining &&\n> +\ttest_must_be_empty remaining\n\nThese two tests to see if the working tree is clean and if the index\nis clean are not wrong per-se, but is insufficient. Judging from\nthe commit message you used, I think you expected this commit to\ncontain both changes to 'file' and 'baz'. That needs to be also\nchecked with something like \"git diff --name-only HEAD^ HEAD\".\n\nNow which behaviour between \"error out because there is no path in\nthe index that matches pathspec 'baz'\" and \"add baz to the index and\ncommit that addition, together with what is already in the index\" we\nwould want to take is probably open for discussion. Such a discussion\nmay decide that the current behaviour is fine. Until then...\n\n> +test_expect_success '--only fails with untracked files' '\n> +\techo content >newfile &&\n> +\ttest_must_fail git commit --only -m \"newfile\" newfile\n> +'\n\nAnd this should fail the same way without \"-o\". Don't we have such\na test that we can just sneak \"with -o the same thing happens\" test\nnext to it?\n\n> +test_expect_success '--only excludes staged changes' '\n> +\techo content >file &&\n> +\techo content >baz &&\n> +\tgit add baz &&\n> +\tgit commit --only -m \"file\" file &&\n\nThis should behave exactly the same way without \"-o\".\n\n> +\tgit diff --name-only >actual &&\n> +\ttest_must_be_empty actual &&\n> +\tgit diff --name-only --staged >actual &&\n> +\ttest_grep \"baz\" actual\n> +'\n> +\n> +test_expect_success '--include and --only do not mix' '\n> +\ttest_when_finished \"git reset --hard\" &&\n> +\techo new >file &&\n> +\techo new >baz &&\n> +\ttest_must_fail git commit --include --only -m \"bar\" bar baz\n\nOK. If you grep for 'cannot be used together' in t/ directory,\nthere are many tests that make sure how an invocation like this\nshould fail, i.e. with an error message that mentions incompatible\noptions. Don't we want to do the same?\n\nThanks.\n\n"},{"id":"486587","messageId":"CYBI6R7H38H9.2VXJRFAHZNJZ1@gmail.com","threadId":"60703","inReplyTo":"xmqqil41avcc.fsf@gitster.g","subject":"Re: [PATCH v3 1/2] t7501: add tests for --include and --only","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-11T01:58:21Z","receivedAt":"2024-01-11T01:58:27Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Thu Jan 11, 2024 at 12:07 AM IST, Junio C Hamano wrote:\n> Ghanshyam Thakkar writes:\n>\n> > -# FIXME: Test the various index usages, -i and -o, test reflog,\n> > +# fixme: test the various index usages, test reflog,\n> > # signoff\n>\n> Why the sudden downcasing? If this were to turn it to \"TODO:\"\n> (110+) or \"NEEDSWORK:\" (110+) from less often used \"FIXME:\" (40-),\n> such a change might be defensible, but there is only one instance\n> of downcased \"fixme:\", so I am curious how this happened.\n\nWow, I must have done it mistakenly. I guess everything on that line\nbecame lowercase. But, I will fix it.\n\n> > +test_expect_success '--include fails with untracked files' '\n> > +\techo content >baz &&\n> > +\ttest_must_fail git commit --include -m \"initial\" baz\n> > +'\n>\n> OK, this is because \"--allow-empty\" is not passed. This should fail\n> without -i/-o (which should be the same as passing -o), with -i, or\n> with -o.\n>\n> Calling this commit \"initial\" is highly misleading. There are bunch\n> of commits already, but path \"baz\" has never been used.\n\nI will fix this.\n\n> > +test_expect_success '--include with staged changes' '\n> > +\techo newcontent >baz &&\n> > +\techo newcontent >file &&\n> > +\tgit add file &&\n> > +\tgit commit --include -m \"file baz\" baz &&\n>\n> I suspect that you found a bug whose behaviour we do not want to\n> carve into stone with this test. When this test begins, because the\n> previous step failed to create the initial commit, we are creating\n> the root commit, without --allow-empty, with contents in the index\n> for path \"file\". At this point\n>\n> $ git commit -m \"file baz\" baz\n>\n> (or with \"-o\", which is the same thing) does error out with\n>\n> error: pathspec 'baz' did not match any file(s) known to git\n>\n> because the \"only\" thing is to take the changes between HEAD and the\n> index and limit them further to those paths that match \"baz\", but\n> there is no path that matches \"baz\".\n\n\n> This command\n>\n> $ git commit -m \"file baz\" -i baz\n>\n> should either error out the same way, or behave more or less[*] like\n>\n> $ git add baz && git commit -m \"file baz\"\n>\n> and record changes to both \"file\" and \"baz\".\n>\n> Side note: The \"more or less\" is we should do \"git rm baz\"\n> instead, if you removed the path.\n>\n> But it seems that the current code simply ignores the unmatching\n> pathspec \"baz\" that is on the command line, hence recording only the\n> change to the contents of \"file\".\n>\nyeah, it seems like if there is anything staged, even if we provide\n--include with untracked files, it will commit the staged files\n(excluding the untracked files) and will not error out like --only.\n\nAlso in v1 there was another test before this one which added the\nbaz file to the index. That test was removed due to duplication and\nwhile cleaning up the v1 for v2, due to --include not giving an error,\nI did not notice that 'baz' was being left out. Will fix the tests.\nApologies for such mistakes.\n\n> > +\tgit diff --name-only >remaining &&\n> > +\ttest_must_be_empty remaining &&\n> > +\tgit diff --name-only --staged >remaining &&\n> > +\ttest_must_be_empty remaining\n>\n> These two tests to see if the working tree is clean and if the index\n> is clean are not wrong per-se, but is insufficient. Judging from\n> the commit message you used, I think you expected this commit to\n> contain both changes to 'file' and 'baz'. That needs to be also\n> checked with something like \"git diff --name-only HEAD^ HEAD\".\n\nYeah, the \"git diff --name-only HEAD^ HEAD\" is more definitive.\nI will add that in v4.\n\n> Now which behaviour between \"error out because there is no path in\n> the index that matches pathspec 'baz'\" and \"add baz to the index and\n> commit that addition, together with what is already in the index\" we\n> would want to take is probably open for discussion. Such a discussion\n> may decide that the current behaviour is fine. Until then...\n>\n> > +test_expect_success '--only fails with untracked files' '\n> > +\techo content >newfile &&\n> > +\ttest_must_fail git commit --only -m \"newfile\" newfile\n> > +'\n>\n> And this should fail the same way without \"-o\". Don't we have such\n> a test that we can just sneak \"with -o the same thing happens\" test\n> next to it?\n\nWell, I could not find any test which specifically for untracked\nfiles. I will keep looking for it and if not found, perhaps, I can add\nboth \"-i and -o with untracked files\" tests after \"nothing to commit\"\ntests in t7501 which are similar in nature.\n\n> > +\tgit commit --only -m \"file\" file &&\n>\n> This should behave exactly the same way without \"-o\".\n\nThat is true, however, I could not find any tests in the t75-- series\nthat test without -o and which provide pathspec in the commit command\nalso. So, should I drop -o option in this test? or add a test without\n-o?\n\n>\n> > +\tgit diff --name-only >actual &&\n> > +\ttest_must_be_empty actual &&\n> > +\tgit diff --name-only --staged >actual &&\n> > +\ttest_grep \"baz\" actual\n> > +'\n> > +\n> > +test_expect_success '--include and --only do not mix' '\n> > +\ttest_when_finished \"git reset --hard\" &&\n> > +\techo new >file &&\n> > +\techo new >baz &&\n> > +\ttest_must_fail git commit --include --only -m \"bar\" bar baz\n>\n> OK. If you grep for 'cannot be used together' in t/ directory,\n> there are many tests that make sure how an invocation like this\n> should fail, i.e. with an error message that mentions incompatible\n> options. Don't we want to do the same?\n\nI did it like this because in t7501, there are couple of existing \n\"do not mix\" tests similar to this one, which only check if the command\nfails. However, the approach you mentioned is obviously better, so, I\nwill update it in v4.\n\n> Thanks.\n\nThank you for the review!\n"},{"id":"486633","messageId":"02cf7ef3-6a16-4fe1-a032-ea7c727c9b12@gmail.com","threadId":"60703","inReplyTo":"20240110163622.51182-6-shyamthakkar001@gmail.com","subject":"Re: [PATCH v3 2/2] t7501: add tests for --amend --signoff","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-01-11T16:30:18Z","receivedAt":"2024-01-11T16:30:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/01/2024 16:35, Ghanshyam Thakkar wrote:\n> Add tests for amending the commit to add Signed-off-by trailer. And\n> also to check if it does not add another trailer if one already exists.\n> \n> Currently, there are tests for --signoff separately in t7501, however,\n> they are not tested with --amend.\n> \n> Therefore, these tests belong with other similar tests of --amend in\n> t7501-commit-basic-functionality.\n> \n> Signed-off-by: Ghanshyam Thakkar \n\nThis version looks good, thanks for re-rolling. I have left one comment \nbelow but it is not worth re-rolling just for that.\n\n> +test_expect_success 'amend commit to add signoff' '\n> +\n> +\ttest_commit \"msg\" file content &&\n> +\tgit commit --amend --signoff &&\n> +\ttest_commit_message HEAD <<-EOF\n> +\tmsg\n> +\n> +\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n> +\tEOF\n> +\n> +'\n\nIf you do happen re-roll I think we could happily lose the empty line \nbefore the closing \"'\" in this test and the next.\n\nBest Wishes\n\nPhillip\n"},{"id":"486634","messageId":"ba8824a3-caa9-4de2-bb80-3e592ec62b92@gmail.com","threadId":"60703","inReplyTo":"20240110163622.51182-4-shyamthakkar001@gmail.com","subject":"Re: [PATCH v3 1/2] t7501: add tests for --include and --only","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-01-11T16:33:57Z","receivedAt":"2024-01-11T16:34:00Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/01/2024 16:35, Ghanshyam Thakkar wrote:\n\nI don't have much to add to what Junio said, I've just left one comment \nbelow\n\n> +test_expect_success '--only excludes staged changes' '\n> +\techo content >file &&\n> +\techo content >baz &&\n> +\tgit add baz &&\n> +\tgit commit --only -m \"file\" file &&\n> +\n> +\tgit diff --name-only >actual &&\n> +\ttest_must_be_empty actual &&\n> +\tgit diff --name-only --staged >actual &&\n> +\ttest_grep \"baz\" actual\n\nusing test_grep feels a bit weak here, I think it would be better to use \ntest_cmp to ensure that there are no other staged changes.\n\nBest Wishes\n\nPhillip\n"},{"id":"486719","messageId":"20240112180109.59350-1-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240110163622.51182-2-shyamthakkar001@gmail.com","subject":"[PATCH v4 0/2] t7501: add tests for --include, --only, --signoff","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-12T18:00:15Z","receivedAt":"2024-01-12T18:01:55Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"The v4 of this series fixes the issue reported by Junio in v3, in\nwhich one of the files was not being added to the index before\ncommitting. This is fixed in v4.\n\nAlso 'untracked files' tests are moved after the 'nothing to commit\ntests', and also show that with -i and -o, behavior remains same as \nwhen not using any flag.\n\nThe v4 also adds extra step to check that certain files are being\ncommitted. Specifically like,\n\n 'git diff --name-only HEAD^ HEAD'\n\nas per Junio's suggestion.\n\nThe '-i and -o do not mix' test is updated according to Junio's\nsuggestion.\n\nApart from that, certain instances of 'test_grep' are replaced with\n'test_cmp' as suggested by Phillip.\n\nThe --signoff tests remain the same apart from removal of empty line\nas pointed out by Phillip.\n\nGhanshyam Thakkar (2):\n t7501: add tests for --include and --only\n t7501: add tests for --amend --signoff\n\n t/t7501-commit-basic-functionality.sh | 102 +++++++++++++++++++++++++-\n 1 file changed, 100 insertions(+), 2 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"486720","messageId":"20240112180109.59350-2-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240112180109.59350-1-shyamthakkar001@gmail.com","subject":"[PATCH v4 1/2] t7501: add tests for --include and --only","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-12T18:00:16Z","receivedAt":"2024-01-12T18:01:59Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Add tests for --only (-o) and --include (-i). This include testing\nwith or without staged changes for both -i and -o. Also to test\nfor committing untracked files with -i, -o and without -i/-o.\n\nSome tests already exist in t7501 for testing --only, however,\nit is tested in combination with --amend and --allow-empty and on\nto-be-born branch. The addition of these tests check, when the\npathspec is provided, that only the files matching the pathspec\nget committed. This behavior is same when we provide --only\n(as --only is the default mode of operation when pathspec is\nprovided.)\n\nAs for --include, there is no prior test for checking if --include\nalso commits staged changes.\n\nTherefore, these tests belong in t7501 with other similar existing\ntests, as described in the case of --only.\n\nAnd also add test for checking incompatibilty when using -o and -i\ntogether.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n t/t7501-commit-basic-functionality.sh | 79 ++++++++++++++++++++++++++-\n 1 file changed, 78 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex 3d8500a52e..e4633b4af5 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -3,7 +3,7 @@\n # Copyright (c) 2007 Kristian Høgsberg \n #\n \n-# FIXME: Test the various index usages, -i and -o, test reflog,\n+# FIXME: Test the various index usages, test reflog,\n # signoff\n \n test_description='git commit'\n@@ -92,6 +92,19 @@ test_expect_success '--long fails with nothing to commit' '\n \ttest_must_fail git commit -m initial --long\n '\n \n+test_expect_success 'fail to commit untracked file' '\n+\techo content >baz &&\n+\ttest_must_fail git commit -m \"baz\" baz\n+'\n+\n+test_expect_success '--only also fail to commit untracked file' '\n+\ttest_must_fail git commit --only -m \"baz\" baz\n+'\n+\n+test_expect_success '--include also fail to commit untracked file' '\n+\ttest_must_fail git commit --include -m \"baz\" baz\n+'\n+\n test_expect_success 'setup: non-initial commit' '\n \techo bongo bongo bongo >file &&\n \tgit commit -m next -a\n@@ -117,6 +130,70 @@ test_expect_success '--long with stuff to commit returns ok' '\n \tgit commit -m next -a --long\n '\n \n+test_expect_success 'only commit given path (also excluding additional staged changes)' '\n+\techo content >file &&\n+\techo content >baz &&\n+\tgit add baz &&\n+\tgit commit -m \"file\" file &&\n+\n+\tgit diff --name-only >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\tgit diff --name-only --staged >actual &&\n+\ttest_cmp - actual <<-EOF &&\n+\tbaz\n+\tEOF\n+\n+\tgit diff --name-only HEAD^ HEAD >actual &&\n+\ttest_cmp - actual <<-EOF\n+\tfile\n+\tEOF\n+'\n+\n+test_expect_success 'same as above with -o/--only' '\n+\techo change >file &&\n+\techo change >baz &&\n+\tgit add baz &&\n+\tgit commit --only -m \"file\" file &&\n+\n+\tgit diff --name-only >actual &&\n+\ttest_must_be_empty actual &&\n+\n+\tgit diff --name-only --staged >actual &&\n+\ttest_cmp - actual <<-EOF &&\n+\tbaz\n+\tEOF\n+\n+\tgit diff --name-only HEAD^ HEAD >actual &&\n+\ttest_cmp - actual <<-EOF\n+\tfile\n+\tEOF\n+'\n+\n+test_expect_success '-i/--include includes staged changes' '\n+\techo newcontent >file &&\n+\techo newcontent >baz &&\n+\tgit add file &&\n+\tgit commit --include -m \"file baz\" baz &&\n+\n+\tgit diff --name-only HEAD >remaining &&\n+\ttest_must_be_empty remaining &&\n+\n+\tgit diff --name-only HEAD^ HEAD >changes &&\n+\ttest_cmp - changes <<-EOF\n+\tbaz\n+\tfile\n+\tEOF\n+'\n+\n+test_expect_success '--include and --only do not mix' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo new >file &&\n+\techo new >baz &&\n+\ttest_must_fail git commit --include --only -m \"file baz\" file baz 2>actual &&\n+\ttest_grep -e \"fatal: options .-i/--include. and .-o/--only. cannot be used together\" actual\n+'\n+\n test_expect_success 'commit message from non-existing file' '\n \techo more bongo: bongo bongo bongo bongo >file &&\n \ttest_must_fail git commit -F gah -a\n-- \n2.43.0\n\n"},{"id":"486721","messageId":"20240112180109.59350-3-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240112180109.59350-1-shyamthakkar001@gmail.com","subject":"[PATCH v4 2/2] t7501: add tests for --amend --signoff","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-12T18:00:17Z","receivedAt":"2024-01-12T18:02:10Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Add tests for amending the commit to add Signed-off-by trailer. And\nalso to check if it does not add another trailer if one already exists.\n\nCurrently, there are tests for --signoff separately in t7501, however,\nthey are not tested with --amend.\n\nTherefore, these tests belong with other similar tests of --amend in\nt7501-commit-basic-functionality.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n t/t7501-commit-basic-functionality.sh | 25 +++++++++++++++++++++++--\n 1 file changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex e4633b4af5..33a9895e72 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -3,8 +3,7 @@\n # Copyright (c) 2007 Kristian Høgsberg \n #\n \n-# FIXME: Test the various index usages, test reflog,\n-# signoff\n+# FIXME: Test the various index usages, test reflog\n \n test_description='git commit'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n@@ -466,6 +465,28 @@ test_expect_success 'amend commit to fix date' '\n \n '\n \n+test_expect_success 'amend commit to add signoff' '\n+\n+\ttest_commit \"msg\" file content &&\n+\tgit commit --amend --signoff &&\n+\ttest_commit_message HEAD <<-EOF\n+\tmsg\n+\n+\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+'\n+\n+test_expect_success 'amend does not add signoff if it already exists' '\n+\n+\ttest_commit --signoff \"tenor\" file newcontent &&\n+\tgit commit --amend --signoff &&\n+\ttest_commit_message HEAD <<-EOF\n+\ttenor\n+\n+\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+'\n+\n test_expect_success 'commit mentions forced date in output' '\n \tgit commit --amend --date=2010-01-02T03:04:05 >output &&\n \tgrep \"Date: *Sat Jan 2 03:04:05 2010\" output\n-- \n2.43.0\n\n"},{"id":"486746","messageId":"xmqqbk9q6tcs.fsf@gitster.g","threadId":"60703","inReplyTo":"20240112180109.59350-2-shyamthakkar001@gmail.com","subject":"Re: [PATCH v4 1/2] t7501: add tests for --include and --only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-12T23:10:27Z","receivedAt":"2024-01-12T23:10:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> @@ -92,6 +92,19 @@ test_expect_success '--long fails with nothing to commit' '\n> \ttest_must_fail git commit -m initial --long\n> '\n> \n> +test_expect_success 'fail to commit untracked file' '\n> +\techo content >baz &&\n> +\ttest_must_fail git commit -m \"baz\" baz\n> +'\n> +\n> +test_expect_success '--only also fail to commit untracked file' '\n> +\ttest_must_fail git commit --only -m \"baz\" baz\n> +'\n> +\n> +test_expect_success '--include also fail to commit untracked file' '\n> +\ttest_must_fail git commit --include -m \"baz\" baz\n> +'\n\nAs the latter two depends on the first one's side effect of leaving\nan untracked 'baz' file in the working tree, I do not know if it is\nsensible to split these into three tests. An obvious alternative is\nto have a single test\n\n\ttest_expect_success 'pathspec that do not match any tracked path' '\n\t\techo content >baz &&\n\t\ttest_must_fail git commit -m baz baz &&\n\t\ttest_must_fail git commit -o -m baz baz &&\n\t\ttest_must_fail git commit -i -m baz baz\n\t'\n\nBy the way, I do not think presence of 'baz' in the working tree\nmatters in the failures from these tests all that much, as the\nreason they fail is because the pathspec does not match any tracked\nfile, whose contents in the index to be updated before made into a\ncommit.\n\n> @@ -117,6 +130,70 @@ test_expect_success '--long with stuff to commit returns ok' '\n> \tgit commit -m next -a --long\n> '\n> \n> +test_expect_success 'only commit given path (also excluding additional staged changes)' '\n> +\techo content >file &&\n> +\techo content >baz &&\n> +\tgit add baz &&\n> +\tgit commit -m \"file\" file &&\n> +\n> +\tgit diff --name-only >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\tgit diff --name-only --staged >actual &&\n> +\ttest_cmp - actual <<-EOF &&\n> +\tbaz\n> +\tEOF\n> +\n> +\tgit diff --name-only HEAD^ HEAD >actual &&\n> +\ttest_cmp - actual <<-EOF\n> +\tfile\n> +\tEOF\n> +'\n\nOK. The change to baz is already in the index, but \"commit file\"\nwill skip over it and record only the changes to file in the commit.\nVery much makes sense.\n\n> +test_expect_success 'same as above with -o/--only' '\n> +\techo change >file &&\n> +\techo change >baz &&\n> +\tgit add baz &&\n> +\tgit commit --only -m \"file\" file &&\n> +\n> +\tgit diff --name-only >actual &&\n> +\ttest_must_be_empty actual &&\n> +\n> +\tgit diff --name-only --staged >actual &&\n> +\ttest_cmp - actual <<-EOF &&\n> +\tbaz\n> +\tEOF\n> +\n> +\tgit diff --name-only HEAD^ HEAD >actual &&\n> +\ttest_cmp - actual <<-EOF\n> +\tfile\n> +\tEOF\n> +'\n\nLikewise. An obvious thing to notice is that this cannot use the\nsame \"contents\" text as before, even though it claims to be \"same as\nabove\". If the final contents of \"file\" and \"baz\" does not matter,\nbut it matters more that these files have been changed, it often is\na good idea to append to the file. That way, you can ensure that\nyou will be making them different, no matter what the initial\ncondition was, i.e.,\n\n\tfor opt in \"\" \"-o\" \"--only\"\n\tdo\n\t\ttest_expect_success 'skip over already added change' '\n\t\t\techo more >>file &&\n\t\t\techo more >>baz &&\n\t\t\tgit add baz &&\n\t\t\tgit commit $opt -m \"file\" file &&\n\n\t\t\t... ensure that changes to file are committed\n\t\t\t... and changes to baz is only in the index\n\t\t'\n\tdone\n\nlet's you test all three combinations.\n\nThanks.\n"},{"id":"486749","messageId":"CYD67AE9V26S.YEDQYOHZJV9M@gmail.com","threadId":"60703","inReplyTo":"xmqqbk9q6tcs.fsf@gitster.g","subject":"Re: [PATCH v4 1/2] t7501: add tests for --include and --only","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-13T01:00:10Z","receivedAt":"2024-01-13T01:00:15Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Sat Jan 13, 2024 at 4:40 AM IST, Junio C Hamano wrote:\n> Ghanshyam Thakkar writes:\n>\n> > @@ -92,6 +92,19 @@ test_expect_success '--long fails with nothing to commit' '\n> > \ttest_must_fail git commit -m initial --long\n> > '\n> > \n> > +test_expect_success 'fail to commit untracked file' '\n> > +\techo content >baz &&\n> > +\ttest_must_fail git commit -m \"baz\" baz\n> > +'\n> > +\n> > +test_expect_success '--only also fail to commit untracked file' '\n> > +\ttest_must_fail git commit --only -m \"baz\" baz\n> > +'\n> > +\n> > +test_expect_success '--include also fail to commit untracked file' '\n> > +\ttest_must_fail git commit --include -m \"baz\" baz\n> > +'\n>\n> As the latter two depends on the first one's side effect of leaving\n> an untracked 'baz' file in the working tree, I do not know if it is\n> sensible to split these into three tests. An obvious alternative is\n> to have a single test\n>\n> \ttest_expect_success 'pathspec that do not match any tracked path' '\n> \t\techo content >baz &&\n> \t\ttest_must_fail git commit -m baz baz &&\n> \t\ttest_must_fail git commit -o -m baz baz &&\n> \t\ttest_must_fail git commit -i -m baz baz\n> \t'\n>\n> By the way, I do not think presence of 'baz' in the working tree\n> matters in the failures from these tests all that much, as the\n> reason they fail is because the pathspec does not match any tracked\n> file, whose contents in the index to be updated before made into a\n> commit.\n\nYes, that is true. However, as per your prior email in which you stated\nabout --include:\n\n \"Now which behaviour between \"error out because there is no path in\n the index that matches pathspec 'baz'\" and \"add baz to the index and\n commit that addition, together with what is already in the index\" we\n would want to take is probably open for discussion. Such a discussion\n may decide that the current behaviour is fine. Until then...\"\n\nIf in such a discussion it is decided that -i should add to index and\ncommit, then by not having 'baz' in the working tree, the -i test\nwould still error out regardless of whether its behaviour is to [add\nto the index and commit] or [error out]. Therefore, by having 'baz'\nwe can detect the change between [-i adds to index and commits] or\n[errors out].\n\n> Likewise. An obvious thing to notice is that this cannot use the\n> same \"contents\" text as before, even though it claims to be \"same as\n> above\". If the final contents of \"file\" and \"baz\" does not matter,\n> but it matters more that these files have been changed, it often is\n> a good idea to append to the file. That way, you can ensure that\n> you will be making them different, no matter what the initial\n> condition was, i.e.,\n>\n> \tfor opt in \"\" \"-o\" \"--only\"\n> \tdo\n> \t\ttest_expect_success 'skip over already added change' '\n> \t\t\techo more >>file &&\n> \t\t\techo more >>baz &&\n> \t\t\tgit add baz &&\n> \t\t\tgit commit $opt -m \"file\" file &&\n>\n> \t\t\t... ensure that changes to file are committed\n> \t\t\t... and changes to baz is only in the index\n> \t\t'\n> \tdone\n>\n> let's you test all three combinations.\n\nYeah, that is a more effective approach. I will change it and reroll\nquickly.\n>\n> Thanks.\n\nThank you for all the help!\n"},{"id":"486750","messageId":"xmqqmsta58ya.fsf@gitster.g","threadId":"60703","inReplyTo":"20240112180109.59350-2-shyamthakkar001@gmail.com","subject":"Re: [PATCH v4 1/2] t7501: add tests for --include and --only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-13T01:16:29Z","receivedAt":"2024-01-13T01:16:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> +test_expect_success '-i/--include includes staged changes' '\n> +\techo newcontent >file &&\n> +\techo newcontent >baz &&\n> +\tgit add file &&\n> +\tgit commit --include -m \"file baz\" baz &&\n\nI may have said this already, but the command invocation that does\nnot result in an error smells like a bug, and I doubt that we want\nto etch the current behaviour into stone, which may make it harder\nto fix [*].\n\nAnother related behaviour that I suspect is a bug is that if you did\n\n git add -u baz\n\ninstead of this \"git commit -i baz\", I think the command will\nsilently succeed without doing anything. They may be the same bug,\nbecause \"git commit -i \" is an equivalent to \"git add -u\n\" followed by \"git commit\" after all. Both should say\n\"there is no such path to update that matches the pathspec \"\nand error out, I suspect.\n\nThanks.\n\n[Footnote]\n\n * A reasonable way out to unblock this particular patch may be to\n clarify that this test is only documenting the current behaviour\n without necessarily endorsing it. Perhaps\n\n\techo more >>file &&\n\techo more >>baz &&\n\tgit add file &&\n\n\t# Note: \"git commit -i baz\" is like \"git add -u baz\"\n\t# followed by \"git commit\" but because baz is untracked,\n\t# only \"file\" is committed.\n\t# This test only documents this current behaviour, which we\n\t# may want to fix, and when it happens, this needs to be\n\t# adjusted to the new behaviour.\n\tgit commit -i -m \"file and baz\" baz &&\n\n or something, probably.\n"},{"id":"486753","messageId":"CYD77D4H2KDG.R1QV6H1T89SN@gmail.com","threadId":"60703","inReplyTo":"xmqqmsta58ya.fsf@gitster.g","subject":"Re: [PATCH v4 1/2] t7501: add tests for --include and --only","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-13T01:47:17Z","receivedAt":"2024-01-13T01:47:22Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Sat Jan 13, 2024 at 6:46 AM IST, Junio C Hamano wrote:\n> Ghanshyam Thakkar writes:\n>\n> > +test_expect_success '-i/--include includes staged changes' '\n> > +\techo newcontent >file &&\n> > +\techo newcontent >baz &&\n> > +\tgit add file &&\n> > +\tgit commit --include -m \"file baz\" baz &&\n>\n> I may have said this already, but the command invocation that does\n> not result in an error smells like a bug, and I doubt that we want\n> to etch the current behaviour into stone, which may make it harder\n> to fix [*].\n\nYeah, that is why baz is added in the index in the test before the one\nyou quoted. And as I understand it, when everything is in the index, it\nworks as intended. Therefore to not get in the way of amending this\nbehaviour, no untracked files are being (attempted to be) committed in\nthese tests (except 'fail to commit untracked files' test, but that is\nnot what you quoted above).\n\n> Another related behaviour that I suspect is a bug is that if you did\n>\n> git add -u baz\n>\n> instead of this \"git commit -i baz\", I think the command will\n> silently succeed without doing anything. They may be the same bug,\n> because \"git commit -i \" is an equivalent to \"git add -u\n> \" followed by \"git commit\" after all. Both should say\n> \"there is no such path to update that matches the pathspec \"\n> and error out, I suspect.\n\nYeah, just checked, that also succeeds silently.\n\n> Thanks.\n>\n> [Footnote]\n>\n> * A reasonable way out to unblock this particular patch may be to\n> clarify that this test is only documenting the current behaviour\n> without necessarily endorsing it. Perhaps\n>\n> \techo more >>file &&\n> \techo more >>baz &&\n> \tgit add file &&\n>\n> \t# Note: \"git commit -i baz\" is like \"git add -u baz\"\n> \t# followed by \"git commit\" but because baz is untracked,\n> \t# only \"file\" is committed.\n> \t# This test only documents this current behaviour, which we\n> \t# may want to fix, and when it happens, this needs to be\n> \t# adjusted to the new behaviour.\n> \tgit commit -i -m \"file and baz\" baz &&\n>\n> or something, probably.\n\nas stated above, baz is tracked from the previous test. In any case,\nI will add a note just to document that untracked files also do not\ngive any error (when there are staged changes) but are not committed.\n\nThanks.\n"},{"id":"486755","messageId":"20240113042254.38602-1-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240112180109.59350-1-shyamthakkar001@gmail.com","subject":"[PATCH v5 0/2] t7501: add tests for --include, --only, --signoff","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-13T04:21:53Z","receivedAt":"2024-01-13T04:23:18Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"The v5 of this series updates the '--only' test to use the for loop\nand >> (append) operator to test all three cases (-o/--only/none) via a\nsingle test.\n\nThis version also documents a bug found via the previous iteration of\nthis series, which does not report error when --include is used with\ninvalid pathspec.\n\nSignoff tests remain unchanged.\n\nGhanshyam Thakkar (2):\n t7501: add tests for --include and --only\n t7501: add tests for --amend --signoff\n\n t/t7501-commit-basic-functionality.sh | 100 +++++++++++++++++++++++++-\n 1 file changed, 98 insertions(+), 2 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"486756","messageId":"20240113042254.38602-2-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240113042254.38602-1-shyamthakkar001@gmail.com","subject":"[PATCH v5 1/2] t7501: add tests for --include and --only","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-13T04:21:54Z","receivedAt":"2024-01-13T04:23:28Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Add tests for --only (-o) and --include (-i). This include testing\nwith or without staged changes for both -i and -o. Also to test\nfor committing untracked files with -i, -o and without -i/-o.\n\nSome tests already exist in t7501 for testing --only, however,\nit is tested in combination with --amend and --allow-empty and on\nto-be-born branch. The addition of these tests check, when the\npathspec is provided, that only the files matching the pathspec\nget committed. This behavior is same when we provide --only and\nchecked by the tests.\n(as --only is the default mode of operation when pathspec is\nprovided.)\n\nAs for --include, there is no prior test for checking if --include\nalso commits staged changes, thus add test for that. Along with \nthe test also document a potential bug, in which, when provided\nwith -i and an untracked pathspec, commit does not fail if there\nare staged changes. And when there are no staged changes commit\nfails. However, no error is returned to stderr in either of the\ncases.\n\nThus, these tests belong in t7501 with other similar existing tests,\nas described in the case of --only.\n\nAnd also add a test for checking incompatibilty when using -o and\n-i together.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n t/t7501-commit-basic-functionality.sh | 77 ++++++++++++++++++++++++++-\n 1 file changed, 76 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex 3d8500a52e..3e18b879b5 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -3,7 +3,7 @@\n # Copyright (c) 2007 Kristian Høgsberg \n #\n \n-# FIXME: Test the various index usages, -i and -o, test reflog,\n+# FIXME: Test the various index usages, test reflog,\n # signoff\n \n test_description='git commit'\n@@ -92,6 +92,32 @@ test_expect_success '--long fails with nothing to commit' '\n \ttest_must_fail git commit -m initial --long\n '\n \n+test_expect_success 'fail to commit untracked pathspec (even with --include/--only)' '\n+\techo content >baz &&\n+\terror=\"error: pathspec .baz. did not match any file(s) known to git\" &&\n+\t\n+\ttest_must_fail git commit -m \"baz\" baz 2>err &&\n+\ttest_grep -e \"$error\" err &&\n+\n+\ttest_must_fail git commit --only -m \"baz\" baz 2>err &&\n+\ttest_grep -e \"$error\" err &&\n+\n+\t# TODO: as for --include, the below command will fail because nothing is\n+\t# staged. If something was staged, it would not fail even if the\n+\t# pathspec was untracked (however, pathspec will remain untracked and\n+\t# staged changes would be committed.) In either cases, no error is\n+\t# returned to stderr like in (-o and without -o/-i) cases. In a\n+\t# similar manner, \"git add -u baz\" also does not error out.\n+\t# \n+\t# Therefore, the below test is just to document the current behavior\n+\t# and is not an endorsement to the current behavior, and we may \n+\t# want to fix this. And when that happens, this test should be\n+\t# updated accordingly.\n+\n+\ttest_must_fail git commit --include -m \"baz\" baz 2>err &&\n+\ttest_must_be_empty err\n+'\n+\n test_expect_success 'setup: non-initial commit' '\n \techo bongo bongo bongo >file &&\n \tgit commit -m next -a\n@@ -117,6 +143,55 @@ test_expect_success '--long with stuff to commit returns ok' '\n \tgit commit -m next -a --long\n '\n \n+for opt in \"\" \"-o\" \"--only\"\n+do \n+\ttest_expect_success 'exclude additional staged changes when given pathspec' '\n+\t\techo content >>file &&\n+\t\techo content >>baz &&\n+\t\tgit add baz &&\n+\t\tgit commit $opt -m \"file\" file &&\n+\n+\t\tgit diff --name-only >actual &&\n+\t\ttest_must_be_empty actual &&\n+\n+\t\tgit diff --name-only --staged >actual &&\n+\t\ttest_cmp - actual <<-EOF &&\n+\t\tbaz\n+\t\tEOF\n+\n+\t\tgit diff --name-only HEAD^ HEAD >actual &&\n+\t\ttest_cmp - actual <<-EOF\n+\t\tfile\n+\t\tEOF\n+\t'\n+done\n+\n+test_expect_success '-i/--include includes staged changes' '\n+\techo content >>file &&\n+\techo content >>baz &&\n+\tgit add file &&\n+\t\n+\t# baz is in the index, therefore, it will be committed\n+\tgit commit --include -m \"file and baz\" baz &&\n+\n+\tgit diff --name-only HEAD >remaining &&\n+\ttest_must_be_empty remaining &&\n+\n+\tgit diff --name-only HEAD^ HEAD >changes &&\n+\ttest_cmp - changes <<-EOF\n+\tbaz\n+\tfile\n+\tEOF\n+'\n+\n+test_expect_success '--include and --only do not mix' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo content >>file &&\n+\techo content >>baz &&\n+\ttest_must_fail git commit --include --only -m \"file baz\" file baz 2>actual &&\n+\ttest_grep -e \"fatal: options .-i/--include. and .-o/--only. cannot be used together\" actual\n+'\n+\n test_expect_success 'commit message from non-existing file' '\n \techo more bongo: bongo bongo bongo bongo >file &&\n \ttest_must_fail git commit -F gah -a\n-- \n2.43.0\n\n"},{"id":"486757","messageId":"20240113042254.38602-3-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240113042254.38602-1-shyamthakkar001@gmail.com","subject":"[PATCH v5 2/2] t7501: add tests for --amend --signoff","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-13T04:21:55Z","receivedAt":"2024-01-13T04:23:31Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Add tests for amending the commit to add Signed-off-by trailer. And\nalso to check if it does not add another trailer if one already exists.\n\nCurrently, there are tests for --signoff separately in t7501, however,\nthey are not tested with --amend.\n\nTherefore, these tests belong with other similar tests of --amend in\nt7501-commit-basic-functionality.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n t/t7501-commit-basic-functionality.sh | 25 +++++++++++++++++++++++--\n 1 file changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex 3e18b879b5..db37e9051b 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -3,8 +3,7 @@\n # Copyright (c) 2007 Kristian Høgsberg \n #\n \n-# FIXME: Test the various index usages, test reflog,\n-# signoff\n+# FIXME: Test the various index usages, test reflog\n \n test_description='git commit'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n@@ -464,6 +463,28 @@ test_expect_success 'amend commit to fix date' '\n \n '\n \n+test_expect_success 'amend commit to add signoff' '\n+\n+\ttest_commit \"msg\" file content &&\n+\tgit commit --amend --signoff &&\n+\ttest_commit_message HEAD <<-EOF\n+\tmsg\n+\n+\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+'\n+\n+test_expect_success 'amend does not add signoff if it already exists' '\n+\n+\ttest_commit --signoff \"tenor\" file newcontent &&\n+\tgit commit --amend --signoff &&\n+\ttest_commit_message HEAD <<-EOF\n+\ttenor\n+\n+\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+'\n+\n test_expect_success 'commit mentions forced date in output' '\n \tgit commit --amend --date=2010-01-02T03:04:05 >output &&\n \tgrep \"Date: *Sat Jan 2 03:04:05 2010\" output\n-- \n2.43.0\n\n"},{"id":"486846","messageId":"xmqqle8p475s.fsf@gitster.g","threadId":"60703","inReplyTo":"20240113042254.38602-2-shyamthakkar001@gmail.com","subject":"Re: [PATCH v5 1/2] t7501: add tests for --include and --only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-16T15:41:51Z","receivedAt":"2024-01-16T15:41:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n\n\"git am\" auto-fixed these whitespace breakage.\n\n.git/rebase-apply/patch:25: trailing whitespace.\n\t\n.git/rebase-apply/patch:38: trailing whitespace.\n\t# \n.git/rebase-apply/patch:40: trailing whitespace.\n\t# and is not an endorsement to the current behavior, and we may \n.git/rebase-apply/patch:56: trailing whitespace.\ndo \n.git/rebase-apply/patch:82: trailing whitespace.\n\t\nwarning: 5 lines applied after fixing whitespace errors.\nApplying: t7501: add tests for --include and --only\n\n"},{"id":"486847","messageId":"xmqq1qah46i0.fsf@gitster.g","threadId":"60703","inReplyTo":"20240113042254.38602-2-shyamthakkar001@gmail.com","subject":"Re: [PATCH v5 1/2] t7501: add tests for --include and --only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-16T15:56:07Z","receivedAt":"2024-01-16T15:56:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> +test_expect_success 'fail to commit untracked pathspec (even with --include/--only)' '\n\n\"untracked pathspec\" is a nonsense word. We do not track pathspec;\nyou may use pathspec to specify which path(s) to work on. I think\nthis is more about untracked files being ignored for the purpose of\n\"-i\" and \"-o\".\n\nYou used to call this \"untracked file\", which probably makes more\nsense.\n\n> +\t# TODO: as for --include, the below command will fail because nothing is\n> +\t# staged. If something was staged, it would not fail even if the\n> +\t# pathspec was untracked (however, pathspec will remain untracked and\n> +\t# staged changes would be committed.) In either cases, no error is\n\nBoth uses of \"pathspec\" here are nonsense. Saying \"even though the\npathspec does not match any tracked path\" is OK. \"(however, the\nuntracked path that match the pathspec are not added and only the\nchanges in the index gets commit)\" is also OK.\n\n> +\t# returned to stderr like in (-o and without -o/-i) cases. In a\n> +\t# similar manner, \"git add -u baz\" also does not error out.\n> +\t# \n> +\t# Therefore, the below test is just to document the current behavior\n> +\t# and is not an endorsement to the current behavior, and we may \n> +\t# want to fix this. And when that happens, this test should be\n> +\t# updated accordingly.\n\nOK.\n\n> @@ -117,6 +143,55 @@ test_expect_success '--long with stuff to commit returns ok' '\n> \tgit commit -m next -a --long\n> '\n> \n> +for opt in \"\" \"-o\" \"--only\"\n> +do \n> +\ttest_expect_success 'exclude additional staged changes when given pathspec' '\n> +\t\techo content >>file &&\n> +\t\techo content >>baz &&\n> +\t\tgit add baz &&\n> +\t\tgit commit $opt -m \"file\" file &&\n> +\n> +\t\tgit diff --name-only >actual &&\n> +\t\ttest_must_be_empty actual &&\n> +\n> +\t\tgit diff --name-only --staged >actual &&\n> +\t\ttest_cmp - actual <<-EOF &&\n> +\t\tbaz\n> +\t\tEOF\n\nIt is probably more common to say\n\n\t\ttest_write_lines baz >expect &&\n\t\tgit diff --name-only --cached >actual &&\n\t\ttest_cmp expect actual\n\nespecially when the expected pattern is a file with short lines,\ninstead of here-text. It makes it easier to debug tests, too,\nbecause after \"sh t7501-*.sh -i\" fails, you can go to the trash\ndirectory and the expectation as well as the actual output are\nthere in files.\n\n> +\t\tgit diff --name-only HEAD^ HEAD >actual &&\n> +\t\ttest_cmp - actual <<-EOF\n> +\t\tfile\n> +\t\tEOF\n\nDitto.\n\nThis tests the most important aspect of \"-o\"; even when the index\nhas other changes relative to HEAD already, they are skipped over\nand only the changes to paths that match the given pathspec are\ncommitted, and this test demonstrates it. Well done.\n\n> +test_expect_success '-i/--include includes staged changes' '\n> +\techo content >>file &&\n> +\techo content >>baz &&\n> +\tgit add file &&\n> +\t\n> +\t# baz is in the index, therefore, it will be committed\n> +\tgit commit --include -m \"file and baz\" baz &&\n> +\n> +\tgit diff --name-only HEAD >remaining &&\n> +\ttest_must_be_empty remaining &&\n> +\n> +\tgit diff --name-only HEAD^ HEAD >changes &&\n> +\ttest_cmp - changes <<-EOF\n> +\tbaz\n> +\tfile\n> +\tEOF\n\nAgain with \"test_write_lines baz file >expect\", this test becomes a\nbit shorter.\n\nThis tests the most important aspect of \"-i\"; changes to the paths\nthat match the given pathspec are added to the index, and recorded\ntogether with changes added to the index already to the commit.\nWell done.\n\n> +'\n> +\n> +test_expect_success '--include and --only do not mix' '\n> +\ttest_when_finished \"git reset --hard\" &&\n> +\techo content >>file &&\n> +\techo content >>baz &&\n> +\ttest_must_fail git commit --include --only -m \"file baz\" file baz 2>actual &&\n> +\ttest_grep -e \"fatal: options .-i/--include. and .-o/--only. cannot be used together\" actual\n> +'\n> +\n> test_expect_success 'commit message from non-existing file' '\n> \techo more bongo: bongo bongo bongo bongo >file &&\n> \ttest_must_fail git commit -F gah -a\n\nVery nicely done.\n\nThanks.\n"},{"id":"486917","messageId":"20240117161421.17333-1-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240113042254.38602-1-shyamthakkar001@gmail.com","subject":"[PATCH v6 0/2] t7501: add tests for --include, --only, --signoff","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-17T16:13:53Z","receivedAt":"2024-01-17T16:15:32Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"The v6 addresses the comments from Junio, which suggested to improve the\nTODO comment describing the potential bug for --include. Also, replace\nhere-doc with expected output file for better debugging, as suggested by\nJunio.\n\n--signoff tests remain unchanged.\n\nGhanshyam Thakkar (2):\n t7501: add tests for --include and --only\n t7501: add tests for --amend --signoff\n\n t/t7501-commit-basic-functionality.sh | 98 ++++++++++++++++++++++++++-\n 1 file changed, 96 insertions(+), 2 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"486918","messageId":"20240117161421.17333-2-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240117161421.17333-1-shyamthakkar001@gmail.com","subject":"[PATCH v6 1/2] t7501: add tests for --include and --only","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-17T16:13:54Z","receivedAt":"2024-01-17T16:15:37Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Add tests for --only (-o) and --include (-i). This include testing\nwith or without staged changes for both -i and -o. Also to test\nfor committing untracked files with -i, -o and without -i/-o.\n\nSome tests already exist in t7501 for testing --only, however,\nit is only tested in combination with --amend and --allow-empty\nand on to-be-born branch. The addition of these tests check, when\nthe pathspec is provided without using -only, that only the files\nmatching the pathspec get committed. This behavior is same when\nwe provide --only and it is checked by the tests.\n(as --only is the default mode of operation when pathspec is\nprovided.)\n\nAs for --include, there is no prior test for checking if --include\nalso commits staged changes, thus add test for that. Along with\nthe tests also document a potential bug, in which, when provided\nwith -i and a pathspec that does not match any tracked path,\ncommit does not fail if there are staged changes. And when there\nare no staged changes commit fails. However, no error is returned\nto stderr in either of the cases. This is described in the TODO\ncomment before the relevent testcase.\n\nAnd also add a test for checking incompatibilty when using -o and\n-i together.\n\nThus, these tests belong in t7501 with other similar existing tests,\nas described in the case of --only.\n\nHelped-by: Junio C Hamano \nHelped-by: Christian Couder \nSigned-off-by: Ghanshyam Thakkar \n---\n t/t7501-commit-basic-functionality.sh | 75 ++++++++++++++++++++++++++-\n 1 file changed, 74 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex 3d8500a52e..c169f10458 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -3,7 +3,7 @@\n # Copyright (c) 2007 Kristian Høgsberg \n #\n \n-# FIXME: Test the various index usages, -i and -o, test reflog,\n+# FIXME: Test the various index usages, test reflog,\n # signoff\n \n test_description='git commit'\n@@ -92,6 +92,34 @@ test_expect_success '--long fails with nothing to commit' '\n \ttest_must_fail git commit -m initial --long\n '\n \n+test_expect_success 'fail to commit untracked file (even with --include/--only)' '\n+\techo content >baz &&\n+\terror=\"error: pathspec .baz. did not match any file(s) known to git\" &&\n+\n+\ttest_must_fail git commit -m \"baz\" baz 2>err &&\n+\ttest_grep -e \"$error\" err &&\n+\n+\ttest_must_fail git commit --only -m \"baz\" baz 2>err &&\n+\ttest_grep -e \"$error\" err &&\n+\n+\t# TODO: as for --include, the below command will fail because\n+\t# nothing is staged. If something was staged, it would not fail\n+\t# even though the provided pathspec does not match any tracked\n+\t# path. (However, the untracked paths that match the pathspec are\n+\t# not committed and only the staged changes get committed.)\n+\t# In either cases, no error is returned to stderr like in (--only\n+\t# and without --only/--include) cases. In a similar manner,\n+\t# \"git add -u baz\" also does not error out.\n+\t#\n+\t# Therefore, the below test is just to document the current behavior\n+\t# and is not an endorsement to the current behavior, and we may\n+\t# want to fix this. And when that happens, this test should be\n+\t# updated accordingly.\n+\n+\ttest_must_fail git commit --include -m \"baz\" baz 2>err &&\n+\ttest_must_be_empty err\n+'\n+\n test_expect_success 'setup: non-initial commit' '\n \techo bongo bongo bongo >file &&\n \tgit commit -m next -a\n@@ -117,6 +145,51 @@ test_expect_success '--long with stuff to commit returns ok' '\n \tgit commit -m next -a --long\n '\n \n+for opt in \"\" \"-o\" \"--only\"\n+do\n+\ttest_expect_success 'exclude additional staged changes when given pathspec' '\n+\t\techo content >>file &&\n+\t\techo content >>baz &&\n+\t\tgit add baz &&\n+\t\tgit commit $opt -m \"file\" file &&\n+\n+\t\tgit diff --name-only >actual &&\n+\t\ttest_must_be_empty actual &&\n+\n+\t\ttest_write_lines baz >expect &&\n+\t\tgit diff --name-only --cached >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\ttest_write_lines file >expect &&\n+\t\tgit diff --name-only HEAD^ HEAD >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n+test_expect_success '-i/--include includes staged changes' '\n+\techo content >>file &&\n+\techo content >>baz &&\n+\tgit add file &&\n+\n+\t# baz is in the index, therefore, it will be committed\n+\tgit commit --include -m \"file and baz\" baz &&\n+\n+\tgit diff --name-only HEAD >remaining &&\n+\ttest_must_be_empty remaining &&\n+\n+\ttest_write_lines baz file >expect &&\n+\tgit diff --name-only HEAD^ HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--include and --only do not mix' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\techo content >>file &&\n+\techo content >>baz &&\n+\ttest_must_fail git commit --include --only -m \"file baz\" file baz 2>actual &&\n+\ttest_grep -e \"fatal: options .-i/--include. and .-o/--only. cannot be used together\" actual\n+'\n+\n test_expect_success 'commit message from non-existing file' '\n \techo more bongo: bongo bongo bongo bongo >file &&\n \ttest_must_fail git commit -F gah -a\n-- \n2.43.0\n\n"},{"id":"486919","messageId":"20240117161421.17333-3-shyamthakkar001@gmail.com","threadId":"60703","inReplyTo":"20240117161421.17333-1-shyamthakkar001@gmail.com","subject":"[PATCH v6 2/2] t7501: add tests for --amend --signoff","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-01-17T16:13:55Z","receivedAt":"2024-01-17T16:15:45Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Add tests for amending the commit to add Signed-off-by trailer. And\nalso to check if it does not add another trailer if one already exists.\n\nCurrently, there are tests for --signoff separately in t7501, however,\nthey are not tested with --amend.\n\nTherefore, these tests belong with other similar tests of --amend in\nt7501-commit-basic-functionality.\n\nHelped-by: Phillip Wood \nSigned-off-by: Ghanshyam Thakkar \n---\n t/t7501-commit-basic-functionality.sh | 25 +++++++++++++++++++++++--\n 1 file changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex c169f10458..bced44a0fc 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -3,8 +3,7 @@\n # Copyright (c) 2007 Kristian Høgsberg \n #\n \n-# FIXME: Test the various index usages, test reflog,\n-# signoff\n+# FIXME: Test the various index usages, test reflog\n \n test_description='git commit'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n@@ -462,6 +461,28 @@ test_expect_success 'amend commit to fix date' '\n \n '\n \n+test_expect_success 'amend commit to add signoff' '\n+\n+\ttest_commit \"msg\" file content &&\n+\tgit commit --amend --signoff &&\n+\ttest_commit_message HEAD <<-EOF\n+\tmsg\n+\n+\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+'\n+\n+test_expect_success 'amend does not add signoff if it already exists' '\n+\n+\ttest_commit --signoff \"tenor\" file newcontent &&\n+\tgit commit --amend --signoff &&\n+\ttest_commit_message HEAD <<-EOF\n+\ttenor\n+\n+\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+'\n+\n test_expect_success 'commit mentions forced date in output' '\n \tgit commit --amend --date=2010-01-02T03:04:05 >output &&\n \tgrep \"Date: *Sat Jan 2 03:04:05 2010\" output\n-- \n2.43.0\n\n"},{"id":"486937","messageId":"xmqq34uvwspa.fsf@gitster.g","threadId":"60703","inReplyTo":"20240117161421.17333-1-shyamthakkar001@gmail.com","subject":"Re: [PATCH v6 0/2] t7501: add tests for --include, --only, --signoff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-17T21:33:37Z","receivedAt":"2024-01-17T21:33:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> The v6 addresses the comments from Junio, which suggested to improve the\n> TODO comment describing the potential bug for --include. Also, replace\n> here-doc with expected output file for better debugging, as suggested by\n> Junio.\n>\n> --signoff tests remain unchanged.\n>\n> Ghanshyam Thakkar (2):\n> t7501: add tests for --include and --only\n> t7501: add tests for --amend --signoff\n>\n> t/t7501-commit-basic-functionality.sh | 98 ++++++++++++++++++++++++++-\n> 1 file changed, 96 insertions(+), 2 deletions(-)\n\nLooks quite good. Let's declare victory and merge it down to 'next'\nsoonish.\n\nThanks.\n"}]}