{"thread":{"id":"61146","subject":"[PATCH 0/2] fix certain cases of add and commit with untracked path not erroring out","startedAt":"2024-03-18T15:54:20Z","lastAt":"2024-04-03T18:19:59Z","messageCount":31,"participants":["Ghanshyam Thakkar","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"490876","messageId":"20240318155219.494206-2-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":null,"subject":"[PATCH 0/2] fix certain cases of add and commit with untracked path not erroring out","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-03-18T15:51:57Z","receivedAt":"2024-03-18T15:54:20Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"While adding tests for 'commit --include', I accidentally reproduced a\npotential bug where in we do not error out when passing a pathspec which\ndoes not match any tracked path. This was noticed by Junio while\nreviewing[1]. This patch series fixes that and a similar case in 'add\n--update'.\n\n[1]: https://lore.kernel.org/git/xmqqil41avcc.fsf@gitster.g/\n\nGhanshyam Thakkar (2):\n builtin/commit: error out when passing untracked path with -i\n builtin/add: error out when passing untracked path with -u\n\n builtin/add.c | 16 ++++++++++++++++\n builtin/commit.c | 15 +++++++++++++++\n t/t1092-sparse-checkout-compatibility.sh | 4 ----\n t/t2200-add-update.sh | 5 +++++\n t/t7501-commit-basic-functionality.sh | 16 +---------------\n 5 files changed, 37 insertions(+), 19 deletions(-)\n\n-- \n2.44.0\n\n"},{"id":"490877","messageId":"20240318155219.494206-4-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":"20240318155219.494206-2-shyamthakkar001@gmail.com","subject":"[PATCH 1/2] builtin/commit: error out when passing untracked path with -i","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-03-18T15:51:59Z","receivedAt":"2024-03-18T15:54:32Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Currently when we provide a pathspec which does not match any tracked\npath alongside --include, we do not error like without --include. If\nthere is something staged, it will commit the staged changes and ignore\nthe pathspec which does not match any tracked path. And if nothing is\nstaged, it will print the status. Exit code is 0 in both cases (unlike\nwithout --include). This was also described in the TODO comment before\nthe relevant testcase.\n\nFix this by matching the pathspec against index and report error if\nany. And amend the relevant testcase and remove the TODO comment.\nAs this matches the pathspec against index, we need to also make sure\nthat the sparse index is expanded before matching the pathspec. A\nside-effect of this is removal of --include related lines from the\ntestcase which checks if the sparse-index is expanded or not in t1092.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n[RFC]: I am still unsure about the removal of --include related lines\nfrom the testcase which checks whether the index is expanded or not from\nt1092. Will separating it into a separate testcase of its own and\nmarking that to expect failure be better?\n\n builtin/commit.c | 15 +++++++++++++++\n t/t1092-sparse-checkout-compatibility.sh | 4 ----\n t/t7501-commit-basic-functionality.sh | 16 +---------------\n 3 files changed, 16 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex a91197245f..f8f5909673 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -441,6 +441,21 @@ static const char *prepare_index(const char **argv, const char *prefix,\n \t * (B) on failure, rollback the real index.\n \t */\n \tif (all || (also && pathspec.nr)) {\n+\t\tif (!all) {\n+\t\t\tint i, ret;\n+\t\t\tchar *ps_matched = xcalloc(pathspec.nr, 1);\n+\n+\t\t\t/* TODO: audit for interaction with sparse-index. */\n+\t\t\tensure_full_index(&the_index);\n+\t\t\tfor (i = 0; i < the_index.cache_nr; i++)\n+\t\t\t\tce_path_match(&the_index, the_index.cache[i],\n+\t\t\t\t\t &pathspec, ps_matched);\n+\n+\t\t\tret = report_path_error(ps_matched, &pathspec);\n+\t\t\tfree(ps_matched);\n+\t\t\tif (ret)\n+\t\t\t\texit(1);\n+\t\t}\n \t\trepo_hold_locked_index(the_repository, &index_lock,\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \t\tadd_files_to_cache(the_repository, also ? prefix : NULL,\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 2f1ae5fd3b..b55c81d4f7 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1418,10 +1418,6 @@ test_expect_success 'sparse-index is not expanded' '\n \tensure_not_expanded commit --allow-empty -m empty &&\n \techo >>sparse-index/a &&\n \tensure_not_expanded commit -a -m a &&\n-\techo >>sparse-index/a &&\n-\tensure_not_expanded commit --include a -m a &&\n-\techo >>sparse-index/deep/deeper1/a &&\n-\tensure_not_expanded commit --include deep/deeper1/a -m deeper &&\n \tensure_not_expanded checkout rename-out-to-out &&\n \tensure_not_expanded checkout - &&\n \tensure_not_expanded switch rename-out-to-out &&\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex bced44a0fc..cc12f99f11 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -101,22 +101,8 @@ test_expect_success 'fail to commit untracked file (even with --include/--only)'\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+\ttest_grep -e \"$error\" err\n '\n \n test_expect_success 'setup: non-initial commit' '\n-- \n2.44.0\n\n"},{"id":"490878","messageId":"20240318155219.494206-6-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":"20240318155219.494206-2-shyamthakkar001@gmail.com","subject":"[PATCH 2/2] builtin/add: error out when passing untracked path with -u","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-03-18T15:52:01Z","receivedAt":"2024-03-18T15:54:42Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Currently when we pass a pathspec which does not match any tracked path\nalong side --update, it silently succeeds, unlike without --update. As\n--update only touches known paths, match the pathspec against the index\nand error out when no match found. And ensure that the index is fully\nexpanded before matching the pathspec. Also add a testcase to check\nfor the error.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n builtin/add.c | 16 ++++++++++++++++\n t/t2200-add-update.sh | 5 +++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 393c10cbcf..7ec5ea4a3e 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -24,6 +24,7 @@\n #include \"strvec.h\"\n #include \"submodule.h\"\n #include \"add-interactive.h\"\n+#include \"sparse-index.h\"\n \n static const char * const builtin_add_usage[] = {\n \tN_(\"git add [] [--] ...\"),\n@@ -536,6 +537,21 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t}\n \n+\t\tif (take_worktree_changes && pathspec.nr) {\n+\t\t\tint i, ret;\n+\t\t\tchar *ps_matched = xcalloc(pathspec.nr, 1);\n+\n+\t\t\t/* TODO: audit for interaction with sparse-index. */\n+\t\t\tensure_full_index(&the_index);\n+\t\t\tfor (i = 0; i < the_index.cache_nr; i++)\n+\t\t\t\tce_path_match(&the_index, the_index.cache[i],\n+\t\t\t\t\t &pathspec, ps_matched);\n+\n+\t\t\tret = report_path_error(ps_matched, &pathspec);\n+\t\t\tfree(ps_matched);\n+\t\t\tif (ret)\n+\t\t\t\texit(1);\n+\t\t}\n \n \t\tif (only_match_skip_worktree.nr) {\n \t\t\tadvise_on_updating_sparse_paths(&only_match_skip_worktree);\ndiff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\nindex c01492f33f..f6a9615d1b 100755\n--- a/t/t2200-add-update.sh\n+++ b/t/t2200-add-update.sh\n@@ -65,6 +65,11 @@ test_expect_success 'update did not touch untracked files' '\n \ttest_must_be_empty out\n '\n \n+test_expect_success 'error out when given untracked path' '\n+\ttest_must_fail git add -u dir2/other 2>err &&\n+\ttest_grep -e \"error: pathspec .dir2/other. did not match any file(s) known to git\" err\n+'\n+\n test_expect_success 'cache tree has not been corrupted' '\n \n \tgit ls-files -s |\n-- \n2.44.0\n\n"},{"id":"490890","messageId":"xmqqedc7h2le.fsf@gitster.g","threadId":"61146","inReplyTo":"20240318155219.494206-4-shyamthakkar001@gmail.com","subject":"Re: [PATCH 1/2] builtin/commit: error out when passing untracked path with -i","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-18T17:27:57Z","receivedAt":"2024-03-18T17:28:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> Currently when we provide a pathspec which does not match any tracked\n> path alongside --include, we do not error like without --include. If\n> there is something staged, it will commit the staged changes and ignore\n> the pathspec which does not match any tracked path. And if nothing is\n> staged, it will print the status. Exit code is 0 in both cases (unlike\n> without --include). This was also described in the TODO comment before\n> the relevant testcase.\n\nDrop \"currently\" (cf. https://lore.kernel.org/git/xmqqle6xbep5.fsf@gitster.g/)\n\n> Fix this by matching the pathspec against index and report error if\n> any. And amend the relevant testcase and remove the TODO comment.\n\n> [RFC]: I am still unsure about the removal of --include related lines\n> from the testcase which checks whether the index is expanded or not from\n> t1092. Will separating it into a separate testcase of its own and\n> marking that to expect failure be better?\n>\n> builtin/commit.c | 15 +++++++++++++++\n> t/t1092-sparse-checkout-compatibility.sh | 4 ----\n> t/t7501-commit-basic-functionality.sh | 16 +---------------\n> 3 files changed, 16 insertions(+), 19 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index a91197245f..f8f5909673 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -441,6 +441,21 @@ static const char *prepare_index(const char **argv, const char *prefix,\n> \t * (B) on failure, rollback the real index.\n> \t */\n> \tif (all || (also && pathspec.nr)) {\n> +\t\tif (!all) {\n> +\t\t\tint i, ret;\n> +\t\t\tchar *ps_matched = xcalloc(pathspec.nr, 1);\n> +\n> +\t\t\t/* TODO: audit for interaction with sparse-index. */\n> +\t\t\tensure_full_index(&the_index);\n> +\t\t\tfor (i = 0; i < the_index.cache_nr; i++)\n> +\t\t\t\tce_path_match(&the_index, the_index.cache[i],\n> +\t\t\t\t\t &pathspec, ps_matched);\n> +\n> +\t\t\tret = report_path_error(ps_matched, &pathspec);\n> +\t\t\tfree(ps_matched);\n> +\t\t\tif (ret)\n> +\t\t\t\texit(1);\n> +\t\t}\n> \t\trepo_hold_locked_index(the_repository, &index_lock,\n> \t\t\t\t LOCK_DIE_ON_ERROR);\n> \t\tadd_files_to_cache(the_repository, also ? prefix : NULL,\n\n\"git grep\" for report_path_error() gives me four or five hits but\nthey way all of them populate ps_matched array are different [*],\nso we cannot have a helper function to do so.\n\n Side note: They tend to do \"looping over all the paths, see with\n ce_path_match() if the path matches, and do something with that\n path if it does\". They do not do a separate useless loop that\n is only for checking if all the pathspec elements match, like\n the loop in this patch does.\n\nIn a sense, not making this into a helper function is the right\nthing to do. It would avoid encouraging this anti-pattern of adding\na separate and otherwise useless loop.\n\nWe must already be using pathspec elements to decide to do the\n\"include\" addition among all paths that we know about in some loop\nseparately, no? Isn't that what the call to add_files_to_cache() we\nsee in the post-context doing? Shouldn't that loop (probably the\none in diff-lib.c:run_diff_files(), that calls ce_path_match() for\neach and every path we know about) be the one who needs to learn to\noptionally collect the ps_matched information in addition to what it\nis already doing?\n\nThanks.\n"},{"id":"490891","messageId":"xmqqa5mvh2fi.fsf@gitster.g","threadId":"61146","inReplyTo":"20240318155219.494206-6-shyamthakkar001@gmail.com","subject":"Re: [PATCH 2/2] builtin/add: error out when passing untracked path with -u","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-18T17:31:29Z","receivedAt":"2024-03-18T17:31:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> Currently when we pass a pathspec which does not match any tracked path\n> along side --update, it silently succeeds, unlike without --update. As\n> --update only touches known paths, match the pathspec against the index\n> and error out when no match found. And ensure that the index is fully\n> expanded before matching the pathspec. Also add a testcase to check\n> for the error.\n>\n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> builtin/add.c | 16 ++++++++++++++++\n> t/t2200-add-update.sh | 5 +++++\n> 2 files changed, 21 insertions(+)\n\nExactly the same comment applies here. If we are using pathspec, we\nshould already have a loop that calls ce_path_match() for each and\nevery path we know about, and we should be updating the code to\ncollect \"have we used all pathspec elements?\" information at the\nsame time if it is not doing so already. Let's not make another\nloop that checks what we should already be doing elsewhere.\n\nThanks.\n\n> diff --git a/builtin/add.c b/builtin/add.c\n> index 393c10cbcf..7ec5ea4a3e 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -24,6 +24,7 @@\n> #include \"strvec.h\"\n> #include \"submodule.h\"\n> #include \"add-interactive.h\"\n> +#include \"sparse-index.h\"\n> \n> static const char * const builtin_add_usage[] = {\n> \tN_(\"git add [] [--] ...\"),\n> @@ -536,6 +537,21 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> \t\t\t}\n> \t\t}\n> \n> +\t\tif (take_worktree_changes && pathspec.nr) {\n> +\t\t\tint i, ret;\n> +\t\t\tchar *ps_matched = xcalloc(pathspec.nr, 1);\n> +\n> +\t\t\t/* TODO: audit for interaction with sparse-index. */\n> +\t\t\tensure_full_index(&the_index);\n> +\t\t\tfor (i = 0; i < the_index.cache_nr; i++)\n> +\t\t\t\tce_path_match(&the_index, the_index.cache[i],\n> +\t\t\t\t\t &pathspec, ps_matched);\n> +\n> +\t\t\tret = report_path_error(ps_matched, &pathspec);\n> +\t\t\tfree(ps_matched);\n> +\t\t\tif (ret)\n> +\t\t\t\texit(1);\n> +\t\t}\n> \n> \t\tif (only_match_skip_worktree.nr) {\n> \t\t\tadvise_on_updating_sparse_paths(&only_match_skip_worktree);\n> diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\n> index c01492f33f..f6a9615d1b 100755\n> --- a/t/t2200-add-update.sh\n> +++ b/t/t2200-add-update.sh\n> @@ -65,6 +65,11 @@ test_expect_success 'update did not touch untracked files' '\n> \ttest_must_be_empty out\n> '\n> \n> +test_expect_success 'error out when given untracked path' '\n> +\ttest_must_fail git add -u dir2/other 2>err &&\n> +\ttest_grep -e \"error: pathspec .dir2/other. did not match any file(s) known to git\" err\n> +'\n> +\n> test_expect_success 'cache tree has not been corrupted' '\n> \n> \tgit ls-files -s |\n"},{"id":"491853","messageId":"20240329205649.1483032-2-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":"20240318155219.494206-2-shyamthakkar001@gmail.com","subject":"[PATCH v2 0/3] commit, add: error out when passing untracked path","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-03-29T20:56:18Z","receivedAt":"2024-03-29T21:02:41Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"Fix 'commit -i' and 'add -u', which expect known paths, not erroring\nout when passing untracked paths.\n\nThe first patch introduces a new parameter to add_files_to_cache() and\nrun_diff_files() to optionally collect pathspec matching info for use\nin reporting error. And the second and third patch use this parameter\nto collect pathspec matching info and report error when passing\nuntracked paths to 'git commit -i' and 'git add -u' respectively.\n\nGhanshyam Thakkar (3):\n optionally collect pathspec matching info\n builtin/commit: error out when passing untracked path with -i\n builtin/add: error out when passing untracked path with -u\n\n add-interactive.c | 2 +-\n builtin/add.c | 13 ++++++++++---\n builtin/checkout.c | 3 ++-\n builtin/commit.c | 9 ++++++++-\n builtin/diff-files.c | 2 +-\n builtin/diff.c | 2 +-\n builtin/merge.c | 2 +-\n builtin/stash.c | 2 +-\n builtin/submodule--helper.c | 4 ++--\n diff-lib.c | 5 +++--\n diff.h | 3 ++-\n read-cache-ll.h | 4 ++--\n read-cache.c | 6 +++---\n t/t2200-add-update.sh | 6 ++++++\n t/t7501-commit-basic-functionality.sh | 16 +---------------\n wt-status.c | 6 +++---\n 16 files changed, 47 insertions(+), 38 deletions(-)\n\n-- \n2.44.0\n\n"},{"id":"491854","messageId":"20240329205649.1483032-3-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":"20240318155219.494206-2-shyamthakkar001@gmail.com","subject":"[PATCH v2 1/3] read-cache: optionally collect pathspec matching info","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-03-29T20:56:19Z","receivedAt":"2024-03-29T21:02:44Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"The add_files_to_cache() adds files to the index. And\nadd_files_to_cache() in turn calls run_diff_files() to perform this\noperation. The run_diff_files() uses ce_path_match() to match the\npathspec against cache entries. However, it is called with NULL value\nfor the seen parameter, which collects the pathspec matching\ninformation.\n\nTherefore, introduce a new parameter 'char *ps_matched' to \nadd_files_to_cache() and in turn to run_diff_files(), to feed it to\nce_path_match() to optionally collect the pathspec matching\ninformation. This will be helpful in reporting error in case of an\nuntracked path being passed when the expectation is a known path. Thus,\nthis will be used in the subsequent commits to fix 'commit -i' and 'add\n-u' not erroring out when given untracked paths.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n add-interactive.c | 2 +-\n builtin/add.c | 6 +++---\n builtin/checkout.c | 3 ++-\n builtin/commit.c | 2 +-\n builtin/diff-files.c | 2 +-\n builtin/diff.c | 2 +-\n builtin/merge.c | 2 +-\n builtin/stash.c | 2 +-\n builtin/submodule--helper.c | 4 ++--\n diff-lib.c | 5 +++--\n diff.h | 3 ++-\n read-cache-ll.h | 4 ++--\n read-cache.c | 6 +++---\n wt-status.c | 6 +++---\n 14 files changed, 26 insertions(+), 23 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 6bf87e7ae7..b33260a611 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -572,7 +572,7 @@ static int get_modified_files(struct repository *r,\n \t\t\trun_diff_index(&rev, DIFF_INDEX_CACHED);\n \t\telse {\n \t\t\trev.diffopt.flags.ignore_dirty_submodules = 1;\n-\t\t\trun_diff_files(&rev, 0);\n+\t\t\trun_diff_files(&rev, NULL, 0);\n \t\t}\n \n \t\trelease_revisions(&rev);\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 393c10cbcf..ffe5fd8d44 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -191,7 +191,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \tout = xopen(file, O_CREAT | O_WRONLY | O_TRUNC, 0666);\n \trev.diffopt.file = xfdopen(out, \"w\");\n \trev.diffopt.close_file = 1;\n-\trun_diff_files(&rev, 0);\n+\trun_diff_files(&rev, NULL, 0);\n \n \tif (launch_editor(file, NULL, NULL))\n \t\tdie(_(\"editing patch failed\"));\n@@ -553,8 +553,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\texit_status |= renormalize_tracked_files(&pathspec, flags);\n \telse\n \t\texit_status |= add_files_to_cache(the_repository, prefix,\n-\t\t\t\t\t\t &pathspec, include_sparse,\n-\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t &pathspec, NULL,\n+\t\t\t\t\t\t include_sparse, flags);\n \n \tif (add_new_files)\n \t\texit_status |= add_files(&dir, flags);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 902c97ab23..02bd035081 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -876,7 +876,8 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t * entries in the index.\n \t\t\t */\n \n-\t\t\tadd_files_to_cache(the_repository, NULL, NULL, 0, 0);\n+\t\t\tadd_files_to_cache(the_repository, NULL, NULL, NULL, 0,\n+\t\t\t\t\t 0);\n \t\t\tinit_merge_options(&o, the_repository);\n \t\t\to.verbosity = 0;\n \t\t\twork = write_in_core_index_as_tree(the_repository);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex a91197245f..24efeaca98 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -444,7 +444,7 @@ static const char *prepare_index(const char **argv, const char *prefix,\n \t\trepo_hold_locked_index(the_repository, &index_lock,\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \t\tadd_files_to_cache(the_repository, also ? prefix : NULL,\n-\t\t\t\t &pathspec, 0, 0);\n+\t\t\t\t &pathspec, NULL, 0, 0);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tcache_tree_update(&the_index, WRITE_TREE_SILENT);\n \t\tif (write_locked_index(&the_index, &index_lock, 0))\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 018011f29e..8559aa254c 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -81,7 +81,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \n \tif (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0)\n \t\tdie_errno(\"repo_read_index_preload\");\n-\trun_diff_files(&rev, options);\n+\trun_diff_files(&rev, NULL, options);\n \tresult = diff_result_code(&rev.diffopt);\n \trelease_revisions(&rev);\n \treturn result;\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 6e196e0c7d..3e9b838bdd 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -283,7 +283,7 @@ static void builtin_diff_files(struct rev_info *revs, int argc, const char **arg\n \t\t\t\t 0) < 0) {\n \t\tdie_errno(\"repo_read_index_preload\");\n \t}\n-\trun_diff_files(revs, options);\n+\trun_diff_files(revs, NULL, options);\n }\n \n struct symdiff {\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a0ba1f9815..4b4c1d6a31 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -979,7 +979,7 @@ static int evaluate_result(void)\n \t\tDIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = count_diff_files;\n \trev.diffopt.format_callback_data = &cnt;\n-\trun_diff_files(&rev, 0);\n+\trun_diff_files(&rev, NULL, 0);\n \n \t/*\n \t * Check how many unmerged entries are\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 7fb355bff0..2c00026390 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1121,7 +1121,7 @@ static int check_changes_tracked_files(const struct pathspec *ps)\n \t\tgoto done;\n \t}\n \n-\trun_diff_files(&rev, 0);\n+\trun_diff_files(&rev, NULL, 0);\n \tif (diff_result_code(&rev.diffopt)) {\n \t\tret = 1;\n \t\tgoto done;\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex fda50f2af1..e9047021e0 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -667,7 +667,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \trepo_init_revisions(the_repository, &rev, NULL);\n \trev.abbrev = 0;\n \tsetup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt);\n-\trun_diff_files(&rev, 0);\n+\trun_diff_files(&rev, NULL, 0);\n \n \tif (!diff_result_code(&rev.diffopt)) {\n \t\tprint_status(flags, ' ', path, ce_oid,\n@@ -1141,7 +1141,7 @@ static int compute_summary_module_list(struct object_id *head_oid,\n \tif (diff_cmd == DIFF_INDEX)\n \t\trun_diff_index(&rev, info->cached ? DIFF_INDEX_CACHED : 0);\n \telse\n-\t\trun_diff_files(&rev, 0);\n+\t\trun_diff_files(&rev, NULL, 0);\n \tprepare_submodule_summary(info, &list);\n cleanup:\n \tstrvec_clear(&diff_args);\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 5e8717c774..2dc3864abd 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -101,7 +101,8 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n \treturn changed;\n }\n \n-void run_diff_files(struct rev_info *revs, unsigned int option)\n+void run_diff_files(struct rev_info *revs, char *ps_matched,\n+\t\t unsigned int option)\n {\n \tint entries, i;\n \tint diff_unmerged_stage = revs->max_count;\n@@ -127,7 +128,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tif (diff_can_quit_early(&revs->diffopt))\n \t\t\tbreak;\n \n-\t\tif (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n+\t\tif (!ce_path_match(istate, ce, &revs->prune_data, ps_matched))\n \t\t\tcontinue;\n \n \t\tif (revs->diffopt.prefix &&\ndiff --git a/diff.h b/diff.h\nindex 66bd8aeb29..a01feaa586 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -638,7 +638,8 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);\n #define DIFF_SILENT_ON_REMOVED 01\n /* report racily-clean paths as modified */\n #define DIFF_RACY_IS_MODIFIED 02\n-void run_diff_files(struct rev_info *revs, unsigned int option);\n+void run_diff_files(struct rev_info *revs, char *ps_matched,\n+\t\t unsigned int option);\n \n #define DIFF_INDEX_CACHED 01\n #define DIFF_INDEX_MERGE_BASE 02\ndiff --git a/read-cache-ll.h b/read-cache-ll.h\nindex 2a50a784f0..09414afd04 100644\n--- a/read-cache-ll.h\n+++ b/read-cache-ll.h\n@@ -480,8 +480,8 @@ extern int verify_ce_order;\n int cmp_cache_name_compare(const void *a_, const void *b_);\n \n int add_files_to_cache(struct repository *repo, const char *prefix,\n-\t\t const struct pathspec *pathspec, int include_sparse,\n-\t\t int flags);\n+\t\t const struct pathspec *pathspec, char *ps_matched,\n+\t\t int include_sparse, int flags);\n \n void overlay_tree_on_index(struct index_state *istate,\n \t\t\t const char *tree_name, const char *prefix);\ndiff --git a/read-cache.c b/read-cache.c\nindex f546cf7875..e179444445 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3958,8 +3958,8 @@ static void update_callback(struct diff_queue_struct *q,\n }\n \n int add_files_to_cache(struct repository *repo, const char *prefix,\n-\t\t const struct pathspec *pathspec, int include_sparse,\n-\t\t int flags)\n+\t\t const struct pathspec *pathspec, char *ps_matched,\n+\t\t int include_sparse, int flags)\n {\n \tstruct update_callback_data data;\n \tstruct rev_info rev;\n@@ -3985,7 +3985,7 @@ int add_files_to_cache(struct repository *repo, const char *prefix,\n \t * may not have their own transaction active.\n \t */\n \tbegin_odb_transaction();\n-\trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n+\trun_diff_files(&rev, ps_matched, DIFF_RACY_IS_MODIFIED);\n \tend_odb_transaction();\n \n \trelease_revisions(&rev);\ndiff --git a/wt-status.c b/wt-status.c\nindex 2db4bb3a12..cf6d61e60c 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -629,7 +629,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)\n \trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n \trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n-\trun_diff_files(&rev, 0);\n+\trun_diff_files(&rev, NULL, 0);\n \trelease_revisions(&rev);\n }\n \n@@ -1173,7 +1173,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)\n \t\tsetup_work_tree();\n \t\trev.diffopt.a_prefix = \"i/\";\n \t\trev.diffopt.b_prefix = \"w/\";\n-\t\trun_diff_files(&rev, 0);\n+\t\trun_diff_files(&rev, NULL, 0);\n \t}\n \trelease_revisions(&rev);\n }\n@@ -2594,7 +2594,7 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules)\n \t}\n \trev_info.diffopt.flags.quick = 1;\n \tdiff_setup_done(&rev_info.diffopt);\n-\trun_diff_files(&rev_info, 0);\n+\trun_diff_files(&rev_info, NULL, 0);\n \tresult = diff_result_code(&rev_info.diffopt);\n \trelease_revisions(&rev_info);\n \treturn result;\n-- \n2.44.0\n\n"},{"id":"491855","messageId":"20240329205649.1483032-4-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":"20240318155219.494206-2-shyamthakkar001@gmail.com","subject":"[PATCH v2 2/3] builtin/commit: error out when passing untracked path with -i","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-03-29T20:56:20Z","receivedAt":"2024-03-29T21:02:47Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"When we provide a pathspec which does not match any tracked path\nalongside --include, we do not error like without --include. If there\nis something staged, it will commit the staged changes and ignore the\npathspec which does not match any tracked path. And if nothing is\nstaged, it will print the status. Exit code is 0 in both cases (unlike\nwithout --include). This is also described in the TODO comment before\nthe relevant testcase.\n\nFix this by passing a character array to add_files_to_cache() to\ncollect the pathspec matching information and error out if the given\npath is untracked. Also, amend the testcase to check for the error\nmessage and remove the TODO comment.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n builtin/commit.c | 9 ++++++++-\n t/t7501-commit-basic-functionality.sh | 16 +---------------\n 2 files changed, 9 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 24efeaca98..355f25ec2a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -441,10 +441,17 @@ static const char *prepare_index(const char **argv, const char *prefix,\n \t * (B) on failure, rollback the real index.\n \t */\n \tif (all || (also && pathspec.nr)) {\n+\t\tchar *ps_matched = xcalloc(pathspec.nr, 1);\n \t\trepo_hold_locked_index(the_repository, &index_lock,\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \t\tadd_files_to_cache(the_repository, also ? prefix : NULL,\n-\t\t\t\t &pathspec, NULL, 0, 0);\n+\t\t\t\t &pathspec, ps_matched, 0, 0);\n+\t\tif (!all && report_path_error(ps_matched, &pathspec)) {\n+\t\t\tfree(ps_matched);\n+\t\t\texit(1);\n+\t\t}\n+\t\tfree(ps_matched);\n+\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tcache_tree_update(&the_index, WRITE_TREE_SILENT);\n \t\tif (write_locked_index(&the_index, &index_lock, 0))\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex bced44a0fc..cc12f99f11 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -101,22 +101,8 @@ test_expect_success 'fail to commit untracked file (even with --include/--only)'\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+\ttest_grep -e \"$error\" err\n '\n \n test_expect_success 'setup: non-initial commit' '\n-- \n2.44.0\n\n"},{"id":"491856","messageId":"20240329205649.1483032-5-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":"20240318155219.494206-2-shyamthakkar001@gmail.com","subject":"[PATCH v2 3/3] builtin/add: error out when passing untracked path with -u","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-03-29T20:56:21Z","receivedAt":"2024-03-29T21:02:50Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"When passing untracked path with -u option, it silently succeeds. There\nis no error message and the exit code is zero. This is inconsistent\nwith other instances of git commands where the expected argument is a\nknown path. In those other instances, we error out when the path is\nnot known.\n\nTherefore, fix this by passing a character array to\nadd_files_to_cache() to collect the pathspec matching information and\nreport the error if a pathspec does not match any cache entry. Also add\na testcase to cover this scenario.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n builtin/add.c | 9 ++++++++-\n t/t2200-add-update.sh | 6 ++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex ffe5fd8d44..650432bb13 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -370,6 +370,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tint add_new_files;\n \tint require_pathspec;\n \tchar *seen = NULL;\n+\tchar *ps_matched = NULL;\n \tstruct lock_file lock_file = LOCK_INIT;\n \n \tgit_config(add_config, NULL);\n@@ -547,15 +548,20 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\tstring_list_clear(&only_match_skip_worktree, 0);\n \t}\n \n+\n \tbegin_odb_transaction();\n \n+\tps_matched = xcalloc(pathspec.nr, 1);\n \tif (add_renormalize)\n \t\texit_status |= renormalize_tracked_files(&pathspec, flags);\n \telse\n \t\texit_status |= add_files_to_cache(the_repository, prefix,\n-\t\t\t\t\t\t &pathspec, NULL,\n+\t\t\t\t\t\t &pathspec, ps_matched,\n \t\t\t\t\t\t include_sparse, flags);\n \n+\tif (take_worktree_changes)\n+\t\texit_status |= report_path_error(ps_matched, &pathspec);\n+\n \tif (add_new_files)\n \t\texit_status |= add_files(&dir, flags);\n \n@@ -568,6 +574,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\tdie(_(\"unable to write new index file\"));\n \n+\tfree(ps_matched);\n \tdir_clear(&dir);\n \tclear_pathspec(&pathspec);\n \treturn exit_status;\ndiff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\nindex c01492f33f..7cba325f08 100755\n--- a/t/t2200-add-update.sh\n+++ b/t/t2200-add-update.sh\n@@ -65,6 +65,12 @@ test_expect_success 'update did not touch untracked files' '\n \ttest_must_be_empty out\n '\n \n+test_expect_success 'error out when passing untracked path' '\n+\techo content >baz &&\n+\ttest_must_fail git add -u baz 2>err &&\n+\ttest_grep -e \"error: pathspec .baz. did not match any file(s) known to git\" err\n+'\n+\n test_expect_success 'cache tree has not been corrupted' '\n \n \tgit ls-files -s |\n-- \n2.44.0\n\n"},{"id":"491858","messageId":"xmqqjzlkwwk9.fsf@gitster.g","threadId":"61146","inReplyTo":"20240329205649.1483032-3-shyamthakkar001@gmail.com","subject":"Re: [PATCH v2 1/3] read-cache: optionally collect pathspec matching info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T21:35:34Z","receivedAt":"2024-03-29T21:35:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> The add_files_to_cache() adds files to the index. And\n> add_files_to_cache() in turn calls run_diff_files() to perform this\n> operation. The run_diff_files() uses ce_path_match() to match the\n> pathspec against cache entries. However, it is called with NULL value\n> for the seen parameter, which collects the pathspec matching\n> information.\n\n\", which collects\" -> \", which means we lose\"\n\n> Therefore, introduce a new parameter 'char *ps_matched' to \n\n\"Therefore, introduce\" -> \"Introduce\"\n\n> add_files_to_cache() and in turn to run_diff_files(), to feed it to\n> ce_path_match() to optionally collect the pathspec matching\n> information. This will be helpful in reporting error in case of an\n> untracked path being passed when the expectation is a known path. Thus,\n> this will be used in the subsequent commits to fix 'commit -i' and 'add\n> -u' not erroring out when given untracked paths.\n\nA new parameter to run_diff_files() came as a bit of surprise to me.\n\nWhen I responded to the previous round, I somehow thought that we'd\nadd a new member to the rev structure that points at an optional\n.ps_matched member next to the existing .prune_data member. \n\nThat way, it would hopefully be easy for a future code to see if a\n\"diff\" invocation, not necessarily run_diff_files() that compares\nthe working tree and the index, consumed all the pathspec elements.\nIf such a new .ps_matched member is initialized to NULL, all the\npatch noise we see in this patch will become unnecessary, no?\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 5e8717c774..2dc3864abd 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -101,7 +101,8 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n> \treturn changed;\n> }\n> \n> -void run_diff_files(struct rev_info *revs, unsigned int option)\n> +void run_diff_files(struct rev_info *revs, char *ps_matched,\n> +\t\t unsigned int option)\n> {\n> \tint entries, i;\n> \tint diff_unmerged_stage = revs->max_count;\n> @@ -127,7 +128,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n> \t\tif (diff_can_quit_early(&revs->diffopt))\n> \t\t\tbreak;\n> \n> -\t\tif (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n> +\t\tif (!ce_path_match(istate, ce, &revs->prune_data, ps_matched))\n> \t\t\tcontinue;\n> \n> \t\tif (revs->diffopt.prefix &&\n\nThis may be a non-issue, but after this point we see the beginning\nof another filter to reject paths outside the hierarchy \"--relative\"\nspecifies. It is possible that a pathspec element matches ce->name\nbut the matched cache entry is outside the current area. Shouldn't\nwe then consider that the pathspec element did not match? E.g., in\nour project, what should happen if we did this?\n\n $ echo >>diff.h\n $ cd t\n $ git diff --relative \\*.h\n\nThe command should show nothing. Did the pathspec '*.h' match? From\nthose who know how the machinery works, yes it did before the resulting\npaths were further filtered out, but from the end-user's point of view,\nbecause \"--relative\" limits the diff to the current directory and below,\nand because 't' and below did not have any C header files, wouldn't it\nbe more natural and useful to say the pathspec wasn't used?\n\nThis does not matter right now because we are not planning to add a\nnew \"--error-unmatch\" option to \"git diff\", but when/if we do, it\nstarts to matter. The hunk at least needs a NEEDSWORK comment,\nsummarizing the above.\n\n\t/*\n\t * NEEDSWORK:\n\t * Here we filter with pathspec but the result is further\n\t * filtered out when --relative is in effect. To end-users,\n * a pathspec element that matched only to paths outside the\n * current directory is like not matching anything at all;\n * the handling of ps_matched[] here may become problematic\n\t * if/when we add the \"--error-unmatch\" option to \"git diff\".\n\t */ \n\nA solution to that problem might be just a matter of swapping the\norder of filtering, but it may have performance implications and I'd\nrather not have to worry about it right now in the context of the\ncurrent topic, hence a NEEDSWORK comment without attempting to \"fix\"\nit would be the most preferred approach to such a side issue.\n"},{"id":"491859","messageId":"xmqqcyrcwwfj.fsf@gitster.g","threadId":"61146","inReplyTo":"20240329205649.1483032-4-shyamthakkar001@gmail.com","subject":"Re: [PATCH v2 2/3] builtin/commit: error out when passing untracked path with -i","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T21:38:24Z","receivedAt":"2024-03-29T21:38:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> When we provide a pathspec which does not match any tracked path\n> alongside --include, we do not error like without --include. If there\n> is something staged, it will commit the staged changes and ignore the\n> pathspec which does not match any tracked path. And if nothing is\n> staged, it will print the status. Exit code is 0 in both cases (unlike\n> without --include). This is also described in the TODO comment before\n> the relevant testcase.\n>\n> Fix this by passing a character array to add_files_to_cache() to\n> collect the pathspec matching information and error out if the given\n> path is untracked. Also, amend the testcase to check for the error\n> message and remove the TODO comment.\n>\n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> builtin/commit.c | 9 ++++++++-\n> t/t7501-commit-basic-functionality.sh | 16 +---------------\n> 2 files changed, 9 insertions(+), 16 deletions(-)\n\nNice.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 24efeaca98..355f25ec2a 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -441,10 +441,17 @@ static const char *prepare_index(const char **argv, const char *prefix,\n> \t * (B) on failure, rollback the real index.\n> \t */\n> \tif (all || (also && pathspec.nr)) {\n> +\t\tchar *ps_matched = xcalloc(pathspec.nr, 1);\n> \t\trepo_hold_locked_index(the_repository, &index_lock,\n> \t\t\t\t LOCK_DIE_ON_ERROR);\n> \t\tadd_files_to_cache(the_repository, also ? prefix : NULL,\n> -\t\t\t\t &pathspec, NULL, 0, 0);\n> +\t\t\t\t &pathspec, ps_matched, 0, 0);\n> +\t\tif (!all && report_path_error(ps_matched, &pathspec)) {\n> +\t\t\tfree(ps_matched);\n> +\t\t\texit(1);\n> +\t\t}\n> +\t\tfree(ps_matched);\n> +\n\nLooking simple and very nice.\n\nThis change would not have to be redone even if we decide not to add\na new parameter to run_diff_files() and instead add a new member to\nthe revs structure instead, because it all happens at the level or\nbelow add_files_to_cache().\n"},{"id":"491860","messageId":"xmqqzfugvhnf.fsf@gitster.g","threadId":"61146","inReplyTo":"20240329205649.1483032-5-shyamthakkar001@gmail.com","subject":"Re: [PATCH v2 3/3] builtin/add: error out when passing untracked path with -u","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T21:43:00Z","receivedAt":"2024-03-29T21:43:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> When passing untracked path with -u option, it silently succeeds. There\n> is no error message and the exit code is zero. This is inconsistent\n> with other instances of git commands where the expected argument is a\n> known path. In those other instances, we error out when the path is\n> not known.\n>\n> Therefore, fix this by passing a character array to\n\n\"Therefore, fix\" -> \"Fix\".\n\n> add_files_to_cache() to collect the pathspec matching information and\n> report the error if a pathspec does not match any cache entry. Also add\n> a testcase to cover this scenario.\n>\n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> builtin/add.c | 9 ++++++++-\n> t/t2200-add-update.sh | 6 ++++++\n> 2 files changed, 14 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index ffe5fd8d44..650432bb13 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -370,6 +370,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> \tint add_new_files;\n> \tint require_pathspec;\n> \tchar *seen = NULL;\n> +\tchar *ps_matched = NULL;\n> \tstruct lock_file lock_file = LOCK_INIT;\n> \n> \tgit_config(add_config, NULL);\n> @@ -547,15 +548,20 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> \t\tstring_list_clear(&only_match_skip_worktree, 0);\n> \t}\n> \n> +\n> \tbegin_odb_transaction();\n\nUnnecessary change.\n\n> +\tps_matched = xcalloc(pathspec.nr, 1);\n> \tif (add_renormalize)\n> \t\texit_status |= renormalize_tracked_files(&pathspec, flags);\n> \telse\n> \t\texit_status |= add_files_to_cache(the_repository, prefix,\n> -\t\t\t\t\t\t &pathspec, NULL,\n> +\t\t\t\t\t\t &pathspec, ps_matched,\n> \t\t\t\t\t\t include_sparse, flags);\n> \n> +\tif (take_worktree_changes)\n> +\t\texit_status |= report_path_error(ps_matched, &pathspec);\n\nHmph, are we sure take_worktree_changes is true only when\nadd_renormalize is false?\n\n> \tif (add_new_files)\n> \t\texit_status |= add_files(&dir, flags);\n\nIf report_path_error() detected that the pathspec were faulty,\nshould we still proceed to add new files? This is NOT a rhetorical\nquestion, as I do not know the answer myself. I do not even know\noffhand what add_files_to_cache() above did when pathspec elements\nare not all consumed---if it does not complain and does not refrain\nfrom doing any change to the index, then we should follow suite and\nadd_files() here, too.\n\n> @@ -568,6 +574,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> \t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n> \t\tdie(_(\"unable to write new index file\"));\n> \n> +\tfree(ps_matched);\n> \tdir_clear(&dir);\n> \tclear_pathspec(&pathspec);\n> \treturn exit_status;\n> diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\n> index c01492f33f..7cba325f08 100755\n> --- a/t/t2200-add-update.sh\n> +++ b/t/t2200-add-update.sh\n> @@ -65,6 +65,12 @@ test_expect_success 'update did not touch untracked files' '\n> \ttest_must_be_empty out\n> '\n> \n> +test_expect_success 'error out when passing untracked path' '\n> +\techo content >baz &&\n> +\ttest_must_fail git add -u baz 2>err &&\n> +\ttest_grep -e \"error: pathspec .baz. did not match any file(s) known to git\" err\n> +'\n> +\n> test_expect_success 'cache tree has not been corrupted' '\n> \n> \tgit ls-files -s |\n"},{"id":"491862","messageId":"xmqqo7awvg2w.fsf@gitster.g","threadId":"61146","inReplyTo":"xmqqjzlkwwk9.fsf@gitster.g","subject":"Re: [PATCH v2 1/3] read-cache: optionally collect pathspec matching info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T22:16:55Z","receivedAt":"2024-03-29T22:17:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> A new parameter to run_diff_files() came as a bit of surprise to me.\n>\n> When I responded to the previous round, I somehow thought that we'd\n> add a new member to the rev structure that points at an optional\n> .ps_matched member next to the existing .prune_data member. \n>\n> That way, it would hopefully be easy for a future code to see if a\n> \"diff\" invocation, not necessarily run_diff_files() that compares\n> the working tree and the index, consumed all the pathspec elements.\n> If such a new .ps_matched member is initialized to NULL, all the\n> patch noise we see in this patch will become unnecessary, no?\n\nThis is how such a change may look like. After applying [2/3] and\n[3/3] steps from your series on top of this patch, the updated tests\nin your series (2200 and 7501) seem to still pass.\n\n------- >8 ------------- >8 ------------- >8 ------------- >8 -------\n\nSubject: [PATCH] revision: optionally record matches with pathspec elements\n\nUnlike \"git add\" and other end-user facing command, where it is\ndiagnosed as an error to give a pathspec with an element that does\nnot match any path, the diff machinery does not care if some\nelements of the pathspec does not match. Given that the diff\nmachinery is heavily used in pathspec-limited \"git log\" machinery,\nand it is common for a path to come and go while traversing the\nproject history, this is usually a good thing.\n\nHowever, in some cases we would want to know if all the pathspec\nelements matched. For example, \"git add -u \" internally\nuses the machinery used by \"git diff-files\" to decide contents from\nwhat paths to add to the index, and as an end-user facing command,\n\"git add -u\" would want to report an unmatched pathspec element.\n\nAdd a new .ps_matched member next to the .prune_data member in\n\"struct rev_info\" so that we can optionally keep track of the use of\n.prune_data pathspec elements that can be inspected by the caller.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/add.c | 4 ++--\n builtin/checkout.c | 3 ++-\n builtin/commit.c | 2 +-\n diff-lib.c | 11 ++++++++++-\n read-cache-ll.h | 4 ++--\n read-cache.c | 8 +++++---\n revision.h | 1 +\n 7 files changed, 23 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 393c10cbcf..dc4b42d0ad 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -553,8 +553,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\texit_status |= renormalize_tracked_files(&pathspec, flags);\n \telse\n \t\texit_status |= add_files_to_cache(the_repository, prefix,\n-\t\t\t\t\t\t &pathspec, include_sparse,\n-\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t &pathspec, NULL,\n+\t\t\t\t\t\t include_sparse, flags);\n \n \tif (add_new_files)\n \t\texit_status |= add_files(&dir, flags);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2e8b0d18f4..56d1828856 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -878,7 +878,8 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t * entries in the index.\n \t\t\t */\n \n-\t\t\tadd_files_to_cache(the_repository, NULL, NULL, 0, 0);\n+\t\t\tadd_files_to_cache(the_repository, NULL, NULL, NULL, 0,\n+\t\t\t\t\t 0);\n \t\t\tinit_merge_options(&o, the_repository);\n \t\t\to.verbosity = 0;\n \t\t\twork = write_in_core_index_as_tree(the_repository);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b27b56c8be..8f31decc6b 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -444,7 +444,7 @@ static const char *prepare_index(const char **argv, const char *prefix,\n \t\trepo_hold_locked_index(the_repository, &index_lock,\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \t\tadd_files_to_cache(the_repository, also ? prefix : NULL,\n-\t\t\t\t &pathspec, 0, 0);\n+\t\t\t\t &pathspec, NULL, 0, 0);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tcache_tree_update(&the_index, WRITE_TREE_SILENT);\n \t\tif (write_locked_index(&the_index, &index_lock, 0))\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 1cd790a4d2..683f11e509 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -127,7 +127,16 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tif (diff_can_quit_early(&revs->diffopt))\n \t\t\tbreak;\n \n-\t\tif (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n+\t\t/*\n+\t\t * NEEDSWORK:\n+\t\t * Here we filter with pathspec but the result is further\n+\t\t * filtered out when --relative is in effect. To end-users,\n+\t\t * a pathspec element that matched only to paths outside the\n+\t\t * current directory is like not matching anything at all;\n+\t\t * the handling of ps_matched[] here may become problematic\n+\t\t * if/when we add the \"--error-unmatch\" option to \"git diff\".\n+\t\t */\n+\t\tif (!ce_path_match(istate, ce, &revs->prune_data, revs->ps_matched))\n \t\t\tcontinue;\n \n \t\tif (revs->diffopt.prefix &&\ndiff --git a/read-cache-ll.h b/read-cache-ll.h\nindex 2a50a784f0..09414afd04 100644\n--- a/read-cache-ll.h\n+++ b/read-cache-ll.h\n@@ -480,8 +480,8 @@ extern int verify_ce_order;\n int cmp_cache_name_compare(const void *a_, const void *b_);\n \n int add_files_to_cache(struct repository *repo, const char *prefix,\n-\t\t const struct pathspec *pathspec, int include_sparse,\n-\t\t int flags);\n+\t\t const struct pathspec *pathspec, char *ps_matched,\n+\t\t int include_sparse, int flags);\n \n void overlay_tree_on_index(struct index_state *istate,\n \t\t\t const char *tree_name, const char *prefix);\ndiff --git a/read-cache.c b/read-cache.c\nindex f546cf7875..e1723ad796 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3958,8 +3958,8 @@ static void update_callback(struct diff_queue_struct *q,\n }\n \n int add_files_to_cache(struct repository *repo, const char *prefix,\n-\t\t const struct pathspec *pathspec, int include_sparse,\n-\t\t int flags)\n+\t\t const struct pathspec *pathspec, char *ps_matched,\n+\t\t int include_sparse, int flags)\n {\n \tstruct update_callback_data data;\n \tstruct rev_info rev;\n@@ -3971,8 +3971,10 @@ int add_files_to_cache(struct repository *repo, const char *prefix,\n \n \trepo_init_revisions(repo, &rev, prefix);\n \tsetup_revisions(0, NULL, &rev, NULL);\n-\tif (pathspec)\n+\tif (pathspec) {\n \t\tcopy_pathspec(&rev.prune_data, pathspec);\n+\t\trev.ps_matched = ps_matched;\n+\t}\n \trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = update_callback;\n \trev.diffopt.format_callback_data = &data;\ndiff --git a/revision.h b/revision.h\nindex 94c43138bc..0e470d1df1 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -142,6 +142,7 @@ struct rev_info {\n \t/* Basic information */\n \tconst char *prefix;\n \tconst char *def;\n+\tchar *ps_matched; /* optionally record matches of prune_data */\n \tstruct pathspec prune_data;\n \n \t/*\n-- \n2.44.0-413-gd6fd04375f\n\n"},{"id":"491886","messageId":"b3j7l2ncstdiaxojtollxddmxvkbbeciou25yptguttr5qugmx@y3bzqbdxkyaw","threadId":"61146","inReplyTo":"xmqqzfugvhnf.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] builtin/add: error out when passing untracked path with -u","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-03-30T14:18:11Z","receivedAt":"2024-03-30T14:18:15Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Fri, 29 Mar 2024, Junio C Hamano wrote:\n> Ghanshyam Thakkar writes:\n> > +\tif (take_worktree_changes)\n> > +\t\texit_status |= report_path_error(ps_matched, &pathspec);\n> \n> Hmph, are we sure take_worktree_changes is true only when\n> add_renormalize is false?\n> \n> > \tif (add_new_files)\n> > \t\texit_status |= add_files(&dir, flags);\n> \n> If report_path_error() detected that the pathspec were faulty,\n> should we still proceed to add new files? This is NOT a rhetorical\n> question, as I do not know the answer myself. I do not even know\n> offhand what add_files_to_cache() above did when pathspec elements\n> are not all consumed---if it does not complain and does not refrain\n> from doing any change to the index, then we should follow suite and\n> add_files() here, too.\nSorry if I'm missing something, but in your last line after '---', do you mean\nthat we should proceed even after report_path_error() detected error like in\nthe above patch or perhaps something like this:\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex dc4b42d0ad..eccda485ed 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -64,7 +64,8 @@ static int chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)\n return ret;\n }\n \n-static int renormalize_tracked_files(const struct pathspec *pathspec, int flags)\n+static int renormalize_tracked_files(const struct pathspec *pathspec,\n+ char *ps_matched, int flags)\n {\n int i, retval = 0;\n \n@@ -79,7 +80,8 @@ static int renormalize_tracked_files(const struct pathspec *pathspec, int flags)\n continue; /* do not touch unmerged paths */\n if (!S_ISREG(ce->ce_mode) && !S_ISLNK(ce->ce_mode))\n continue; /* do not touch non blobs */\n- if (pathspec && !ce_path_match(&the_index, ce, pathspec, NULL))\n+ if (pathspec &&\n+ !ce_path_match(&the_index, ce, pathspec, ps_matched))\n continue;\n retval |= add_file_to_index(&the_index, ce->name,\n flags | ADD_CACHE_RENORMALIZE);\n@@ -370,7 +372,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n int add_new_files;\n int require_pathspec;\n char *seen = NULL;\n+ char *ps_matched = NULL;\n struct lock_file lock_file = LOCK_INIT;\n+ struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;\n \n git_config(add_config, NULL);\n \n@@ -487,7 +491,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n if (pathspec.nr) {\n int i;\n char *skip_worktree_seen = NULL;\n- struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;\n \n if (!seen)\n seen = find_pathspecs_matching_against_index(&pathspec,\n@@ -544,18 +547,26 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n free(seen);\n free(skip_worktree_seen);\n- string_list_clear(&only_match_skip_worktree, 0);\n }\n \n begin_odb_transaction();\n \n+ ps_matched = xcalloc(pathspec.nr, 1);\n if (add_renormalize)\n- exit_status |= renormalize_tracked_files(&pathspec, flags);\n+ exit_status |=\n+ renormalize_tracked_files(&pathspec, ps_matched, flags);\n else\n exit_status |= add_files_to_cache(the_repository, prefix,\n- &pathspec, NULL,\n+ &pathspec, ps_matched,\n include_sparse, flags);\n \n+ if ((take_worktree_changes ||\n+ (add_renormalize && !only_match_skip_worktree.nr)) &&\n include_sparse, flags);\n \n+ if ((take_worktree_changes ||\n+ (add_renormalize && !only_match_skip_worktree.nr)) &&\n+ report_path_error(ps_matched, &pathspec)) {\n+ exit_status = 1;\n+ goto cleanup;\n+ }\n+\n if (add_new_files)\n exit_status |= add_files(&dir, flags);\n \n@@ -568,6 +579,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n COMMIT_LOCK | SKIP_IF_UNCHANGED))\n die(_(\"unable to write new index file\"));\n \n+cleanup:\n+ string_list_clear(&only_match_skip_worktree, 0);\n+ free(ps_matched);\n dir_clear(&dir);\n clear_pathspec(&pathspec);\n return exit_status;\n\nAlthough I'm not sure if we should flush_odb_transaction() in the\ncleanup, because end_odb_transaction() would not be called if we go\nstraight to cleanup.\n"},{"id":"491887","messageId":"gfwbrhhklmus4yyxkn3gi6jrt54azgqexi6kyb6snvs5dxlu4g@7g77due7iiq3","threadId":"61146","inReplyTo":"xmqqo7awvg2w.fsf@gitster.g","subject":"Re: [PATCH v2 1/3] read-cache: optionally collect pathspec matching info","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-03-30T14:27:21Z","receivedAt":"2024-03-30T14:27:25Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Fri, 29 Mar 2024, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > A new parameter to run_diff_files() came as a bit of surprise to me.\n> >\n> > When I responded to the previous round, I somehow thought that we'd\n> > add a new member to the rev structure that points at an optional\n> > .ps_matched member next to the existing .prune_data member. \n> >\n> > That way, it would hopefully be easy for a future code to see if a\n> > \"diff\" invocation, not necessarily run_diff_files() that compares\n> > the working tree and the index, consumed all the pathspec elements.\n> > If such a new .ps_matched member is initialized to NULL, all the\n> > patch noise we see in this patch will become unnecessary, no?\n> \n> This is how such a change may look like. After applying [2/3] and\n> [3/3] steps from your series on top of this patch, the updated tests\n> in your series (2200 and 7501) seem to still pass.\n\nThis seems perfect. I hope you're OK with me using this patch as a base\nfor patch [2/3] and [3/3]. :)\n\n> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------\n> \n> Subject: [PATCH] revision: optionally record matches with pathspec elements\n> \n> Unlike \"git add\" and other end-user facing command, where it is\n> diagnosed as an error to give a pathspec with an element that does\n> not match any path, the diff machinery does not care if some\n> elements of the pathspec does not match. Given that the diff\n> machinery is heavily used in pathspec-limited \"git log\" machinery,\n> and it is common for a path to come and go while traversing the\n> project history, this is usually a good thing.\n> \n> However, in some cases we would want to know if all the pathspec\n> elements matched. For example, \"git add -u \" internally\n> uses the machinery used by \"git diff-files\" to decide contents from\n> what paths to add to the index, and as an end-user facing command,\n> \"git add -u\" would want to report an unmatched pathspec element.\n> \n> Add a new .ps_matched member next to the .prune_data member in\n> \"struct rev_info\" so that we can optionally keep track of the use of\n> .prune_data pathspec elements that can be inspected by the caller.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/add.c | 4 ++--\n> builtin/checkout.c | 3 ++-\n> builtin/commit.c | 2 +-\n> diff-lib.c | 11 ++++++++++-\n> read-cache-ll.h | 4 ++--\n> read-cache.c | 8 +++++---\n> revision.h | 1 +\n> 7 files changed, 23 insertions(+), 10 deletions(-)\n> \n> diff --git a/builtin/add.c b/builtin/add.c\n> index 393c10cbcf..dc4b42d0ad 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -553,8 +553,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> \t\texit_status |= renormalize_tracked_files(&pathspec, flags);\n> \telse\n> \t\texit_status |= add_files_to_cache(the_repository, prefix,\n> -\t\t\t\t\t\t &pathspec, include_sparse,\n> -\t\t\t\t\t\t flags);\n> +\t\t\t\t\t\t &pathspec, NULL,\n> +\t\t\t\t\t\t include_sparse, flags);\n> \n> \tif (add_new_files)\n> \t\texit_status |= add_files(&dir, flags);\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 2e8b0d18f4..56d1828856 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -878,7 +878,8 @@ static int merge_working_tree(const struct checkout_opts *opts,\n> \t\t\t * entries in the index.\n> \t\t\t */\n> \n> -\t\t\tadd_files_to_cache(the_repository, NULL, NULL, 0, 0);\n> +\t\t\tadd_files_to_cache(the_repository, NULL, NULL, NULL, 0,\n> +\t\t\t\t\t 0);\n> \t\t\tinit_merge_options(&o, the_repository);\n> \t\t\to.verbosity = 0;\n> \t\t\twork = write_in_core_index_as_tree(the_repository);\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index b27b56c8be..8f31decc6b 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -444,7 +444,7 @@ static const char *prepare_index(const char **argv, const char *prefix,\n> \t\trepo_hold_locked_index(the_repository, &index_lock,\n> \t\t\t\t LOCK_DIE_ON_ERROR);\n> \t\tadd_files_to_cache(the_repository, also ? prefix : NULL,\n> -\t\t\t\t &pathspec, 0, 0);\n> +\t\t\t\t &pathspec, NULL, 0, 0);\n> \t\trefresh_cache_or_die(refresh_flags);\n> \t\tcache_tree_update(&the_index, WRITE_TREE_SILENT);\n> \t\tif (write_locked_index(&the_index, &index_lock, 0))\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 1cd790a4d2..683f11e509 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -127,7 +127,16 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n> \t\tif (diff_can_quit_early(&revs->diffopt))\n> \t\t\tbreak;\n> \n> -\t\tif (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n> +\t\t/*\n> +\t\t * NEEDSWORK:\n> +\t\t * Here we filter with pathspec but the result is further\n> +\t\t * filtered out when --relative is in effect. To end-users,\n> +\t\t * a pathspec element that matched only to paths outside the\n> +\t\t * current directory is like not matching anything at all;\n> +\t\t * the handling of ps_matched[] here may become problematic\n> +\t\t * if/when we add the \"--error-unmatch\" option to \"git diff\".\n> +\t\t */\n> +\t\tif (!ce_path_match(istate, ce, &revs->prune_data, revs->ps_matched))\n> \t\t\tcontinue;\n> \n> \t\tif (revs->diffopt.prefix &&\n> diff --git a/read-cache-ll.h b/read-cache-ll.h\n> index 2a50a784f0..09414afd04 100644\n> --- a/read-cache-ll.h\n> +++ b/read-cache-ll.h\n> @@ -480,8 +480,8 @@ extern int verify_ce_order;\n> int cmp_cache_name_compare(const void *a_, const void *b_);\n> \n> int add_files_to_cache(struct repository *repo, const char *prefix,\n> -\t\t const struct pathspec *pathspec, int include_sparse,\n> -\t\t int flags);\n> +\t\t const struct pathspec *pathspec, char *ps_matched,\n> +\t\t int include_sparse, int flags);\n> \n> void overlay_tree_on_index(struct index_state *istate,\n> \t\t\t const char *tree_name, const char *prefix);\n> diff --git a/read-cache.c b/read-cache.c\n> index f546cf7875..e1723ad796 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -3958,8 +3958,8 @@ static void update_callback(struct diff_queue_struct *q,\n> }\n> \n> int add_files_to_cache(struct repository *repo, const char *prefix,\n> -\t\t const struct pathspec *pathspec, int include_sparse,\n> -\t\t int flags)\n> +\t\t const struct pathspec *pathspec, char *ps_matched,\n> +\t\t int include_sparse, int flags)\n> {\n> \tstruct update_callback_data data;\n> \tstruct rev_info rev;\n> @@ -3971,8 +3971,10 @@ int add_files_to_cache(struct repository *repo, const char *prefix,\n> \n> \trepo_init_revisions(repo, &rev, prefix);\n> \tsetup_revisions(0, NULL, &rev, NULL);\n> -\tif (pathspec)\n> +\tif (pathspec) {\n> \t\tcopy_pathspec(&rev.prune_data, pathspec);\n> +\t\trev.ps_matched = ps_matched;\n> +\t}\n> \trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n> \trev.diffopt.format_callback = update_callback;\n> \trev.diffopt.format_callback_data = &data;\n> diff --git a/revision.h b/revision.h\n> index 94c43138bc..0e470d1df1 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -142,6 +142,7 @@ struct rev_info {\n> \t/* Basic information */\n> \tconst char *prefix;\n> \tconst char *def;\n> +\tchar *ps_matched; /* optionally record matches of prune_data */\n> \tstruct pathspec prune_data;\n> \n> \t/*\n> -- \n> 2.44.0-413-gd6fd04375f\n> \n"},{"id":"491891","messageId":"xmqqv853n0qf.fsf@gitster.g","threadId":"61146","inReplyTo":"gfwbrhhklmus4yyxkn3gi6jrt54azgqexi6kyb6snvs5dxlu4g@7g77due7iiq3","subject":"Re: [PATCH v2 1/3] read-cache: optionally collect pathspec matching info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-30T16:27:52Z","receivedAt":"2024-03-30T16:27:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n>> This is how such a change may look like. After applying [2/3] and\n>> [3/3] steps from your series on top of this patch, the updated tests\n>> in your series (2200 and 7501) seem to still pass.\n>\n> This seems perfect. I hope you're OK with me using this patch as a base\n> for patch [2/3] and [3/3]. :)\n\nYes, as long as you promise to fix typos and grammatical mistakes in\nmy proposed log messages (there are several I just noticed X-<).\n\nThanks.\n"},{"id":"491892","messageId":"xmqqh6gnmzqs.fsf@gitster.g","threadId":"61146","inReplyTo":"b3j7l2ncstdiaxojtollxddmxvkbbeciou25yptguttr5qugmx@y3bzqbdxkyaw","subject":"Re: [PATCH v2 3/3] builtin/add: error out when passing untracked path with -u","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-30T16:49:15Z","receivedAt":"2024-03-30T16:49:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> On Fri, 29 Mar 2024, Junio C Hamano wrote:\n>> Ghanshyam Thakkar writes:\n>> > +\tif (take_worktree_changes)\n>> > +\t\texit_status |= report_path_error(ps_matched, &pathspec);\n>> \n>> Hmph, are we sure take_worktree_changes is true only when\n>> add_renormalize is false?\n>> \n>> > \tif (add_new_files)\n>> > \t\texit_status |= add_files(&dir, flags);\n>> \n>> If report_path_error() detected that the pathspec were faulty,\n>> should we still proceed to add new files? This is NOT a rhetorical\n>> question, as I do not know the answer myself. I do not even know\n>> offhand what add_files_to_cache() above did when pathspec elements\n>> are not all consumed---if it does not complain and does not refrain\n>> from doing any change to the index, then we should follow suite and\n>> add_files() here, too.\n> Sorry if I'm missing something, but in your last line after '---', do you mean\n> that we should proceed even after report_path_error() detected error like in\n> the above patch or perhaps something like this:\n\nWe roughly do:\n\n\tif (add_renorm)\n\t\texit_status |= renorm();\n\telse\n\t\texit_status |= add_files_to_cache();\n+\tif (take_worktree_changes)\n+\t\texit_status |= report_path_error();\n\tif (add_new_files)\n\t\texit_status |= add_files();\n\nI was wondering if we should refrain from adding new files when we\nexit_status is true to avoid making \"further damage\", and was\nwondering if the last one should become:\n\n\tif (!exit_status && add_new_files)\n\t\texit_status |= add_files();\n\nBut that was merely because I was not thinking things through. If\nwe were to go that route, the whole thing needs to become (because\nthere are other things that notice errors before this part of the\ncode):\n\t\n\tif (!exit_status) {\n\t\tif (add_renorm)\n\t\t\texit_status |= renorm();\n\t\telse\n \texit_status |= add_files_to_cache();\n\t}\n\tif (!exit_status && take_worktree_changes)\n\t\texit_status |= report_path_error();\n\n\tif (!exit_status && add_new_files)\n\t\texit_status |= add_files();\n\nbut (1) that is far bigger change of behaviour to the code than\nsuitable for \"notice unmatched pathspec elements and report an\nerror\" topic, and more importantly (2) it is still not sufficient to\nmake it \"all-or-none\". E.g., if \"add_files_to_cache()\" call added\ncontents from a few paths and then noticed that some pathspec\nelements were not used, we are not restoring the previous state to\nrecover. The damage is already done, and not making further damage\ndoes not help the user all that much.\n\nSo, it was a fairly pointless thing that I was wondering about. The\ncurrent behaviour, and the new behaviour with the new check, are\nfine as-is.\n\nIf we wanted to make it \"all-or-none\", I think the way to do so is\nto tweak the final part of the cmd_add() function to skip committing\nthe updated index, e.g.,\n\n finish:\n -\tif (write_locked_index(&the_index, &lock_file,\n +\tif (exit_status)\n +\t\tfputs(_(\"not updating the index due to failure(s)\\n\"), stderr);\n +\telse if (write_locked_index(&the_index, &lock_file,\n COMMIT_LOCK | SKIP_IF_UNCHANGED))\n die(_(\"unable to write new index file\"));\n \nAnd if/when we do so, the existing code (with or without the updates\nmade by the topic under discussion) needs no change. We can do all\nsteps regardless of the errors we notice along the way with earlier\nsteps, and discard the in-core index if we saw any errors.\n\nThe renormalize() thing is not noticing unused pathspec elements,\nwhich we might want to fix, but I suspect it is far less commonly\nused mode of operation, so it may be OK to leave it to future\nfollow-up series.\n\nThanks.\n"},{"id":"491959","messageId":"h7yk7nk7cwyv35reqzfy7brpbn3xoaarhudteyvxfpkodvltt2@eggaahzrjryq","threadId":"61146","inReplyTo":"xmqqh6gnmzqs.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] builtin/add: error out when passing untracked path with -u","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-04-01T13:27:12Z","receivedAt":"2024-04-01T13:27:16Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Sat, 30 Mar 2024, Junio C Hamano wrote:\n> So, it was a fairly pointless thing that I was wondering about. The\n> current behaviour, and the new behaviour with the new check, are\n> fine as-is.\n\nWell I think we should be going 'all-or-none' way as I can't think of\nany major user-facing command that does partial changes incase of\nerror (besides two testcase below).\n\n> If we wanted to make it \"all-or-none\", I think the way to do so is\n> to tweak the final part of the cmd_add() function to skip committing\n> the updated index, e.g.,\n> \n> finish:\n> -\tif (write_locked_index(&the_index, &lock_file,\n> +\tif (exit_status)\n> +\t\tfputs(_(\"not updating the index due to failure(s)\\n\"), stderr);\n> +\telse if (write_locked_index(&the_index, &lock_file,\n> COMMIT_LOCK | SKIP_IF_UNCHANGED))\n> die(_(\"unable to write new index file\"));\n> \n> And if/when we do so, the existing code (with or without the updates\n> made by the topic under discussion) needs no change. We can do all\n> steps regardless of the errors we notice along the way with earlier\n> steps, and discard the in-core index if we saw any errors.\n\nDoing this, we would need to take care of atleast 4 tests breaking in\nt3700-add:\n error out when attempting to add ignored ones but add others\n git add --ignore-errors\n git add (add.ignore-errors)\n git add --chmod fails with non regular files (but updates the other paths)\n\nwhile ignore-errors ones would be trivial to fix, fixing other 2 would\nprobably require some more than trivial code changes, as from the title,\ntheir behavior seems pretty much set in stone. That's why I did the\n'goto cleanup' approach to not break these.\n\nThanks.\n\n> The renormalize() thing is not noticing unused pathspec elements,\n> which we might want to fix, but I suspect it is far less commonly\n> used mode of operation, so it may be OK to leave it to future\n> follow-up series.\n> \n> Thanks.\n"},{"id":"491966","messageId":"xmqqjzlhavu0.fsf@gitster.g","threadId":"61146","inReplyTo":"h7yk7nk7cwyv35reqzfy7brpbn3xoaarhudteyvxfpkodvltt2@eggaahzrjryq","subject":"Re: [PATCH v2 3/3] builtin/add: error out when passing untracked path with -u","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-01T16:31:19Z","receivedAt":"2024-04-01T16:31:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> Well I think we should be going 'all-or-none' way as I can't think of\n> any major user-facing command that does partial changes incase of\n> error (besides two testcase below).\n\nI agree that in the longer run, all-or-none would be something we\nshould aim for, but I'd strongly prefer leaving that outside this\ntopic, especially the existing ones that set exit_status to non-zero\nbut still commits the index changes.\n\nI am OK, as a place to stop for now, if the topic had something like\n\n+\tif (take_worktree_changes) {\n+\t\tif (report_path_error(ps_matched, &pathspec))\n+\t\t\texit(128);\n+\t}\n\nin it, though, because this is a new behaviour.\n\n> Doing this, we would need to take care of atleast 4 tests breaking in\n> t3700-add:\n> error out when attempting to add ignored ones but add others\n> git add --ignore-errors\n> git add (add.ignore-errors)\n> git add --chmod fails with non regular files (but updates the other paths)\n>\n> while ignore-errors ones would be trivial to fix, fixing other 2 would\n> probably require some more than trivial code changes, as from the title,\n> their behavior seems pretty much set in stone. That's why I did the\n> 'goto cleanup' approach to not break these.\n\nI am not sure if these are expecting the right outcome in the first\nplace, and the need to examine what the right behaviour should be is\nwhat makes me say \"I do not want to make the all-or-none thing part\nof this topic\".\n\n>> The renormalize() thing is not noticing unused pathspec elements,\n>> which we might want to fix, but I suspect it is far less commonly\n>> used mode of operation, so it may be OK to leave it to future\n>> follow-up series.\n\nThanks.\n"},{"id":"492113","messageId":"20240402213640.139682-2-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":"20240329205649.1483032-2-shyamthakkar001@gmail.com","subject":"[PATCH v3 0/3] commit, add: error out when passing untracked paths","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-04-02T21:36:21Z","receivedAt":"2024-04-02T21:37:54Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"This version uses Junio's patch ,which adds a new .ps_matched member to\n'strcut rev_info' to record the pathspec elements that matched tracked\npaths, as base for patch [2/3] and [3/3]. \n\nPatch [2/3] remains unchanged from v2. And patch [3/3] is updated\naccording to Junio's suggestions.\n\nGhanshyam Thakkar (2):\n builtin/commit: error out when passing untracked path with -i\n builtin/add: error out when passing untracked path with -u\n\nJunio C Hamano (1):\n revision: optionally record matches with pathspec elements\n\n builtin/add.c | 13 +++++++++++--\n builtin/checkout.c | 3 ++-\n builtin/commit.c | 9 ++++++++-\n diff-lib.c | 11 ++++++++++-\n read-cache-ll.h | 4 ++--\n read-cache.c | 8 +++++---\n revision.h | 1 +\n t/t2200-add-update.sh | 10 ++++++++++\n t/t7501-commit-basic-functionality.sh | 16 +---------------\n 9 files changed, 50 insertions(+), 25 deletions(-)\n\n-- \n2.44.0\n\n"},{"id":"492114","messageId":"20240402213640.139682-4-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":"20240329205649.1483032-2-shyamthakkar001@gmail.com","subject":"[PATCH v3 1/3] revision: optionally record matches with pathspec elements","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-04-02T21:36:23Z","receivedAt":"2024-04-02T21:38:30Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"From: Junio C Hamano \n\nUnlike \"git add\" and other end-user facing commands, where it is\ndiagnosed as an error to give a pathspec with an element that does\nnot match any path, the diff machinery does not care if some\nelements of the pathspec do not match. Given that the diff\nmachinery is heavily used in pathspec-limited \"git log\" machinery,\nand it is common for a path to come and go while traversing the\nproject history, this is usually a good thing.\n\nHowever, in some cases, we would want to know if all the pathspec\nelements matched. For example, \"git add -u \" internally\nuses the machinery used by \"git diff-files\" to decide contents from\nwhat paths to add to the index, and as an end-user facing command,\n\"git add -u\" would want to report an unmatched pathspec element.\n\nAdd a new .ps_matched member next to the .prune_data member in\n\"struct rev_info\" so that we can optionally keep track of the use of\n.prune_data pathspec elements that can be inspected by the caller.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/add.c | 4 ++--\n builtin/checkout.c | 3 ++-\n builtin/commit.c | 2 +-\n diff-lib.c | 11 ++++++++++-\n read-cache-ll.h | 4 ++--\n read-cache.c | 8 +++++---\n revision.h | 1 +\n 7 files changed, 23 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 393c10cbcf..dc4b42d0ad 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -553,8 +553,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\texit_status |= renormalize_tracked_files(&pathspec, flags);\n \telse\n \t\texit_status |= add_files_to_cache(the_repository, prefix,\n-\t\t\t\t\t\t &pathspec, include_sparse,\n-\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t &pathspec, NULL,\n+\t\t\t\t\t\t include_sparse, flags);\n \n \tif (add_new_files)\n \t\texit_status |= add_files(&dir, flags);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2b6166c284..c297aa0e32 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -882,7 +882,8 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t * entries in the index.\n \t\t\t */\n \n-\t\t\tadd_files_to_cache(the_repository, NULL, NULL, 0, 0);\n+\t\t\tadd_files_to_cache(the_repository, NULL, NULL, NULL, 0,\n+\t\t\t\t\t 0);\n \t\t\tinit_merge_options(&o, the_repository);\n \t\t\to.verbosity = 0;\n \t\t\twork = write_in_core_index_as_tree(the_repository);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b27b56c8be..8f31decc6b 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -444,7 +444,7 @@ static const char *prepare_index(const char **argv, const char *prefix,\n \t\trepo_hold_locked_index(the_repository, &index_lock,\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \t\tadd_files_to_cache(the_repository, also ? prefix : NULL,\n-\t\t\t\t &pathspec, 0, 0);\n+\t\t\t\t &pathspec, NULL, 0, 0);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tcache_tree_update(&the_index, WRITE_TREE_SILENT);\n \t\tif (write_locked_index(&the_index, &index_lock, 0))\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 1cd790a4d2..683f11e509 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -127,7 +127,16 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tif (diff_can_quit_early(&revs->diffopt))\n \t\t\tbreak;\n \n-\t\tif (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n+\t\t/*\n+\t\t * NEEDSWORK:\n+\t\t * Here we filter with pathspec but the result is further\n+\t\t * filtered out when --relative is in effect. To end-users,\n+\t\t * a pathspec element that matched only to paths outside the\n+\t\t * current directory is like not matching anything at all;\n+\t\t * the handling of ps_matched[] here may become problematic\n+\t\t * if/when we add the \"--error-unmatch\" option to \"git diff\".\n+\t\t */\n+\t\tif (!ce_path_match(istate, ce, &revs->prune_data, revs->ps_matched))\n \t\t\tcontinue;\n \n \t\tif (revs->diffopt.prefix &&\ndiff --git a/read-cache-ll.h b/read-cache-ll.h\nindex 2a50a784f0..09414afd04 100644\n--- a/read-cache-ll.h\n+++ b/read-cache-ll.h\n@@ -480,8 +480,8 @@ extern int verify_ce_order;\n int cmp_cache_name_compare(const void *a_, const void *b_);\n \n int add_files_to_cache(struct repository *repo, const char *prefix,\n-\t\t const struct pathspec *pathspec, int include_sparse,\n-\t\t int flags);\n+\t\t const struct pathspec *pathspec, char *ps_matched,\n+\t\t int include_sparse, int flags);\n \n void overlay_tree_on_index(struct index_state *istate,\n \t\t\t const char *tree_name, const char *prefix);\ndiff --git a/read-cache.c b/read-cache.c\nindex f546cf7875..e1723ad796 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3958,8 +3958,8 @@ static void update_callback(struct diff_queue_struct *q,\n }\n \n int add_files_to_cache(struct repository *repo, const char *prefix,\n-\t\t const struct pathspec *pathspec, int include_sparse,\n-\t\t int flags)\n+\t\t const struct pathspec *pathspec, char *ps_matched,\n+\t\t int include_sparse, int flags)\n {\n \tstruct update_callback_data data;\n \tstruct rev_info rev;\n@@ -3971,8 +3971,10 @@ int add_files_to_cache(struct repository *repo, const char *prefix,\n \n \trepo_init_revisions(repo, &rev, prefix);\n \tsetup_revisions(0, NULL, &rev, NULL);\n-\tif (pathspec)\n+\tif (pathspec) {\n \t\tcopy_pathspec(&rev.prune_data, pathspec);\n+\t\trev.ps_matched = ps_matched;\n+\t}\n \trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = update_callback;\n \trev.diffopt.format_callback_data = &data;\ndiff --git a/revision.h b/revision.h\nindex 94c43138bc..0e470d1df1 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -142,6 +142,7 @@ struct rev_info {\n \t/* Basic information */\n \tconst char *prefix;\n \tconst char *def;\n+\tchar *ps_matched; /* optionally record matches of prune_data */\n \tstruct pathspec prune_data;\n \n \t/*\n-- \n2.44.0\n\n"},{"id":"492115","messageId":"20240402213640.139682-5-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":"20240329205649.1483032-2-shyamthakkar001@gmail.com","subject":"[PATCH v3 2/3] builtin/commit: error out when passing untracked path with -i","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-04-02T21:36:24Z","receivedAt":"2024-04-02T21:38:41Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"When we provide a pathspec which does not match any tracked path\nalongside --include, we do not error like without --include. If there\nis something staged, it will commit the staged changes and ignore the\npathspec which does not match any tracked path. And if nothing is\nstaged, it will print the status. Exit code is 0 in both cases (unlike\nwithout --include). This is also described in the TODO comment before\nthe relevant testcase.\n\nFix this by passing a character array to add_files_to_cache() to\ncollect the pathspec matching information and error out if the given\npath is untracked. Also, amend the testcase to check for the error\nmessage and remove the TODO comment.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n builtin/commit.c | 9 ++++++++-\n t/t7501-commit-basic-functionality.sh | 16 +---------------\n 2 files changed, 9 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 8f31decc6b..09c48a835a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -441,10 +441,17 @@ static const char *prepare_index(const char **argv, const char *prefix,\n \t * (B) on failure, rollback the real index.\n \t */\n \tif (all || (also && pathspec.nr)) {\n+\t\tchar *ps_matched = xcalloc(pathspec.nr, 1);\n \t\trepo_hold_locked_index(the_repository, &index_lock,\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \t\tadd_files_to_cache(the_repository, also ? prefix : NULL,\n-\t\t\t\t &pathspec, NULL, 0, 0);\n+\t\t\t\t &pathspec, ps_matched, 0, 0);\n+\t\tif (!all && report_path_error(ps_matched, &pathspec)) {\n+\t\t\tfree(ps_matched);\n+\t\t\texit(1);\n+\t\t}\n+\t\tfree(ps_matched);\n+\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tcache_tree_update(&the_index, WRITE_TREE_SILENT);\n \t\tif (write_locked_index(&the_index, &index_lock, 0))\ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex bced44a0fc..cc12f99f11 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -101,22 +101,8 @@ test_expect_success 'fail to commit untracked file (even with --include/--only)'\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+\ttest_grep -e \"$error\" err\n '\n \n test_expect_success 'setup: non-initial commit' '\n-- \n2.44.0\n\n"},{"id":"492116","messageId":"20240402213640.139682-7-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":"20240329205649.1483032-2-shyamthakkar001@gmail.com","subject":"[PATCH v3 3/3] builtin/add: error out when passing untracked path with -u","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-04-02T21:36:26Z","receivedAt":"2024-04-02T21:39:37Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"When passing untracked path with -u option, it silently succeeds.\nThere is no error message and the exit code is zero. This is\ninconsistent with other instances of git commands where the expected\nargument is a known path. In those other instances, we error out when\nthe path is not known.\n\nFix this by passing a character array to add_files_to_cache() to\ncollect the pathspec matching information and report the error and\nexit if a pathspec does not match any cache entry. Also add a testcase\nto cover this scenario.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n builtin/add.c | 11 ++++++++++-\n t/t2200-add-update.sh | 10 ++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex dc4b42d0ad..88261b0f2b 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -370,6 +370,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tint add_new_files;\n \tint require_pathspec;\n \tchar *seen = NULL;\n+\tchar *ps_matched = NULL;\n \tstruct lock_file lock_file = LOCK_INIT;\n \n \tgit_config(add_config, NULL);\n@@ -549,13 +550,20 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n \tbegin_odb_transaction();\n \n+\tps_matched = xcalloc(pathspec.nr, 1);\n \tif (add_renormalize)\n \t\texit_status |= renormalize_tracked_files(&pathspec, flags);\n \telse\n \t\texit_status |= add_files_to_cache(the_repository, prefix,\n-\t\t\t\t\t\t &pathspec, NULL,\n+\t\t\t\t\t\t &pathspec, ps_matched,\n \t\t\t\t\t\t include_sparse, flags);\n \n+\tif (take_worktree_changes && !add_renormalize &&\n+\t report_path_error(ps_matched, &pathspec)) {\n+\t\tfree(ps_matched);\n+\t\texit(1);\n+\t}\n+\n \tif (add_new_files)\n \t\texit_status |= add_files(&dir, flags);\n \n@@ -568,6 +576,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\tdie(_(\"unable to write new index file\"));\n \n+\tfree(ps_matched);\n \tdir_clear(&dir);\n \tclear_pathspec(&pathspec);\n \treturn exit_status;\ndiff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\nindex c01492f33f..df235ac306 100755\n--- a/t/t2200-add-update.sh\n+++ b/t/t2200-add-update.sh\n@@ -65,6 +65,16 @@ test_expect_success 'update did not touch untracked files' '\n \ttest_must_be_empty out\n '\n \n+test_expect_success 'error out when passing untracked path' '\n+\tgit reset --hard &&\n+\techo content >>baz &&\n+\techo content >>top &&\n+\ttest_must_fail git add -u baz top 2>err &&\n+\ttest_grep -e \"error: pathspec .baz. did not match any file(s) known to git\" err &&\n+\tgit diff --cached --name-only >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'cache tree has not been corrupted' '\n \n \tgit ls-files -s |\n-- \n2.44.0\n\n"},{"id":"492117","messageId":"xmqqmsqb30a1.fsf@gitster.g","threadId":"61146","inReplyTo":"20240402213640.139682-5-shyamthakkar001@gmail.com","subject":"Re: [PATCH v3 2/3] builtin/commit: error out when passing untracked path with -i","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-02T21:47:02Z","receivedAt":"2024-04-02T21:47:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 8f31decc6b..09c48a835a 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -441,10 +441,17 @@ static const char *prepare_index(const char **argv, const char *prefix,\n> \t * (B) on failure, rollback the real index.\n> \t */\n> \tif (all || (also && pathspec.nr)) {\n> +\t\tchar *ps_matched = xcalloc(pathspec.nr, 1);\n> \t\trepo_hold_locked_index(the_repository, &index_lock,\n> \t\t\t\t LOCK_DIE_ON_ERROR);\n> \t\tadd_files_to_cache(the_repository, also ? prefix : NULL,\n> -\t\t\t\t &pathspec, NULL, 0, 0);\n> +\t\t\t\t &pathspec, ps_matched, 0, 0);\n> +\t\tif (!all && report_path_error(ps_matched, &pathspec)) {\n> +\t\t\tfree(ps_matched);\n> +\t\t\texit(1);\n\nNo need to free(ps_matched) immediately before exiting. There are\nother recources (like pathspec) we are holding and not clearing, and\nwe do not want to bother cleaning them all.\n\nAs we have another \"if failed, die()\" immediately after this hunk,\nadding another exit() would be OK. Shouldn't we be exiting with 128\nto match what die() does, though?\n\nOther than that, looking good.\n"},{"id":"492118","messageId":"xmqqh6gj305n.fsf@gitster.g","threadId":"61146","inReplyTo":"20240402213640.139682-7-shyamthakkar001@gmail.com","subject":"Re: [PATCH v3 3/3] builtin/add: error out when passing untracked path with -u","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-02T21:49:40Z","receivedAt":"2024-04-02T21:49:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar writes:\n\n> When passing untracked path with -u option, it silently succeeds.\n> There is no error message and the exit code is zero. This is\n> inconsistent with other instances of git commands where the expected\n> argument is a known path. In those other instances, we error out when\n> the path is not known.\n>\n> Fix this by passing a character array to add_files_to_cache() to\n> collect the pathspec matching information and report the error and\n> exit if a pathspec does not match any cache entry. Also add a testcase\n> to cover this scenario.\n>\n> Signed-off-by: Ghanshyam Thakkar \n> ---\n> builtin/add.c | 11 ++++++++++-\n> t/t2200-add-update.sh | 10 ++++++++++\n> 2 files changed, 20 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index dc4b42d0ad..88261b0f2b 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -370,6 +370,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> \tint add_new_files;\n> \tint require_pathspec;\n> \tchar *seen = NULL;\n> +\tchar *ps_matched = NULL;\n> \tstruct lock_file lock_file = LOCK_INIT;\n> \n> \tgit_config(add_config, NULL);\n> @@ -549,13 +550,20 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> \n> \tbegin_odb_transaction();\n> \n> +\tps_matched = xcalloc(pathspec.nr, 1);\n> \tif (add_renormalize)\n> \t\texit_status |= renormalize_tracked_files(&pathspec, flags);\n> \telse\n> \t\texit_status |= add_files_to_cache(the_repository, prefix,\n> -\t\t\t\t\t\t &pathspec, NULL,\n> +\t\t\t\t\t\t &pathspec, ps_matched,\n> \t\t\t\t\t\t include_sparse, flags);\n> \n> +\tif (take_worktree_changes && !add_renormalize &&\n> +\t report_path_error(ps_matched, &pathspec)) {\n> +\t\tfree(ps_matched);\n> +\t\texit(1);\n> +\t}\n\nShouldn't we pay attention to ignore_add_errors? The same comments\nabout free'ing and exit code from the review on the previous step\napply here, too.\n\nOther than that, looking good.\n"},{"id":"492119","messageId":"gvb4jewvfu733mnrqvna4ulbinep5cjs5b4tw5vr2zet7p2bky@2hg2nmq6gnvz","threadId":"61146","inReplyTo":"xmqqmsqb30a1.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] builtin/commit: error out when passing untracked path with -i","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-04-02T21:58:43Z","receivedAt":"2024-04-02T21:58:48Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Tue, 02 Apr 2024, Junio C Hamano wrote:\n> Ghanshyam Thakkar writes:\n> \n> > diff --git a/builtin/commit.c b/builtin/commit.c\n> > index 8f31decc6b..09c48a835a 100644\n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n> > @@ -441,10 +441,17 @@ static const char *prepare_index(const char **argv, const char *prefix,\n> > \t * (B) on failure, rollback the real index.\n> > \t */\n> > \tif (all || (also && pathspec.nr)) {\n> > +\t\tchar *ps_matched = xcalloc(pathspec.nr, 1);\n> > \t\trepo_hold_locked_index(the_repository, &index_lock,\n> > \t\t\t\t LOCK_DIE_ON_ERROR);\n> > \t\tadd_files_to_cache(the_repository, also ? prefix : NULL,\n> > -\t\t\t\t &pathspec, NULL, 0, 0);\n> > +\t\t\t\t &pathspec, ps_matched, 0, 0);\n> > +\t\tif (!all && report_path_error(ps_matched, &pathspec)) {\n> > +\t\t\tfree(ps_matched);\n> > +\t\t\texit(1);\n> \n> No need to free(ps_matched) immediately before exiting. There are\n> other recources (like pathspec) we are holding and not clearing, and\n> we do not want to bother cleaning them all.\n\nUnderstood.\n\n> As we have another \"if failed, die()\" immediately after this hunk,\n> adding another exit() would be OK. Shouldn't we be exiting with 128\n> to match what die() does, though?\n\nI tried to match the exit code with the existing invocations of the same\nwhen doing partial commit and reporting path errors. In\nbuiltin/commit.c:\n\n511\tif (list_paths(&partial, !current_head ? NULL : \"HEAD\", &pathspec))\n512\t\texit(1);\n\nlist_paths() returns the return value of report_path_error().\n\n> Other than that, looking good.\n\nThanks.\n"},{"id":"492120","messageId":"rrt3yuhd7sjhgqhra75w43dp2okrx5h4urqiyopxe4dmnwunnk@tifrkksvm3ak","threadId":"61146","inReplyTo":"xmqqh6gj305n.fsf@gitster.g","subject":"Re: [PATCH v3 3/3] builtin/add: error out when passing untracked path with -u","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-04-02T22:00:32Z","receivedAt":"2024-04-02T22:00:36Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Tue, 02 Apr 2024, Junio C Hamano wrote:\n> Ghanshyam Thakkar writes:\n> \n> > When passing untracked path with -u option, it silently succeeds.\n> > There is no error message and the exit code is zero. This is\n> > inconsistent with other instances of git commands where the expected\n> > argument is a known path. In those other instances, we error out when\n> > the path is not known.\n> >\n> > Fix this by passing a character array to add_files_to_cache() to\n> > collect the pathspec matching information and report the error and\n> > exit if a pathspec does not match any cache entry. Also add a testcase\n> > to cover this scenario.\n> >\n> > Signed-off-by: Ghanshyam Thakkar \n> > ---\n> > builtin/add.c | 11 ++++++++++-\n> > t/t2200-add-update.sh | 10 ++++++++++\n> > 2 files changed, 20 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/add.c b/builtin/add.c\n> > index dc4b42d0ad..88261b0f2b 100644\n> > --- a/builtin/add.c\n> > +++ b/builtin/add.c\n> > @@ -370,6 +370,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> > \tint add_new_files;\n> > \tint require_pathspec;\n> > \tchar *seen = NULL;\n> > +\tchar *ps_matched = NULL;\n> > \tstruct lock_file lock_file = LOCK_INIT;\n> > \n> > \tgit_config(add_config, NULL);\n> > @@ -549,13 +550,20 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> > \n> > \tbegin_odb_transaction();\n> > \n> > +\tps_matched = xcalloc(pathspec.nr, 1);\n> > \tif (add_renormalize)\n> > \t\texit_status |= renormalize_tracked_files(&pathspec, flags);\n> > \telse\n> > \t\texit_status |= add_files_to_cache(the_repository, prefix,\n> > -\t\t\t\t\t\t &pathspec, NULL,\n> > +\t\t\t\t\t\t &pathspec, ps_matched,\n> > \t\t\t\t\t\t include_sparse, flags);\n> > \n> > +\tif (take_worktree_changes && !add_renormalize &&\n> > +\t report_path_error(ps_matched, &pathspec)) {\n> > +\t\tfree(ps_matched);\n> > +\t\texit(1);\n> > +\t}\n> \n> Shouldn't we pay attention to ignore_add_errors? The same comments\n> about free'ing and exit code from the review on the previous step\n> apply here, too.\n\nWill update.\n\n> Other than that, looking good.\n\nThanks.\n"},{"id":"492176","messageId":"20240403181531.59505-2-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":"20240402213640.139682-2-shyamthakkar001@gmail.com","subject":"[PATCH v4 0/3] commit,add: error out when passing untracked path","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-04-03T18:14:46Z","receivedAt":"2024-04-03T18:17:33Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"This version makes the changes as suggested by Junio. In particular,\nchanged exit codes from 1 to 128. And, removed unnecessary free()\ncalls immediately before exiting.\n\nGhanshyam Thakkar (2):\n builtin/commit: error out when passing untracked path with -i\n builtin/add: error out when passing untracked path with -u\n\nJunio C Hamano (1):\n revision: optionally record matches with pathspec elements\n\n builtin/add.c | 11 +++++++++--\n builtin/checkout.c | 3 ++-\n builtin/commit.c | 7 ++++++-\n diff-lib.c | 11 ++++++++++-\n read-cache-ll.h | 4 ++--\n read-cache.c | 8 +++++---\n revision.h | 1 +\n t/t2200-add-update.sh | 10 ++++++++++\n t/t7501-commit-basic-functionality.sh | 16 +---------------\n 9 files changed, 46 insertions(+), 25 deletions(-)\n\n-- \n2.44.0\n\n"},{"id":"492177","messageId":"20240403181531.59505-4-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":"20240402213640.139682-2-shyamthakkar001@gmail.com","subject":"[PATCH v4 1/3] revision: optionally record matches with pathspec elements","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-04-03T18:14:48Z","receivedAt":"2024-04-03T18:18:48Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"From: Junio C Hamano \n\nUnlike \"git add\" and other end-user facing commands, where it is\ndiagnosed as an error to give a pathspec with an element that does\nnot match any path, the diff machinery does not care if some\nelements of the pathspec do not match. Given that the diff\nmachinery is heavily used in pathspec-limited \"git log\" machinery,\nand it is common for a path to come and go while traversing the\nproject history, this is usually a good thing.\n\nHowever, in some cases we would want to know if all the pathspec\nelements matched. For example, \"git add -u \" internally\nuses the machinery used by \"git diff-files\" to decide contents from\nwhat paths to add to the index, and as an end-user facing command,\n\"git add -u\" would want to report an unmatched pathspec element.\n\nAdd a new .ps_matched member next to the .prune_data member in\n\"struct rev_info\" so that we can optionally keep track of the use of\n.prune_data pathspec elements that can be inspected by the caller.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/add.c | 4 ++--\n builtin/checkout.c | 3 ++-\n builtin/commit.c | 2 +-\n diff-lib.c | 11 ++++++++++-\n read-cache-ll.h | 4 ++--\n read-cache.c | 8 +++++---\n revision.h | 1 +\n 7 files changed, 23 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 393c10cbcf..dc4b42d0ad 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -553,8 +553,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\texit_status |= renormalize_tracked_files(&pathspec, flags);\n \telse\n \t\texit_status |= add_files_to_cache(the_repository, prefix,\n-\t\t\t\t\t\t &pathspec, include_sparse,\n-\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t &pathspec, NULL,\n+\t\t\t\t\t\t include_sparse, flags);\n \n \tif (add_new_files)\n \t\texit_status |= add_files(&dir, flags);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2b6166c284..c297aa0e32 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -882,7 +882,8 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t * entries in the index.\n \t\t\t */\n \n-\t\t\tadd_files_to_cache(the_repository, NULL, NULL, 0, 0);\n+\t\t\tadd_files_to_cache(the_repository, NULL, NULL, NULL, 0,\n+\t\t\t\t\t 0);\n \t\t\tinit_merge_options(&o, the_repository);\n \t\t\to.verbosity = 0;\n \t\t\twork = write_in_core_index_as_tree(the_repository);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b27b56c8be..8f31decc6b 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -444,7 +444,7 @@ static const char *prepare_index(const char **argv, const char *prefix,\n \t\trepo_hold_locked_index(the_repository, &index_lock,\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \t\tadd_files_to_cache(the_repository, also ? prefix : NULL,\n-\t\t\t\t &pathspec, 0, 0);\n+\t\t\t\t &pathspec, NULL, 0, 0);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tcache_tree_update(&the_index, WRITE_TREE_SILENT);\n \t\tif (write_locked_index(&the_index, &index_lock, 0))\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 1cd790a4d2..683f11e509 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -127,7 +127,16 @@ void run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tif (diff_can_quit_early(&revs->diffopt))\n \t\t\tbreak;\n \n-\t\tif (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n+\t\t/*\n+\t\t * NEEDSWORK:\n+\t\t * Here we filter with pathspec but the result is further\n+\t\t * filtered out when --relative is in effect. To end-users,\n+\t\t * a pathspec element that matched only to paths outside the\n+\t\t * current directory is like not matching anything at all;\n+\t\t * the handling of ps_matched[] here may become problematic\n+\t\t * if/when we add the \"--error-unmatch\" option to \"git diff\".\n+\t\t */\n+\t\tif (!ce_path_match(istate, ce, &revs->prune_data, revs->ps_matched))\n \t\t\tcontinue;\n \n \t\tif (revs->diffopt.prefix &&\ndiff --git a/read-cache-ll.h b/read-cache-ll.h\nindex 2a50a784f0..09414afd04 100644\n--- a/read-cache-ll.h\n+++ b/read-cache-ll.h\n@@ -480,8 +480,8 @@ extern int verify_ce_order;\n int cmp_cache_name_compare(const void *a_, const void *b_);\n \n int add_files_to_cache(struct repository *repo, const char *prefix,\n-\t\t const struct pathspec *pathspec, int include_sparse,\n-\t\t int flags);\n+\t\t const struct pathspec *pathspec, char *ps_matched,\n+\t\t int include_sparse, int flags);\n \n void overlay_tree_on_index(struct index_state *istate,\n \t\t\t const char *tree_name, const char *prefix);\ndiff --git a/read-cache.c b/read-cache.c\nindex f546cf7875..e1723ad796 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3958,8 +3958,8 @@ static void update_callback(struct diff_queue_struct *q,\n }\n \n int add_files_to_cache(struct repository *repo, const char *prefix,\n-\t\t const struct pathspec *pathspec, int include_sparse,\n-\t\t int flags)\n+\t\t const struct pathspec *pathspec, char *ps_matched,\n+\t\t int include_sparse, int flags)\n {\n \tstruct update_callback_data data;\n \tstruct rev_info rev;\n@@ -3971,8 +3971,10 @@ int add_files_to_cache(struct repository *repo, const char *prefix,\n \n \trepo_init_revisions(repo, &rev, prefix);\n \tsetup_revisions(0, NULL, &rev, NULL);\n-\tif (pathspec)\n+\tif (pathspec) {\n \t\tcopy_pathspec(&rev.prune_data, pathspec);\n+\t\trev.ps_matched = ps_matched;\n+\t}\n \trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = update_callback;\n \trev.diffopt.format_callback_data = &data;\ndiff --git a/revision.h b/revision.h\nindex 94c43138bc..0e470d1df1 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -142,6 +142,7 @@ struct rev_info {\n \t/* Basic information */\n \tconst char *prefix;\n \tconst char *def;\n+\tchar *ps_matched; /* optionally record matches of prune_data */\n \tstruct pathspec prune_data;\n \n \t/*\n-- \n2.44.0\n\n"},{"id":"492178","messageId":"20240403181531.59505-6-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":"20240402213640.139682-2-shyamthakkar001@gmail.com","subject":"[PATCH v4 2/3] builtin/commit: error out when passing untracked path with -i","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-04-03T18:14:50Z","receivedAt":"2024-04-03T18:19:05Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"When we provide a pathspec which does not match any tracked path\nalongside --include, we do not error like without --include. If there\nis something staged, it will commit the staged changes and ignore the\npathspec which does not match any tracked path. And if nothing is\nstaged, it will print the status. Exit code is 0 in both cases (unlike\nwithout --include). This is also described in the TODO comment before\nthe relevant testcase.\n\nFix this by passing a character array to add_files_to_cache() to\ncollect the pathspec matching information and error out if the given\npath is untracked. Also, amend the testcase to check for the error\nmessage and remove the TODO comment.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n builtin/commit.c | 7 ++++++-\n t/t7501-commit-basic-functionality.sh | 16 +---------------\n 2 files changed, 7 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 8f31decc6b..84caf65603 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -441,16 +441,21 @@ static const char *prepare_index(const char **argv, const char *prefix,\n \t * (B) on failure, rollback the real index.\n \t */\n \tif (all || (also && pathspec.nr)) {\n+\t\tchar *ps_matched = xcalloc(pathspec.nr, 1);\n \t\trepo_hold_locked_index(the_repository, &index_lock,\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \t\tadd_files_to_cache(the_repository, also ? prefix : NULL,\n-\t\t\t\t &pathspec, NULL, 0, 0);\n+\t\t\t\t &pathspec, ps_matched, 0, 0);\n+\t\tif (!all && report_path_error(ps_matched, &pathspec))\n+\t\t\texit(128);\n+\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tcache_tree_update(&the_index, WRITE_TREE_SILENT);\n \t\tif (write_locked_index(&the_index, &index_lock, 0))\n \t\t\tdie(_(\"unable to write new index file\"));\n \t\tcommit_style = COMMIT_NORMAL;\n \t\tret = get_lock_file_path(&index_lock);\n+\t\tfree(ps_matched);\n \t\tgoto out;\n \t}\n \ndiff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh\nindex bced44a0fc..cc12f99f11 100755\n--- a/t/t7501-commit-basic-functionality.sh\n+++ b/t/t7501-commit-basic-functionality.sh\n@@ -101,22 +101,8 @@ test_expect_success 'fail to commit untracked file (even with --include/--only)'\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+\ttest_grep -e \"$error\" err\n '\n \n test_expect_success 'setup: non-initial commit' '\n-- \n2.44.0\n\n"},{"id":"492179","messageId":"20240403181531.59505-8-shyamthakkar001@gmail.com","threadId":"61146","inReplyTo":"20240402213640.139682-2-shyamthakkar001@gmail.com","subject":"[PATCH v4 3/3] builtin/add: error out when passing untracked path with -u","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-04-03T18:14:52Z","receivedAt":"2024-04-03T18:19:59Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"When passing untracked path with -u option, it silently succeeds.\nThere is no error message and the exit code is zero. This is\ninconsistent with other instances of git commands where the expected\nargument is a known path. In those other instances, we error out when\nthe path is not known.\n\nFix this by passing a character array to add_files_to_cache() to\ncollect the pathspec matching information and report the error if a\npathspec does not match any cache entry. Also add a testcase to cover\nthis scenario.\n\nSigned-off-by: Ghanshyam Thakkar \n---\n builtin/add.c | 9 ++++++++-\n t/t2200-add-update.sh | 10 ++++++++++\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex dc4b42d0ad..1937c19097 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -370,6 +370,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tint add_new_files;\n \tint require_pathspec;\n \tchar *seen = NULL;\n+\tchar *ps_matched = NULL;\n \tstruct lock_file lock_file = LOCK_INIT;\n \n \tgit_config(add_config, NULL);\n@@ -549,13 +550,18 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n \tbegin_odb_transaction();\n \n+\tps_matched = xcalloc(pathspec.nr, 1);\n \tif (add_renormalize)\n \t\texit_status |= renormalize_tracked_files(&pathspec, flags);\n \telse\n \t\texit_status |= add_files_to_cache(the_repository, prefix,\n-\t\t\t\t\t\t &pathspec, NULL,\n+\t\t\t\t\t\t &pathspec, ps_matched,\n \t\t\t\t\t\t include_sparse, flags);\n \n+\tif (take_worktree_changes && !add_renormalize && !ignore_add_errors &&\n+\t report_path_error(ps_matched, &pathspec))\n+\t\texit(128);\n+\n \tif (add_new_files)\n \t\texit_status |= add_files(&dir, flags);\n \n@@ -568,6 +574,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\tdie(_(\"unable to write new index file\"));\n \n+\tfree(ps_matched);\n \tdir_clear(&dir);\n \tclear_pathspec(&pathspec);\n \treturn exit_status;\ndiff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\nindex c01492f33f..df235ac306 100755\n--- a/t/t2200-add-update.sh\n+++ b/t/t2200-add-update.sh\n@@ -65,6 +65,16 @@ test_expect_success 'update did not touch untracked files' '\n \ttest_must_be_empty out\n '\n \n+test_expect_success 'error out when passing untracked path' '\n+\tgit reset --hard &&\n+\techo content >>baz &&\n+\techo content >>top &&\n+\ttest_must_fail git add -u baz top 2>err &&\n+\ttest_grep -e \"error: pathspec .baz. did not match any file(s) known to git\" err &&\n+\tgit diff --cached --name-only >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'cache tree has not been corrupted' '\n \n \tgit ls-files -s |\n-- \n2.44.0\n\n"}]}