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

From: Junio C Hamano <gitster@pobox.com>
To: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 2/2] builtin/add: error out when passing untracked path with -u
Date: Mon, 18 Mar 2024 10:31:29 -0700	[thread overview]
Message-ID: <xmqqa5mvh2fi.fsf@gitster.g> (raw)
In-Reply-To: <20240318155219.494206-6-shyamthakkar001@gmail.com> (Ghanshyam Thakkar's message of "Mon, 18 Mar 2024 21:22:01 +0530")

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 |