From 86829f3f3ea62853a4a632637ade6de94a1f4c7c Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 3 Apr 2024 23:44:48 +0530 Subject: [PATCH 1/3] revision: optionally record matches with pathspec elements Unlike "git add" and other end-user facing commands, where it is diagnosed as an error to give a pathspec with an element that does not match any path, the diff machinery does not care if some elements of the pathspec do not match. Given that the diff machinery is heavily used in pathspec-limited "git log" machinery, and it is common for a path to come and go while traversing the project history, this is usually a good thing. However, in some cases we would want to know if all the pathspec elements matched. For example, "git add -u " internally uses the machinery used by "git diff-files" to decide contents from what paths to add to the index, and as an end-user facing command, "git add -u" would want to report an unmatched pathspec element. Add a new .ps_matched member next to the .prune_data member in "struct rev_info" so that we can optionally keep track of the use of .prune_data pathspec elements that can be inspected by the caller. Signed-off-by: Junio C Hamano --- builtin/add.c | 4 ++-- builtin/checkout.c | 3 ++- builtin/commit.c | 2 +- diff-lib.c | 11 ++++++++++- read-cache-ll.h | 4 ++-- read-cache.c | 8 +++++--- revision.h | 1 + 7 files changed, 23 insertions(+), 10 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index 393c10cbcf..dc4b42d0ad 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -553,8 +553,8 @@ int cmd_add(int argc, const char **argv, const char *prefix) exit_status |= renormalize_tracked_files(&pathspec, flags); else exit_status |= add_files_to_cache(the_repository, prefix, - &pathspec, include_sparse, - flags); + &pathspec, NULL, + include_sparse, flags); if (add_new_files) exit_status |= add_files(&dir, flags); diff --git a/builtin/checkout.c b/builtin/checkout.c index 2e8b0d18f4..56d1828856 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -878,7 +878,8 @@ static int merge_working_tree(const struct checkout_opts *opts, * entries in the index. */ - add_files_to_cache(the_repository, NULL, NULL, 0, 0); + add_files_to_cache(the_repository, NULL, NULL, NULL, 0, + 0); init_merge_options(&o, the_repository); o.verbosity = 0; work = write_in_core_index_as_tree(the_repository); diff --git a/builtin/commit.c b/builtin/commit.c index b27b56c8be..8f31decc6b 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -444,7 +444,7 @@ static const char *prepare_index(const char **argv, const char *prefix, repo_hold_locked_index(the_repository, &index_lock, LOCK_DIE_ON_ERROR); add_files_to_cache(the_repository, also ? prefix : NULL, - &pathspec, 0, 0); + &pathspec, NULL, 0, 0); refresh_cache_or_die(refresh_flags); cache_tree_update(&the_index, WRITE_TREE_SILENT); if (write_locked_index(&the_index, &index_lock, 0)) diff --git a/diff-lib.c b/diff-lib.c index 1cd790a4d2..683f11e509 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -127,7 +127,16 @@ void run_diff_files(struct rev_info *revs, unsigned int option) if (diff_can_quit_early(&revs->diffopt)) break; - if (!ce_path_match(istate, ce, &revs->prune_data, NULL)) + /* + * NEEDSWORK: + * Here we filter with pathspec but the result is further + * filtered out when --relative is in effect. To end-users, + * a pathspec element that matched only to paths outside the + * current directory is like not matching anything at all; + * the handling of ps_matched[] here may become problematic + * if/when we add the "--error-unmatch" option to "git diff". + */ + if (!ce_path_match(istate, ce, &revs->prune_data, revs->ps_matched)) continue; if (revs->diffopt.prefix && diff --git a/read-cache-ll.h b/read-cache-ll.h index 2a50a784f0..09414afd04 100644 --- a/read-cache-ll.h +++ b/read-cache-ll.h @@ -480,8 +480,8 @@ extern int verify_ce_order; int cmp_cache_name_compare(const void *a_, const void *b_); int add_files_to_cache(struct repository *repo, const char *prefix, - const struct pathspec *pathspec, int include_sparse, - int flags); + const struct pathspec *pathspec, char *ps_matched, + int include_sparse, int flags); void overlay_tree_on_index(struct index_state *istate, const char *tree_name, const char *prefix); diff --git a/read-cache.c b/read-cache.c index f546cf7875..e1723ad796 100644 --- a/read-cache.c +++ b/read-cache.c @@ -3958,8 +3958,8 @@ static void update_callback(struct diff_queue_struct *q, } int add_files_to_cache(struct repository *repo, const char *prefix, - const struct pathspec *pathspec, int include_sparse, - int flags) + const struct pathspec *pathspec, char *ps_matched, + int include_sparse, int flags) { struct update_callback_data data; struct rev_info rev; @@ -3971,8 +3971,10 @@ int add_files_to_cache(struct repository *repo, const char *prefix, repo_init_revisions(repo, &rev, prefix); setup_revisions(0, NULL, &rev, NULL); - if (pathspec) + if (pathspec) { copy_pathspec(&rev.prune_data, pathspec); + rev.ps_matched = ps_matched; + } rev.diffopt.output_format = DIFF_FORMAT_CALLBACK; rev.diffopt.format_callback = update_callback; rev.diffopt.format_callback_data = &data; diff --git a/revision.h b/revision.h index 94c43138bc..0e470d1df1 100644 --- a/revision.h +++ b/revision.h @@ -142,6 +142,7 @@ struct rev_info { /* Basic information */ const char *prefix; const char *def; + char *ps_matched; /* optionally record matches of prune_data */ struct pathspec prune_data; /* From ac5946e6248eb84458d236099f356e5e09e2482f Mon Sep 17 00:00:00 2001 From: Ghanshyam Thakkar Date: Wed, 3 Apr 2024 23:44:50 +0530 Subject: [PATCH 2/3] builtin/commit: error out when passing untracked path with -i When we provide a pathspec which does not match any tracked path alongside --include, we do not error like without --include. If there is something staged, it will commit the staged changes and ignore the pathspec which does not match any tracked path. And if nothing is staged, it will print the status. Exit code is 0 in both cases (unlike without --include). This is also described in the TODO comment before the relevant testcase. Fix this by passing a character array to add_files_to_cache() to collect the pathspec matching information and error out if the given path is untracked. Also, amend the testcase to check for the error message and remove the TODO comment. Signed-off-by: Ghanshyam Thakkar Signed-off-by: Junio C Hamano --- builtin/commit.c | 7 ++++++- t/t7501-commit-basic-functionality.sh | 16 +--------------- 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 8f31decc6b..84caf65603 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -441,16 +441,21 @@ static const char *prepare_index(const char **argv, const char *prefix, * (B) on failure, rollback the real index. */ if (all || (also && pathspec.nr)) { + char *ps_matched = xcalloc(pathspec.nr, 1); repo_hold_locked_index(the_repository, &index_lock, LOCK_DIE_ON_ERROR); add_files_to_cache(the_repository, also ? prefix : NULL, - &pathspec, NULL, 0, 0); + &pathspec, ps_matched, 0, 0); + if (!all && report_path_error(ps_matched, &pathspec)) + exit(128); + refresh_cache_or_die(refresh_flags); cache_tree_update(&the_index, WRITE_TREE_SILENT); if (write_locked_index(&the_index, &index_lock, 0)) die(_("unable to write new index file")); commit_style = COMMIT_NORMAL; ret = get_lock_file_path(&index_lock); + free(ps_matched); goto out; } diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh index bced44a0fc..cc12f99f11 100755 --- a/t/t7501-commit-basic-functionality.sh +++ b/t/t7501-commit-basic-functionality.sh @@ -101,22 +101,8 @@ test_expect_success 'fail to commit untracked file (even with --include/--only)' test_must_fail git commit --only -m "baz" baz 2>err && test_grep -e "$error" err && - # TODO: as for --include, the below command will fail because - # nothing is staged. If something was staged, it would not fail - # even though the provided pathspec does not match any tracked - # path. (However, the untracked paths that match the pathspec are - # not committed and only the staged changes get committed.) - # In either cases, no error is returned to stderr like in (--only - # and without --only/--include) cases. In a similar manner, - # "git add -u baz" also does not error out. - # - # Therefore, the below test is just to document the current behavior - # and is not an endorsement to the current behavior, and we may - # want to fix this. And when that happens, this test should be - # updated accordingly. - test_must_fail git commit --include -m "baz" baz 2>err && - test_must_be_empty err + test_grep -e "$error" err ' test_expect_success 'setup: non-initial commit' ' From 7de13cfef3042478223012841e07cd91d7234d22 Mon Sep 17 00:00:00 2001 From: Ghanshyam Thakkar Date: Wed, 3 Apr 2024 23:44:52 +0530 Subject: [PATCH 3/3] builtin/add: error out when passing untracked path with -u When passing untracked path with -u option, it silently succeeds. There is no error message and the exit code is zero. This is inconsistent with other instances of git commands where the expected argument is a known path. In those other instances, we error out when the path is not known. Fix this by passing a character array to add_files_to_cache() to collect the pathspec matching information and report the error if a pathspec does not match any cache entry. Also add a testcase to cover this scenario. Signed-off-by: Ghanshyam Thakkar Signed-off-by: Junio C Hamano --- builtin/add.c | 9 ++++++++- t/t2200-add-update.sh | 10 ++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/builtin/add.c b/builtin/add.c index dc4b42d0ad..1937c19097 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -370,6 +370,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) int add_new_files; int require_pathspec; char *seen = NULL; + char *ps_matched = NULL; struct lock_file lock_file = LOCK_INIT; git_config(add_config, NULL); @@ -549,13 +550,18 @@ int cmd_add(int argc, const char **argv, const char *prefix) begin_odb_transaction(); + ps_matched = xcalloc(pathspec.nr, 1); if (add_renormalize) exit_status |= renormalize_tracked_files(&pathspec, flags); else exit_status |= add_files_to_cache(the_repository, prefix, - &pathspec, NULL, + &pathspec, ps_matched, include_sparse, flags); + if (take_worktree_changes && !add_renormalize && !ignore_add_errors && + report_path_error(ps_matched, &pathspec)) + exit(128); + if (add_new_files) exit_status |= add_files(&dir, flags); @@ -568,6 +574,7 @@ finish: COMMIT_LOCK | SKIP_IF_UNCHANGED)) die(_("unable to write new index file")); + free(ps_matched); dir_clear(&dir); clear_pathspec(&pathspec); return exit_status; diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh index c01492f33f..df235ac306 100755 --- a/t/t2200-add-update.sh +++ b/t/t2200-add-update.sh @@ -65,6 +65,16 @@ test_expect_success 'update did not touch untracked files' ' test_must_be_empty out ' +test_expect_success 'error out when passing untracked path' ' + git reset --hard && + echo content >>baz && + echo content >>top && + test_must_fail git add -u baz top 2>err && + test_grep -e "error: pathspec .baz. did not match any file(s) known to git" err && + git diff --cached --name-only >actual && + test_must_be_empty actual +' + test_expect_success 'cache tree has not been corrupted' ' git ls-files -s |