[PATCH 0/2] fix certain cases of add and commit with untracked path not erroring out

[PATCH 0/2] fix certain cases of add and commit with untracked path not erroring out

[Ghanshyam Thakkar]

While adding tests for 'commit --include', I accidentally reproduced a
potential bug where in we do not error out when passing a pathspec which
does not match any tracked path. This was noticed by Junio while
reviewing[1]. This patch series fixes that and a similar case in 'add
--update'.

[1]: https://lore.kernel.org/git/xmqqil41avcc.fsf@gitster.g/

Ghanshyam Thakkar (2):
  builtin/commit: error out when passing untracked path with -i
  builtin/add: error out when passing untracked path with -u

 builtin/add.c                            | 16 ++++++++++++++++
 builtin/commit.c                         | 15 +++++++++++++++
 t/t1092-sparse-checkout-compatibility.sh |  4 ----
 t/t2200-add-update.sh                    |  5 +++++
 t/t7501-commit-basic-functionality.sh    | 16 +---------------
 5 files changed, 37 insertions(+), 19 deletions(-)

-- 
2.44.0

[PATCH 1/2] builtin/commit: error out when passing untracked path with -i

[Ghanshyam Thakkar]

Currently 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 was also described in the TODO comment before
the relevant testcase.

Fix this by matching the pathspec against index and report error if
any. And amend the relevant testcase and remove the TODO comment.
As this matches the pathspec against index, we need to also make sure
that the sparse index is expanded before matching the pathspec. A
side-effect of this is removal of --include related lines from the
testcase which checks if the sparse-index is expanded or not in t1092.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
[RFC]: I am still unsure about the removal of --include related lines
from the testcase which checks whether the index is expanded or not from
t1092. Will separating it into a separate testcase of its own and
marking that to expect failure be better?

 builtin/commit.c                         | 15 +++++++++++++++
 t/t1092-sparse-checkout-compatibility.sh |  4 ----
 t/t7501-commit-basic-functionality.sh    | 16 +---------------
 3 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a91197245f..f8f5909673 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -441,6 +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)) {
+		if (!all) {
+			int i, ret;
+			char *ps_matched = xcalloc(pathspec.nr, 1);
+
+			/* TODO: audit for interaction with sparse-index. */
+			ensure_full_index(&the_index);
+			for (i = 0; i < the_index.cache_nr; i++)
+				ce_path_match(&the_index, the_index.cache[i],
+					      &pathspec, ps_matched);
+
+			ret = report_path_error(ps_matched, &pathspec);
+			free(ps_matched);
+			if (ret)
+				exit(1);
+		}
 		repo_hold_locked_index(the_repository, &index_lock,
 				       LOCK_DIE_ON_ERROR);
 		add_files_to_cache(the_repository, also ? prefix : NULL,
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 2f1ae5fd3b..b55c81d4f7 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1418,10 +1418,6 @@ test_expect_success 'sparse-index is not expanded' '
 	ensure_not_expanded commit --allow-empty -m empty &&
 	echo >>sparse-index/a &&
 	ensure_not_expanded commit -a -m a &&
-	echo >>sparse-index/a &&
-	ensure_not_expanded commit --include a -m a &&
-	echo >>sparse-index/deep/deeper1/a &&
-	ensure_not_expanded commit --include deep/deeper1/a -m deeper &&
 	ensure_not_expanded checkout rename-out-to-out &&
 	ensure_not_expanded checkout - &&
 	ensure_not_expanded switch rename-out-to-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' '
-- 
2.44.0

Re: [PATCH 1/2] builtin/commit: error out when passing untracked path with -i

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> Currently 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 was also described in the TODO comment before
> the relevant testcase.

Drop "currently" (cf. https://lore.kernel.org/git/xmqqle6xbep5.fsf@gitster.g/)

> Fix this by matching the pathspec against index and report error if
> any. And amend the relevant testcase and remove the TODO comment.

> [RFC]: I am still unsure about the removal of --include related lines
> from the testcase which checks whether the index is expanded or not from
> t1092. Will separating it into a separate testcase of its own and
> marking that to expect failure be better?
>
>  builtin/commit.c                         | 15 +++++++++++++++
>  t/t1092-sparse-checkout-compatibility.sh |  4 ----
>  t/t7501-commit-basic-functionality.sh    | 16 +---------------
>  3 files changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index a91197245f..f8f5909673 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -441,6 +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)) {
> +		if (!all) {
> +			int i, ret;
> +			char *ps_matched = xcalloc(pathspec.nr, 1);
> +
> +			/* TODO: audit for interaction with sparse-index. */
> +			ensure_full_index(&the_index);
> +			for (i = 0; i < the_index.cache_nr; i++)
> +				ce_path_match(&the_index, the_index.cache[i],
> +					      &pathspec, ps_matched);
> +
> +			ret = report_path_error(ps_matched, &pathspec);
> +			free(ps_matched);
> +			if (ret)
> +				exit(1);
> +		}
>  		repo_hold_locked_index(the_repository, &index_lock,
>  				       LOCK_DIE_ON_ERROR);
>  		add_files_to_cache(the_repository, also ? prefix : NULL,

"git grep" for report_path_error() gives me four or five hits but
they way all of them populate ps_matched array are different [*],
so we cannot have a helper function to do so.

    Side note: They tend to do "looping over all the paths, see with
    ce_path_match() if the path matches, and do something with that
    path if it does".  They do not do a separate useless loop that
    is only for checking if all the pathspec elements match, like
    the loop in this patch does.

In a sense, not making this into a helper function is the right
thing to do.  It would avoid encouraging this anti-pattern of adding
a separate and otherwise useless loop.

We must already be using pathspec elements to decide to do the
"include" addition among all paths that we know about in some loop
separately, no?  Isn't that what the call to add_files_to_cache() we
see in the post-context doing?  Shouldn't that loop (probably the
one in diff-lib.c:run_diff_files(), that calls ce_path_match() for
each and every path we know about) be the one who needs to learn to
optionally collect the ps_matched information in addition to what it
is already doing?

Thanks.

[PATCH 2/2] builtin/add: error out when passing untracked path with -u

[Ghanshyam Thakkar]

Currently when we pass a pathspec which does not match any tracked path
along side --update, it silently succeeds, unlike without --update. As
--update only touches known paths, match the pathspec against the index
and error out when no match found. And ensure that the index is fully
expanded before matching the pathspec. Also add a testcase to check
for the error.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 builtin/add.c         | 16 ++++++++++++++++
 t/t2200-add-update.sh |  5 +++++
 2 files changed, 21 insertions(+)

diff --git a/builtin/add.c b/builtin/add.c
index 393c10cbcf..7ec5ea4a3e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -24,6 +24,7 @@
 #include "strvec.h"
 #include "submodule.h"
 #include "add-interactive.h"
+#include "sparse-index.h"
 
 static const char * const builtin_add_usage[] = {
 	N_("git add [<options>] [--] <pathspec>..."),
@@ -536,6 +537,21 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			}
 		}
 
+		if (take_worktree_changes && pathspec.nr) {
+			int i, ret;
+			char *ps_matched = xcalloc(pathspec.nr, 1);
+
+			/* TODO: audit for interaction with sparse-index. */
+			ensure_full_index(&the_index);
+			for (i = 0; i < the_index.cache_nr; i++)
+				ce_path_match(&the_index, the_index.cache[i],
+					      &pathspec, ps_matched);
+
+			ret = report_path_error(ps_matched, &pathspec);
+			free(ps_matched);
+			if (ret)
+				exit(1);
+		}
 
 		if (only_match_skip_worktree.nr) {
 			advise_on_updating_sparse_paths(&only_match_skip_worktree);
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index c01492f33f..f6a9615d1b 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -65,6 +65,11 @@ test_expect_success 'update did not touch untracked files' '
 	test_must_be_empty out
 '
 
+test_expect_success 'error out when given untracked path' '
+	test_must_fail git add -u dir2/other 2>err &&
+	test_grep -e "error: pathspec .dir2/other. did not match any file(s) known to git" err
+'
+
 test_expect_success 'cache tree has not been corrupted' '
 
 	git ls-files -s |
-- 
2.44.0

Re: [PATCH 2/2] builtin/add: error out when passing untracked path with -u

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> Currently when we pass a pathspec which does not match any tracked path
> along side --update, it silently succeeds, unlike without --update. As
> --update only touches known paths, match the pathspec against the index
> and error out when no match found. And ensure that the index is fully
> expanded before matching the pathspec. Also add a testcase to check
> for the error.
>
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
>  builtin/add.c         | 16 ++++++++++++++++
>  t/t2200-add-update.sh |  5 +++++
>  2 files changed, 21 insertions(+)

Exactly the same comment applies here.  If we are using pathspec, we
should already have a loop that calls ce_path_match() for each and
every path we know about, and we should be updating the code to
collect "have we used all pathspec elements?" information at the
same time if it is not doing so already.  Let's not make another
loop that checks what we should already be doing elsewhere.

Thanks.

> diff --git a/builtin/add.c b/builtin/add.c
> index 393c10cbcf..7ec5ea4a3e 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -24,6 +24,7 @@
>  #include "strvec.h"
>  #include "submodule.h"
>  #include "add-interactive.h"
> +#include "sparse-index.h"
>  
>  static const char * const builtin_add_usage[] = {
>  	N_("git add [<options>] [--] <pathspec>..."),
> @@ -536,6 +537,21 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  			}
>  		}
>  
> +		if (take_worktree_changes && pathspec.nr) {
> +			int i, ret;
> +			char *ps_matched = xcalloc(pathspec.nr, 1);
> +
> +			/* TODO: audit for interaction with sparse-index. */
> +			ensure_full_index(&the_index);
> +			for (i = 0; i < the_index.cache_nr; i++)
> +				ce_path_match(&the_index, the_index.cache[i],
> +					      &pathspec, ps_matched);
> +
> +			ret = report_path_error(ps_matched, &pathspec);
> +			free(ps_matched);
> +			if (ret)
> +				exit(1);
> +		}
>  
>  		if (only_match_skip_worktree.nr) {
>  			advise_on_updating_sparse_paths(&only_match_skip_worktree);
> diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
> index c01492f33f..f6a9615d1b 100755
> --- a/t/t2200-add-update.sh
> +++ b/t/t2200-add-update.sh
> @@ -65,6 +65,11 @@ test_expect_success 'update did not touch untracked files' '
>  	test_must_be_empty out
>  '
>  
> +test_expect_success 'error out when given untracked path' '
> +	test_must_fail git add -u dir2/other 2>err &&
> +	test_grep -e "error: pathspec .dir2/other. did not match any file(s) known to git" err
> +'
> +
>  test_expect_success 'cache tree has not been corrupted' '
>  
>  	git ls-files -s |

[PATCH v2 0/3] commit, add: error out when passing untracked path

[Ghanshyam Thakkar]

Fix 'commit -i' and 'add -u', which expect known paths, not erroring
out when passing untracked paths.

The first patch introduces a new parameter to add_files_to_cache() and
run_diff_files() to optionally collect pathspec matching info for use
in reporting error. And the second and third patch use this parameter
to collect pathspec matching info and report error when passing
untracked paths to 'git commit -i' and 'git add -u' respectively.

Ghanshyam Thakkar (3):
  optionally collect pathspec matching info
  builtin/commit: error out when passing untracked path with -i
  builtin/add: error out when passing untracked path with -u

 add-interactive.c                     |  2 +-
 builtin/add.c                         | 13 ++++++++++---
 builtin/checkout.c                    |  3 ++-
 builtin/commit.c                      |  9 ++++++++-
 builtin/diff-files.c                  |  2 +-
 builtin/diff.c                        |  2 +-
 builtin/merge.c                       |  2 +-
 builtin/stash.c                       |  2 +-
 builtin/submodule--helper.c           |  4 ++--
 diff-lib.c                            |  5 +++--
 diff.h                                |  3 ++-
 read-cache-ll.h                       |  4 ++--
 read-cache.c                          |  6 +++---
 t/t2200-add-update.sh                 |  6 ++++++
 t/t7501-commit-basic-functionality.sh | 16 +---------------
 wt-status.c                           |  6 +++---
 16 files changed, 47 insertions(+), 38 deletions(-)

-- 
2.44.0

[PATCH v3 0/3] commit, add: error out when passing untracked paths

[Ghanshyam Thakkar]

This version uses Junio's patch ,which adds a new .ps_matched member to
'strcut rev_info' to record the pathspec elements that matched tracked
paths, as base for patch [2/3] and [3/3]. 

Patch [2/3] remains unchanged from v2. And patch [3/3] is updated
according to Junio's suggestions.

Ghanshyam Thakkar (2):
  builtin/commit: error out when passing untracked path with -i
  builtin/add: error out when passing untracked path with -u

Junio C Hamano (1):
  revision: optionally record matches with pathspec elements

 builtin/add.c                         | 13 +++++++++++--
 builtin/checkout.c                    |  3 ++-
 builtin/commit.c                      |  9 ++++++++-
 diff-lib.c                            | 11 ++++++++++-
 read-cache-ll.h                       |  4 ++--
 read-cache.c                          |  8 +++++---
 revision.h                            |  1 +
 t/t2200-add-update.sh                 | 10 ++++++++++
 t/t7501-commit-basic-functionality.sh | 16 +---------------
 9 files changed, 50 insertions(+), 25 deletions(-)

-- 
2.44.0

[PATCH v4 0/3] commit,add: error out when passing untracked path

[Ghanshyam Thakkar]

This version makes the changes as suggested by Junio. In particular,
changed exit codes from 1 to 128. And, removed unnecessary free()
calls immediately before exiting.

Ghanshyam Thakkar (2):
  builtin/commit: error out when passing untracked path with -i
  builtin/add: error out when passing untracked path with -u

Junio C Hamano (1):
  revision: optionally record matches with pathspec elements

 builtin/add.c                         | 11 +++++++++--
 builtin/checkout.c                    |  3 ++-
 builtin/commit.c                      |  7 ++++++-
 diff-lib.c                            | 11 ++++++++++-
 read-cache-ll.h                       |  4 ++--
 read-cache.c                          |  8 +++++---
 revision.h                            |  1 +
 t/t2200-add-update.sh                 | 10 ++++++++++
 t/t7501-commit-basic-functionality.sh | 16 +---------------
 9 files changed, 46 insertions(+), 25 deletions(-)

-- 
2.44.0

[PATCH v4 1/3] revision: optionally record matches with pathspec elements

[Ghanshyam Thakkar]

From: Junio C Hamano <gitster@pobox.com>

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 <pathspec>" 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 <gitster@pobox.com>
---
 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 2b6166c284..c297aa0e32 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -882,7 +882,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;
 
 	/*
-- 
2.44.0

[PATCH v4 2/3] builtin/commit: error out when passing untracked path with -i

[Ghanshyam Thakkar]

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

[PATCH v4 3/3] builtin/add: error out when passing untracked path with -u

[Ghanshyam Thakkar]

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 <shyamthakkar001@gmail.com>
---
 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 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			       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 |
-- 
2.44.0

[PATCH v3 1/3] revision: optionally record matches with pathspec elements

[Ghanshyam Thakkar]

From: Junio C Hamano <gitster@pobox.com>

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 <pathspec>" 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 <gitster@pobox.com>
---
 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 2b6166c284..c297aa0e32 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -882,7 +882,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;
 
 	/*
-- 
2.44.0

[PATCH v3 2/3] builtin/commit: error out when passing untracked path with -i

[Ghanshyam Thakkar]

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(-)

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

Re: [PATCH v3 2/3] builtin/commit: error out when passing untracked path with -i

[Junio C Hamano]

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.

Re: [PATCH v3 2/3] builtin/commit: error out when passing untracked path with -i

[Ghanshyam Thakkar]

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.

[PATCH v3 3/3] builtin/add: error out when passing untracked path with -u

[Ghanshyam Thakkar]

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 and
exit if a pathspec does not match any cache entry. Also add a testcase
to cover this scenario.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 builtin/add.c         | 11 ++++++++++-
 t/t2200-add-update.sh | 10 ++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index dc4b42d0ad..88261b0f2b 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,20 @@ 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 &&
+	    report_path_error(ps_matched, &pathspec)) {
+		free(ps_matched);
+		exit(1);
+	}
+
 	if (add_new_files)
 		exit_status |= add_files(&dir, flags);
 
@@ -568,6 +576,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			       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 |
-- 
2.44.0

Re: [PATCH v3 3/3] builtin/add: error out when passing untracked path with -u

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> 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 and
> exit if a pathspec does not match any cache entry. Also add a testcase
> to cover this scenario.
>
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
>  builtin/add.c         | 11 ++++++++++-
>  t/t2200-add-update.sh | 10 ++++++++++
>  2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index dc4b42d0ad..88261b0f2b 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,20 @@ 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 &&
> +	    report_path_error(ps_matched, &pathspec)) {
> +		free(ps_matched);
> +		exit(1);
> +	}

Shouldn't we pay attention to ignore_add_errors?  The same comments
about free'ing and exit code from the review on the previous step
apply here, too.

Other than that, looking good.

Re: [PATCH v3 3/3] builtin/add: error out when passing untracked path with -u

[Ghanshyam Thakkar]

On Tue, 02 Apr 2024, Junio C Hamano <gitster@pobox.com> wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> 
> > 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 and
> > exit if a pathspec does not match any cache entry. Also add a testcase
> > to cover this scenario.
> >
> > Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> > ---
> >  builtin/add.c         | 11 ++++++++++-
> >  t/t2200-add-update.sh | 10 ++++++++++
> >  2 files changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/add.c b/builtin/add.c
> > index dc4b42d0ad..88261b0f2b 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,20 @@ 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 &&
> > +	    report_path_error(ps_matched, &pathspec)) {
> > +		free(ps_matched);
> > +		exit(1);
> > +	}
> 
> Shouldn't we pay attention to ignore_add_errors?  The same comments
> about free'ing and exit code from the review on the previous step
> apply here, too.

Will update.

> Other than that, looking good.

Thanks.

[PATCH v2 1/3] read-cache: optionally collect pathspec matching info

[Ghanshyam Thakkar]

The add_files_to_cache() adds files to the index. And
add_files_to_cache() in turn calls run_diff_files() to perform this
operation. The run_diff_files() uses ce_path_match() to match the
pathspec against cache entries. However, it is called with NULL value
for the seen parameter, which collects the pathspec matching
information.

Therefore, introduce a new parameter 'char *ps_matched' to 
add_files_to_cache() and in turn to run_diff_files(), to feed it to
ce_path_match() to optionally collect the pathspec matching
information. This will be helpful in reporting error in case of an
untracked path being passed when the expectation is a known path. Thus,
this will be used in the subsequent commits to fix 'commit -i' and 'add
-u' not erroring out when given untracked paths.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 add-interactive.c           | 2 +-
 builtin/add.c               | 6 +++---
 builtin/checkout.c          | 3 ++-
 builtin/commit.c            | 2 +-
 builtin/diff-files.c        | 2 +-
 builtin/diff.c              | 2 +-
 builtin/merge.c             | 2 +-
 builtin/stash.c             | 2 +-
 builtin/submodule--helper.c | 4 ++--
 diff-lib.c                  | 5 +++--
 diff.h                      | 3 ++-
 read-cache-ll.h             | 4 ++--
 read-cache.c                | 6 +++---
 wt-status.c                 | 6 +++---
 14 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index 6bf87e7ae7..b33260a611 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -572,7 +572,7 @@ static int get_modified_files(struct repository *r,
 			run_diff_index(&rev, DIFF_INDEX_CACHED);
 		else {
 			rev.diffopt.flags.ignore_dirty_submodules = 1;
-			run_diff_files(&rev, 0);
+			run_diff_files(&rev, NULL, 0);
 		}
 
 		release_revisions(&rev);
diff --git a/builtin/add.c b/builtin/add.c
index 393c10cbcf..ffe5fd8d44 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -191,7 +191,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 	out = xopen(file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
 	rev.diffopt.file = xfdopen(out, "w");
 	rev.diffopt.close_file = 1;
-	run_diff_files(&rev, 0);
+	run_diff_files(&rev, NULL, 0);
 
 	if (launch_editor(file, NULL, NULL))
 		die(_("editing patch failed"));
@@ -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 902c97ab23..02bd035081 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -876,7 +876,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 a91197245f..24efeaca98 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/builtin/diff-files.c b/builtin/diff-files.c
index 018011f29e..8559aa254c 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -81,7 +81,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 
 	if (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0)
 		die_errno("repo_read_index_preload");
-	run_diff_files(&rev, options);
+	run_diff_files(&rev, NULL, options);
 	result = diff_result_code(&rev.diffopt);
 	release_revisions(&rev);
 	return result;
diff --git a/builtin/diff.c b/builtin/diff.c
index 6e196e0c7d..3e9b838bdd 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -283,7 +283,7 @@ static void builtin_diff_files(struct rev_info *revs, int argc, const char **arg
 				    0) < 0) {
 		die_errno("repo_read_index_preload");
 	}
-	run_diff_files(revs, options);
+	run_diff_files(revs, NULL, options);
 }
 
 struct symdiff {
diff --git a/builtin/merge.c b/builtin/merge.c
index a0ba1f9815..4b4c1d6a31 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -979,7 +979,7 @@ static int evaluate_result(void)
 		DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = count_diff_files;
 	rev.diffopt.format_callback_data = &cnt;
-	run_diff_files(&rev, 0);
+	run_diff_files(&rev, NULL, 0);
 
 	/*
 	 * Check how many unmerged entries are
diff --git a/builtin/stash.c b/builtin/stash.c
index 7fb355bff0..2c00026390 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1121,7 +1121,7 @@ static int check_changes_tracked_files(const struct pathspec *ps)
 		goto done;
 	}
 
-	run_diff_files(&rev, 0);
+	run_diff_files(&rev, NULL, 0);
 	if (diff_result_code(&rev.diffopt)) {
 		ret = 1;
 		goto done;
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fda50f2af1..e9047021e0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -667,7 +667,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 	repo_init_revisions(the_repository, &rev, NULL);
 	rev.abbrev = 0;
 	setup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt);
-	run_diff_files(&rev, 0);
+	run_diff_files(&rev, NULL, 0);
 
 	if (!diff_result_code(&rev.diffopt)) {
 		print_status(flags, ' ', path, ce_oid,
@@ -1141,7 +1141,7 @@ static int compute_summary_module_list(struct object_id *head_oid,
 	if (diff_cmd == DIFF_INDEX)
 		run_diff_index(&rev, info->cached ? DIFF_INDEX_CACHED : 0);
 	else
-		run_diff_files(&rev, 0);
+		run_diff_files(&rev, NULL, 0);
 	prepare_submodule_summary(info, &list);
 cleanup:
 	strvec_clear(&diff_args);
diff --git a/diff-lib.c b/diff-lib.c
index 5e8717c774..2dc3864abd 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -101,7 +101,8 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
 	return changed;
 }
 
-void run_diff_files(struct rev_info *revs, unsigned int option)
+void run_diff_files(struct rev_info *revs, char *ps_matched,
+		    unsigned int option)
 {
 	int entries, i;
 	int diff_unmerged_stage = revs->max_count;
@@ -127,7 +128,7 @@ 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))
+		if (!ce_path_match(istate, ce, &revs->prune_data, ps_matched))
 			continue;
 
 		if (revs->diffopt.prefix &&
diff --git a/diff.h b/diff.h
index 66bd8aeb29..a01feaa586 100644
--- a/diff.h
+++ b/diff.h
@@ -638,7 +638,8 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);
 #define DIFF_SILENT_ON_REMOVED 01
 /* report racily-clean paths as modified */
 #define DIFF_RACY_IS_MODIFIED 02
-void run_diff_files(struct rev_info *revs, unsigned int option);
+void run_diff_files(struct rev_info *revs, char *ps_matched,
+		    unsigned int option);
 
 #define DIFF_INDEX_CACHED 01
 #define DIFF_INDEX_MERGE_BASE 02
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..e179444445 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;
@@ -3985,7 +3985,7 @@ int add_files_to_cache(struct repository *repo, const char *prefix,
 	 * may not have their own transaction active.
 	 */
 	begin_odb_transaction();
-	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
+	run_diff_files(&rev, ps_matched, DIFF_RACY_IS_MODIFIED);
 	end_odb_transaction();
 
 	release_revisions(&rev);
diff --git a/wt-status.c b/wt-status.c
index 2db4bb3a12..cf6d61e60c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -629,7 +629,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
 	rev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;
 	rev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;
 	copy_pathspec(&rev.prune_data, &s->pathspec);
-	run_diff_files(&rev, 0);
+	run_diff_files(&rev, NULL, 0);
 	release_revisions(&rev);
 }
 
@@ -1173,7 +1173,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
 		setup_work_tree();
 		rev.diffopt.a_prefix = "i/";
 		rev.diffopt.b_prefix = "w/";
-		run_diff_files(&rev, 0);
+		run_diff_files(&rev, NULL, 0);
 	}
 	release_revisions(&rev);
 }
@@ -2594,7 +2594,7 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules)
 	}
 	rev_info.diffopt.flags.quick = 1;
 	diff_setup_done(&rev_info.diffopt);
-	run_diff_files(&rev_info, 0);
+	run_diff_files(&rev_info, NULL, 0);
 	result = diff_result_code(&rev_info.diffopt);
 	release_revisions(&rev_info);
 	return result;
-- 
2.44.0

Re: [PATCH v2 1/3] read-cache: optionally collect pathspec matching info

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> The add_files_to_cache() adds files to the index. And
> add_files_to_cache() in turn calls run_diff_files() to perform this
> operation. The run_diff_files() uses ce_path_match() to match the
> pathspec against cache entries. However, it is called with NULL value
> for the seen parameter, which collects the pathspec matching
> information.

", which collects" -> ", which means we lose"

> Therefore, introduce a new parameter 'char *ps_matched' to 

"Therefore, introduce" -> "Introduce"

> add_files_to_cache() and in turn to run_diff_files(), to feed it to
> ce_path_match() to optionally collect the pathspec matching
> information. This will be helpful in reporting error in case of an
> untracked path being passed when the expectation is a known path. Thus,
> this will be used in the subsequent commits to fix 'commit -i' and 'add
> -u' not erroring out when given untracked paths.

A new parameter to run_diff_files() came as a bit of surprise to me.

When I responded to the previous round, I somehow thought that we'd
add a new member to the rev structure that points at an optional
.ps_matched member next to the existing .prune_data member.  

That way, it would hopefully be easy for a future code to see if a
"diff" invocation, not necessarily run_diff_files() that compares
the working tree and the index, consumed all the pathspec elements.
If such a new .ps_matched member is initialized to NULL, all the
patch noise we see in this patch will become unnecessary, no?

> diff --git a/diff-lib.c b/diff-lib.c
> index 5e8717c774..2dc3864abd 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -101,7 +101,8 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
>  	return changed;
>  }
>  
> -void run_diff_files(struct rev_info *revs, unsigned int option)
> +void run_diff_files(struct rev_info *revs, char *ps_matched,
> +		    unsigned int option)
>  {
>  	int entries, i;
>  	int diff_unmerged_stage = revs->max_count;
> @@ -127,7 +128,7 @@ 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))
> +		if (!ce_path_match(istate, ce, &revs->prune_data, ps_matched))
>  			continue;
>  
>  		if (revs->diffopt.prefix &&

This may be a non-issue, but after this point we see the beginning
of another filter to reject paths outside the hierarchy "--relative"
specifies.  It is possible that a pathspec element matches ce->name
but the matched cache entry is outside the current area.  Shouldn't
we then consider that the pathspec element did not match?  E.g., in
our project, what should happen if we did this?

    $ echo >>diff.h
    $ cd t
    $ git diff --relative \*.h

The command should show nothing.  Did the pathspec '*.h' match?  From
those who know how the machinery works, yes it did before the resulting
paths were further filtered out, but from the end-user's point of view,
because "--relative" limits the diff to the current directory and below,
and because 't' and below did not have any C header files, wouldn't it
be more natural and useful to say the pathspec wasn't used?

This does not matter right now because we are not planning to add a
new "--error-unmatch" option to "git diff", but when/if we do, it
starts to matter.  The hunk at least needs a NEEDSWORK comment,
summarizing the above.

	/*
	 * 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".
	 */ 

A solution to that problem might be just a matter of swapping the
order of filtering, but it may have performance implications and I'd
rather not have to worry about it right now in the context of the
current topic, hence a NEEDSWORK comment without attempting to "fix"
it would be the most preferred approach to such a side issue.

Re: [PATCH v2 1/3] read-cache: optionally collect pathspec matching info

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> A new parameter to run_diff_files() came as a bit of surprise to me.
>
> When I responded to the previous round, I somehow thought that we'd
> add a new member to the rev structure that points at an optional
> .ps_matched member next to the existing .prune_data member.  
>
> That way, it would hopefully be easy for a future code to see if a
> "diff" invocation, not necessarily run_diff_files() that compares
> the working tree and the index, consumed all the pathspec elements.
> If such a new .ps_matched member is initialized to NULL, all the
> patch noise we see in this patch will become unnecessary, no?

This is how such a change may look like.  After applying [2/3] and
[3/3] steps from your series on top of this patch, the updated tests
in your series (2200 and 7501) seem to still pass.

------- >8 ------------- >8 ------------- >8 ------------- >8 -------

Subject: [PATCH] revision: optionally record matches with pathspec elements

Unlike "git add" and other end-user facing command, 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 does 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 <pathspec>" 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 <gitster@pobox.com>
---
 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;
 
 	/*
-- 
2.44.0-413-gd6fd04375f

Re: [PATCH v2 1/3] read-cache: optionally collect pathspec matching info

[Ghanshyam Thakkar]

On Fri, 29 Mar 2024, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > A new parameter to run_diff_files() came as a bit of surprise to me.
> >
> > When I responded to the previous round, I somehow thought that we'd
> > add a new member to the rev structure that points at an optional
> > .ps_matched member next to the existing .prune_data member.  
> >
> > That way, it would hopefully be easy for a future code to see if a
> > "diff" invocation, not necessarily run_diff_files() that compares
> > the working tree and the index, consumed all the pathspec elements.
> > If such a new .ps_matched member is initialized to NULL, all the
> > patch noise we see in this patch will become unnecessary, no?
> 
> This is how such a change may look like.  After applying [2/3] and
> [3/3] steps from your series on top of this patch, the updated tests
> in your series (2200 and 7501) seem to still pass.

This seems perfect. I hope you're OK with me using this patch as a base
for patch [2/3] and [3/3]. :)

> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
> 
> Subject: [PATCH] revision: optionally record matches with pathspec elements
> 
> Unlike "git add" and other end-user facing command, 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 does 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 <pathspec>" 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 <gitster@pobox.com>
> ---
>  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;
>  
>  	/*
> -- 
> 2.44.0-413-gd6fd04375f
> 

Re: [PATCH v2 1/3] read-cache: optionally collect pathspec matching info

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

>> This is how such a change may look like.  After applying [2/3] and
>> [3/3] steps from your series on top of this patch, the updated tests
>> in your series (2200 and 7501) seem to still pass.
>
> This seems perfect. I hope you're OK with me using this patch as a base
> for patch [2/3] and [3/3]. :)

Yes, as long as you promise to fix typos and grammatical mistakes in
my proposed log messages (there are several I just noticed X-<).

Thanks.

[PATCH v2 2/3] builtin/commit: error out when passing untracked path with -i

[Ghanshyam Thakkar]

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(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 24efeaca98..355f25ec2a 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' '
-- 
2.44.0

Re: [PATCH v2 2/3] builtin/commit: error out when passing untracked path with -i

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> 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(-)

Nice.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 24efeaca98..355f25ec2a 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);
> +

Looking simple and very nice.

This change would not have to be redone even if we decide not to add
a new parameter to run_diff_files() and instead add a new member to
the revs structure instead, because it all happens at the level or
below add_files_to_cache().

[PATCH v2 3/3] builtin/add: error out when passing untracked path with -u

[Ghanshyam Thakkar]

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.

Therefore, 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 <shyamthakkar001@gmail.com>
---
 builtin/add.c         | 9 ++++++++-
 t/t2200-add-update.sh | 6 ++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index ffe5fd8d44..650432bb13 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);
@@ -547,15 +548,20 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		string_list_clear(&only_match_skip_worktree, 0);
 	}
 
+
 	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)
+		exit_status |= report_path_error(ps_matched, &pathspec);
+
 	if (add_new_files)
 		exit_status |= add_files(&dir, flags);
 
@@ -568,6 +574,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 			       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..7cba325f08 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -65,6 +65,12 @@ test_expect_success 'update did not touch untracked files' '
 	test_must_be_empty out
 '
 
+test_expect_success 'error out when passing untracked path' '
+	echo content >baz &&
+	test_must_fail git add -u baz 2>err &&
+	test_grep -e "error: pathspec .baz. did not match any file(s) known to git" err
+'
+
 test_expect_success 'cache tree has not been corrupted' '
 
 	git ls-files -s |
-- 
2.44.0

Re: [PATCH v2 3/3] builtin/add: error out when passing untracked path with -u

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> 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.
>
> Therefore, fix this by passing a character array to

"Therefore, fix" -> "Fix".

> 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 <shyamthakkar001@gmail.com>
> ---
>  builtin/add.c         | 9 ++++++++-
>  t/t2200-add-update.sh | 6 ++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index ffe5fd8d44..650432bb13 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);
> @@ -547,15 +548,20 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  		string_list_clear(&only_match_skip_worktree, 0);
>  	}
>  
> +
>  	begin_odb_transaction();

Unnecessary change.

> +	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)
> +		exit_status |= report_path_error(ps_matched, &pathspec);

Hmph, are we sure take_worktree_changes is true only when
add_renormalize is false?

>  	if (add_new_files)
>  		exit_status |= add_files(&dir, flags);

If report_path_error() detected that the pathspec were faulty,
should we still proceed to add new files?  This is NOT a rhetorical
question, as I do not know the answer myself.  I do not even know
offhand what add_files_to_cache() above did when pathspec elements
are not all consumed---if it does not complain and does not refrain
from doing any change to the index, then we should follow suite and
add_files() here, too.

> @@ -568,6 +574,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  			       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..7cba325f08 100755
> --- a/t/t2200-add-update.sh
> +++ b/t/t2200-add-update.sh
> @@ -65,6 +65,12 @@ test_expect_success 'update did not touch untracked files' '
>  	test_must_be_empty out
>  '
>  
> +test_expect_success 'error out when passing untracked path' '
> +	echo content >baz &&
> +	test_must_fail git add -u baz 2>err &&
> +	test_grep -e "error: pathspec .baz. did not match any file(s) known to git" err
> +'
> +
>  test_expect_success 'cache tree has not been corrupted' '
>  
>  	git ls-files -s |

Re: [PATCH v2 3/3] builtin/add: error out when passing untracked path with -u

[Ghanshyam Thakkar]

On Fri, 29 Mar 2024, Junio C Hamano <gitster@pobox.com> wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> > +	if (take_worktree_changes)
> > +		exit_status |= report_path_error(ps_matched, &pathspec);
> 
> Hmph, are we sure take_worktree_changes is true only when
> add_renormalize is false?
> 
> >  	if (add_new_files)
> >  		exit_status |= add_files(&dir, flags);
> 
> If report_path_error() detected that the pathspec were faulty,
> should we still proceed to add new files?  This is NOT a rhetorical
> question, as I do not know the answer myself.  I do not even know
> offhand what add_files_to_cache() above did when pathspec elements
> are not all consumed---if it does not complain and does not refrain
> from doing any change to the index, then we should follow suite and
> add_files() here, too.
Sorry if I'm missing something, but in your last line after '---', do you mean
that we should proceed even after report_path_error() detected error like in
the above patch or perhaps something like this:

diff --git a/builtin/add.c b/builtin/add.c
index dc4b42d0ad..eccda485ed 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -64,7 +64,8 @@ static int chmod_pathspec(struct pathspec *pathspec, char flip, int show_only)
        return ret;
 }
 
-static int renormalize_tracked_files(const struct pathspec *pathspec, int flags)
+static int renormalize_tracked_files(const struct pathspec *pathspec,
+                                    char *ps_matched, int flags)
 {
        int i, retval = 0;
 
@@ -79,7 +80,8 @@ static int renormalize_tracked_files(const struct pathspec *pathspec, int flags)
                        continue; /* do not touch unmerged paths */
                if (!S_ISREG(ce->ce_mode) && !S_ISLNK(ce->ce_mode))
                        continue; /* do not touch non blobs */
-               if (pathspec && !ce_path_match(&the_index, ce, pathspec, NULL))
+               if (pathspec &&
+                   !ce_path_match(&the_index, ce, pathspec, ps_matched))
                        continue;
                retval |= add_file_to_index(&the_index, ce->name,
                                            flags | ADD_CACHE_RENORMALIZE);
@@ -370,7 +372,9 @@ 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;
+       struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
 
        git_config(add_config, NULL);
 
@@ -487,7 +491,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
        if (pathspec.nr) {
                int i;
                char *skip_worktree_seen = NULL;
-               struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
 
                if (!seen)
                        seen = find_pathspecs_matching_against_index(&pathspec,
@@ -544,18 +547,26 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
                free(seen);
                free(skip_worktree_seen);
-               string_list_clear(&only_match_skip_worktree, 0);
        }
 
        begin_odb_transaction();
 
+       ps_matched = xcalloc(pathspec.nr, 1);
        if (add_renormalize)
-               exit_status |= renormalize_tracked_files(&pathspec, flags);
+               exit_status |=
+                       renormalize_tracked_files(&pathspec, ps_matched, 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 && !only_match_skip_worktree.nr)) &&
                                                  include_sparse, flags);
 
+       if ((take_worktree_changes ||
+            (add_renormalize && !only_match_skip_worktree.nr)) &&
+           report_path_error(ps_matched, &pathspec)) {
+               exit_status = 1;
+               goto cleanup;
+       }
+
        if (add_new_files)
                exit_status |= add_files(&dir, flags);
 
@@ -568,6 +579,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
                               COMMIT_LOCK | SKIP_IF_UNCHANGED))
                die(_("unable to write new index file"));
 
+cleanup:
+       string_list_clear(&only_match_skip_worktree, 0);
+       free(ps_matched);
        dir_clear(&dir);
        clear_pathspec(&pathspec);
        return exit_status;

Although I'm not sure if we should flush_odb_transaction() in the
cleanup, because end_odb_transaction() would not be called if we go
straight to cleanup.

Re: [PATCH v2 3/3] builtin/add: error out when passing untracked path with -u

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> On Fri, 29 Mar 2024, Junio C Hamano <gitster@pobox.com> wrote:
>> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>> > +	if (take_worktree_changes)
>> > +		exit_status |= report_path_error(ps_matched, &pathspec);
>> 
>> Hmph, are we sure take_worktree_changes is true only when
>> add_renormalize is false?
>> 
>> >  	if (add_new_files)
>> >  		exit_status |= add_files(&dir, flags);
>> 
>> If report_path_error() detected that the pathspec were faulty,
>> should we still proceed to add new files?  This is NOT a rhetorical
>> question, as I do not know the answer myself.  I do not even know
>> offhand what add_files_to_cache() above did when pathspec elements
>> are not all consumed---if it does not complain and does not refrain
>> from doing any change to the index, then we should follow suite and
>> add_files() here, too.
> Sorry if I'm missing something, but in your last line after '---', do you mean
> that we should proceed even after report_path_error() detected error like in
> the above patch or perhaps something like this:

We roughly do:

	if (add_renorm)
		exit_status |= renorm();
	else
		exit_status |= add_files_to_cache();
+	if (take_worktree_changes)
+		exit_status |= report_path_error();
	if (add_new_files)
		exit_status |= add_files();

I was wondering if we should refrain from adding new files when we
exit_status is true to avoid making "further damage", and was
wondering if the last one should become:

	if (!exit_status && add_new_files)
		exit_status |= add_files();

But that was merely because I was not thinking things through.  If
we were to go that route, the whole thing needs to become (because
there are other things that notice errors before this part of the
code):
	
	if (!exit_status) {
		if (add_renorm)
			exit_status |= renorm();
		else
                	exit_status |= add_files_to_cache();
	}
	if (!exit_status && take_worktree_changes)
		exit_status |= report_path_error();

	if (!exit_status && add_new_files)
		exit_status |= add_files();

but (1) that is far bigger change of behaviour to the code than
suitable for "notice unmatched pathspec elements and report an
error" topic, and more importantly (2) it is still not sufficient to
make it "all-or-none". E.g., if "add_files_to_cache()" call added
contents from a few paths and then noticed that some pathspec
elements were not used, we are not restoring the previous state to
recover.  The damage is already done, and not making further damage
does not help the user all that much.

So, it was a fairly pointless thing that I was wondering about.  The
current behaviour, and the new behaviour with the new check, are
fine as-is.

If we wanted to make it "all-or-none", I think the way to do so is
to tweak the final part of the cmd_add() function to skip committing
the updated index, e.g.,

         finish:
        -	if (write_locked_index(&the_index, &lock_file,
        +	if (exit_status)
        +		fputs(_("not updating the index due to failure(s)\n"), stderr);
        +	else if (write_locked_index(&the_index, &lock_file,
                                       COMMIT_LOCK | SKIP_IF_UNCHANGED))
                        die(_("unable to write new index file"));
 
And if/when we do so, the existing code (with or without the updates
made by the topic under discussion) needs no change.  We can do all
steps regardless of the errors we notice along the way with earlier
steps, and discard the in-core index if we saw any errors.

The renormalize() thing is not noticing unused pathspec elements,
which we might want to fix, but I suspect it is far less commonly
used mode of operation, so it may be OK to leave it to future
follow-up series.

Thanks.

Re: [PATCH v2 3/3] builtin/add: error out when passing untracked path with -u

[Ghanshyam Thakkar]

On Sat, 30 Mar 2024, Junio C Hamano <gitster@pobox.com> wrote:
> So, it was a fairly pointless thing that I was wondering about.  The
> current behaviour, and the new behaviour with the new check, are
> fine as-is.

Well I think we should be going 'all-or-none' way as I can't think of
any major user-facing command that does partial changes incase of
error (besides two testcase below).

> If we wanted to make it "all-or-none", I think the way to do so is
> to tweak the final part of the cmd_add() function to skip committing
> the updated index, e.g.,
> 
>          finish:
>         -	if (write_locked_index(&the_index, &lock_file,
>         +	if (exit_status)
>         +		fputs(_("not updating the index due to failure(s)\n"), stderr);
>         +	else if (write_locked_index(&the_index, &lock_file,
>                                        COMMIT_LOCK | SKIP_IF_UNCHANGED))
>                         die(_("unable to write new index file"));
>  
> And if/when we do so, the existing code (with or without the updates
> made by the topic under discussion) needs no change.  We can do all
> steps regardless of the errors we notice along the way with earlier
> steps, and discard the in-core index if we saw any errors.

Doing this, we would need to take care of atleast 4 tests breaking in
t3700-add:
 error out when attempting to add ignored ones but add others
 git add --ignore-errors
 git add (add.ignore-errors)
 git add --chmod fails with non regular files (but updates the other paths)

while ignore-errors ones would be trivial to fix, fixing other 2 would
probably require some more than trivial code changes, as from the title,
their behavior seems pretty much set in stone. That's why I did the
'goto cleanup' approach to not break these.

Thanks.

> The renormalize() thing is not noticing unused pathspec elements,
> which we might want to fix, but I suspect it is far less commonly
> used mode of operation, so it may be OK to leave it to future
> follow-up series.
> 
> Thanks.

Re: [PATCH v2 3/3] builtin/add: error out when passing untracked path with -u

[Junio C Hamano]

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> Well I think we should be going 'all-or-none' way as I can't think of
> any major user-facing command that does partial changes incase of
> error (besides two testcase below).

I agree that in the longer run, all-or-none would be something we
should aim for, but I'd strongly prefer leaving that outside this
topic, especially the existing ones that set exit_status to non-zero
but still commits the index changes.

I am OK, as a place to stop for now, if the topic had something like

+	if (take_worktree_changes) {
+		if (report_path_error(ps_matched, &pathspec))
+			exit(128);
+	}

in it, though, because this is a new behaviour.

> Doing this, we would need to take care of atleast 4 tests breaking in
> t3700-add:
>  error out when attempting to add ignored ones but add others
>  git add --ignore-errors
>  git add (add.ignore-errors)
>  git add --chmod fails with non regular files (but updates the other paths)
>
> while ignore-errors ones would be trivial to fix, fixing other 2 would
> probably require some more than trivial code changes, as from the title,
> their behavior seems pretty much set in stone. That's why I did the
> 'goto cleanup' approach to not break these.

I am not sure if these are expecting the right outcome in the first
place, and the need to examine what the right behaviour should be is
what makes me say "I do not want to make the all-or-none thing part
of this topic".

>> The renormalize() thing is not noticing unused pathspec elements,
>> which we might want to fix, but I suspect it is far less commonly
>> used mode of operation, so it may be OK to leave it to future
>> follow-up series.

Thanks.