| Message ID | 20240402213640.139682-5-shyamthakkar001@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | commit, add: error out when passing untracked path | expand |
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > diff --git a/builtin/commit.c b/builtin/commit.c > index 8f31decc6b..09c48a835a 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -441,10 +441,17 @@ 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)) { > + free(ps_matched); > + exit(1); No need to free(ps_matched) immediately before exiting. There are other recources (like pathspec) we are holding and not clearing, and we do not want to bother cleaning them all. As we have another "if failed, die()" immediately after this hunk, adding another exit() would be OK. Shouldn't we be exiting with 128 to match what die() does, though? Other than that, looking good.
On Tue, 02 Apr 2024, Junio C Hamano <gitster@pobox.com> wrote: > Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > > > diff --git a/builtin/commit.c b/builtin/commit.c > > index 8f31decc6b..09c48a835a 100644 > > --- a/builtin/commit.c > > +++ b/builtin/commit.c > > @@ -441,10 +441,17 @@ 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)) { > > + free(ps_matched); > > + exit(1); > > No need to free(ps_matched) immediately before exiting. There are > other recources (like pathspec) we are holding and not clearing, and > we do not want to bother cleaning them all. Understood. > As we have another "if failed, die()" immediately after this hunk, > adding another exit() would be OK. Shouldn't we be exiting with 128 > to match what die() does, though? I tried to match the exit code with the existing invocations of the same when doing partial commit and reporting path errors. In builtin/commit.c: 511 if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec)) 512 exit(1); list_paths() returns the return value of report_path_error(). > Other than that, looking good. Thanks.
diff --git a/builtin/commit.c b/builtin/commit.c index 8f31decc6b..09c48a835a 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -441,10 +441,17 @@ 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)) { + free(ps_matched); + exit(1); + } + free(ps_matched); + 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/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' '
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 <shyamthakkar001@gmail.com> --- builtin/commit.c | 9 ++++++++- t/t7501-commit-basic-functionality.sh | 16 +--------------- 2 files changed, 9 insertions(+), 16 deletions(-)