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

[Ghanshyam Thakkar <shyamthakkar001@gmail.com>]

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
>