git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Mar 29, 2024, 22:16 UTC
Message-ID
<xmqqo7awvg2w.fsf@gitster.g>
In-Reply-To
<xmqqjzlkwwk9.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> 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
Previous: Junio C HamanoNext: Ghanshyam Thakkar
Message 21 of 31 in “fix certain cases of add and commit with untracked path not erroring out”
  1. 0/2 fix certain cases of add and commit with untracked path not erroring outGhanshyam Thakkar, Mar 18, 2024
  2. 1/2 builtin/commit: error out when passing untracked path with -iGhanshyam Thakkar, Mar 18, 2024
  3. Junio C HamanoMar 18, 2024
  4. 2/2 builtin/add: error out when passing untracked path with -uGhanshyam Thakkar, Mar 18, 2024
  5. Junio C HamanoMar 18, 2024
  6. 0/3 commit, add: error out when passing untracked pathGhanshyam Thakkar, Mar 29, 2024
  7. 0/3 commit, add: error out when passing untracked pathsGhanshyam Thakkar, Apr 2, 2024
  8. 0/3 commit,add: error out when passing untracked pathGhanshyam Thakkar, Apr 3, 2024
  9. 1/3 revision: optionally record matches with pathspec elementsGhanshyam Thakkar, Apr 3, 2024
  10. 2/3 builtin/commit: error out when passing untracked path with -iGhanshyam Thakkar, Apr 3, 2024
  11. 3/3 builtin/add: error out when passing untracked path with -uGhanshyam Thakkar, Apr 3, 2024
  12. 1/3 revision: optionally record matches with pathspec elementsGhanshyam Thakkar, Apr 2, 2024
  13. 2/3 builtin/commit: error out when passing untracked path with -iGhanshyam Thakkar, Apr 2, 2024
  14. Junio C HamanoApr 2, 2024
  15. Ghanshyam ThakkarApr 2, 2024
  16. 3/3 builtin/add: error out when passing untracked path with -uGhanshyam Thakkar, Apr 2, 2024
  17. Junio C HamanoApr 2, 2024
  18. Ghanshyam ThakkarApr 2, 2024
  19. 1/3 read-cache: optionally collect pathspec matching infoGhanshyam Thakkar, Mar 29, 2024
  20. Junio C HamanoMar 29, 2024
  21. Junio C HamanoMar 29, 2024
  22. Ghanshyam ThakkarMar 30, 2024
  23. Junio C HamanoMar 30, 2024
  24. 2/3 builtin/commit: error out when passing untracked path with -iGhanshyam Thakkar, Mar 29, 2024
  25. Junio C HamanoMar 29, 2024
  26. 3/3 builtin/add: error out when passing untracked path with -uGhanshyam Thakkar, Mar 29, 2024
  27. Junio C HamanoMar 29, 2024
  28. Ghanshyam ThakkarMar 30, 2024
  29. Junio C HamanoMar 30, 2024
  30. Ghanshyam ThakkarApr 1, 2024
  31. Junio C HamanoApr 1, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.