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

Re: [PATCH v2] ls-files: fix pathspec display on error

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 1, 2011, 22:30 UTC
Message-ID
<7vvcughii5.fsf@alter.siamese.dyndns.org>
In-Reply-To
<20110801211958.GA23238@toss>
Clemens Buchacher <drizzd@aon.at> writes:
> ... No changes except to address your comments.
> Thanks for reviewing.
Thank *you* for re-rolling.
Show 20 quoted lines
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 0e98bff..fef5642 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -353,11 +353,14 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
>  	}
>  }
>  
> -int report_path_error(const char *ps_matched, const char **pathspec, int prefix_len)
> +int report_path_error(const char *ps_matched, const char **pathspec,
> +		const char *prefix, int prefix_len)
>  {
>  	/*
>  	 * Make sure all pathspec matched; otherwise it is an error.
>  	 */
> +	struct strbuf sb = STRBUF_INIT;
> +	const char *name;
>  	int num, errors = 0;
>  	for (num = 0; pathspec[num]; num++) {
>  		int other, found_dup;
Show 8 quoted lines
> @@ -382,10 +385,12 @@ int report_path_error(const char *ps_matched, const char **pathspec, int prefix_
>  		if (found_dup)
>  			continue;
>  
> +		name = quote_path_relative(pathspec[num], -1, &sb, prefix);
>  		error("pathspec '%s' did not match any file(s) known to git.",
> -		      pathspec[num] + prefix_len);
> +		      name);
Is prefix_len still being used in this function?
> +		ls ../x* >>expect &&
> +		(git ls-files -c --error-unmatch ../[xy]* || true) >actual 2>&1 &&

The "|| true" construct says "ls-files _may_ exit with non-zero, but we do not care". Shouldn't we be actively expecting it to exit with non-zero, using test-must-fail here?

I am not sure if passing NULL in checkout.c is the right thing to do. I know that is not the problem this patch introduces, but it leads to this inconsistent behaviour:

	$ cd Documentation
        $ git checkout nosuch.txt
        error: pathspec 'Documentation/nosuch.txt' did not match...
        $ git commit nosuch.txt
        error: pathspec 'nosuch.txt' did not match...

I suspect that cmd_checkout() should pass prefix down to checkout_paths() so that the latter can properly strip it from the pathspec.

Perhaps this squashed in on top of your patch...
 builtin/checkout.c           |    6 +++---
 builtin/commit.c             |    2 +-
 builtin/ls-files.c           |    5 ++---
 cache.h                      |    2 +-
 t/t3005-ls-files-relative.sh |    6 +++---
 5 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index efe5e8e..a5717f1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -201,7 +201,7 @@ static int checkout_merged(int pos, struct checkout *state)
 }
 
 static int checkout_paths(struct tree *source_tree, const char **pathspec,
-			  struct checkout_opts *opts)
+			  const char *prefix, struct checkout_opts *opts)
 {
 	int pos;
 	struct checkout state;
@@ -231,7 +231,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
 		match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, ps_matched);
 	}
 
-	if (report_path_error(ps_matched, pathspec, NULL, -1))
+	if (report_path_error(ps_matched, pathspec, prefix))
 		return 1;
 
 	/* "checkout -m path" to recreate conflicted state */
@@ -1060,7 +1060,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		if (1 < !!opts.writeout_stage + !!opts.force + !!opts.merge)
 			die(_("git checkout: --ours/--theirs, --force and --merge are incompatible when\nchecking out of the index."));
 
-		return checkout_paths(source_tree, pathspec, &opts);
+		return checkout_paths(source_tree, pathspec, prefix, &opts);
 	}
 
 	if (patch_mode)
diff --git a/builtin/commit.c b/builtin/commit.c
index a16d00b..9679a99 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -272,7 +272,7 @@ static int list_paths(struct string_list *list, const char *with_tree,
 			item->util = item; /* better a valid pointer than a fake one */
 	}
 
-	return report_path_error(m, pattern, prefix, -1);
+	return report_path_error(m, pattern, prefix);
 }
 
 static void add_remove_files(struct string_list *list)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 72b986f..468bb13 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -388,8 +388,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 	}
 }
 
-int report_path_error(const char *ps_matched, const char **pathspec,
-		const char *prefix, int prefix_len)
+int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix)
 {
 	/*
 	 * Make sure all pathspec matched; otherwise it is an error.
@@ -616,7 +615,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 
 	if (ps_matched) {
 		int bad;
-		bad = report_path_error(ps_matched, pathspec, prefix, prefix_len);
+		bad = report_path_error(ps_matched, pathspec, prefix);
 		if (bad)
 			fprintf(stderr, "Did you forget to 'git add'?\n");
 
diff --git a/cache.h b/cache.h
index 80c60b4..d55a6bb 100644
--- a/cache.h
+++ b/cache.h
@@ -1175,7 +1175,7 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule);
 #define ws_tab_width(rule)     ((rule) & WS_TAB_WIDTH_MASK)
 
 /* ls-files */
-int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix, int prefix_len);
+int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix);
 void overlay_tree_on_cache(const char *tree_name, const char *prefix);
 
 char *alias_lookup(const char *alias);
diff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh
index 3c3ff5e..a2b63e2 100755
--- a/t/t3005-ls-files-relative.sh
+++ b/t/t3005-ls-files-relative.sh
@@ -48,7 +48,7 @@ test_expect_success 'ls-files -c' '
 		done >expect &&
 		echo "Did you forget to ${sq}git add${sq}?" >>expect &&
 		ls ../x* >>expect &&
-		(git ls-files -c --error-unmatch ../[xy]* || true) >actual 2>&1 &&
+		test_must_fail git ls-files -c --error-unmatch ../[xy]* >actual 2>&1 &&
 		test_cmp expect actual
 	)
 '
@@ -58,11 +58,11 @@ test_expect_success 'ls-files -o' '
 		cd top/sub &&
 		for f in ../x*
 		do
-			echo "error: pathspec ${sq}${f}${sq} did not match any file(s) known to git."
+			echo "error: pathspec $sq$f$sq did not match any file(s) known to git."
 		done >expect &&
 		echo "Did you forget to ${sq}git add${sq}?" >>expect &&
 		ls ../y* >>expect &&
-		(git ls-files -o --error-unmatch ../[xy]* || true) >actual 2>&1 &&
+		test_must_fail git ls-files -o --error-unmatch ../[xy]* >actual 2>&1 &&
 		test_cmp expect actual
 	)
 '
Previous: Clemens BuchacherNext: Erik Faye-Lund
Message 16 of 37 in “Possible bug”
  1. Reuben ThomasJul 23, 2011
  2. commit: allow partial commits with relative pathsMichael J Gruber, Jul 25, 2011
  3. Junio C HamanoJul 25, 2011
  4. Junio C HamanoJul 25, 2011
  5. Michael J GruberJul 27, 2011
  6. Reuben ThomasJul 27, 2011
  7. Michael J GruberJul 27, 2011
  8. Reuben ThomasJul 27, 2011
  9. John SzakmeisterJul 27, 2011
  10. ls-files: fix pathspec display on errorMichael J Gruber, Jul 27, 2011
  11. Clemens BuchacherJul 29, 2011
  12. Junio C HamanoAug 1, 2011
  13. test ls-files with relative pathsClemens Buchacher, Aug 1, 2011
  14. Junio C HamanoAug 1, 2011
  15. ls-files: fix pathspec display on errorClemens Buchacher, Aug 1, 2011
  16. Junio C HamanoAug 1, 2011
  17. Erik Faye-LundJul 28, 2011
  18. Junio C HamanoJul 27, 2011
  19. Junio C HamanoJul 27, 2011
  20. Michael J GruberJul 27, 2011
  21. commit: allow partial commits with relative pathsClemens Buchacher, Jul 29, 2011
  22. Michael J GruberJul 30, 2011
  23. Clemens BuchacherJul 30, 2011
  24. Michael J GruberJul 30, 2011
  25. commit: allow partial commits with relative pathsClemens Buchacher, Jul 30, 2011
  26. Junio C HamanoAug 1, 2011
  27. Junio C HamanoAug 2, 2011
  28. Clemens BuchacherAug 3, 2011
  29. Junio C HamanoAug 3, 2011
  30. renaming pathspec_prefix (was: Re: [PATCH v2] commit: allow partial commits with relative paths)Clemens Buchacher, Sep 4, 2011
  31. 1/3 remove prefix argument from pathspec_prefixClemens Buchacher, Sep 4, 2011
  32. Junio C HamanoSep 6, 2011
  33. Junio C HamanoSep 6, 2011
  34. Clemens BuchacherSep 8, 2011
  35. Junio C HamanoSep 8, 2011
  36. 2/3 consolidate pathspec_prefix and common_prefixClemens Buchacher, Sep 4, 2011
  37. 3/3 rename pathspec_prefix -> common_prefix and move to dir.[ch]Clemens Buchacher, Sep 4, 2011

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.