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

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

From
Clemens Buchacher <drizzd@aon.at>
Date
Jul 29, 2011, 13:03 UTC
Message-ID
<20110729130330.GA31941@toss.lan>
In-Reply-To
<b535c21e1becdf8aeeb1d3f8ddaa7f5415830844.1311767679.git.git@drmicha.warpmail.net>

The following sequence of commands reveals an issue with error reporting of relative paths:

 $ mkdir sub
 $ cd sub
 $ git ls-files --error-unmatch ../bbbbb
 error: pathspec 'b' did not match any file(s) known to git.
 $ git commit --error-unmatch ../bbbbb
 error: pathspec 'b' did not match any file(s) known to git.

This bug is visible only if the normalized path (i.e., the relative path from the repository root) is longer than the prefix. Otherwise, the code skips over the normalized path and reads from an unused memory location which still contains a leftover of the original command line argument.

So instead, use the existing facilities to deal with relative paths correctly.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
On Wed, Jul 27, 2011 at 01:56:14PM +0200, Michael J Gruber wrote:
>
> So, I couldn't reproduce because it depends on the length of the dirname.
> This may be fallout from efad1a5 (ls-files: allow relative pathspec, 2010-06-03)

Since that was me I had a look. I don't think this is a regression, however. We have had this bug probably forever. Fortunately, it's easy to fix with what we already have.

Clemens
 builtin/checkout.c |    2 +-
 builtin/commit.c   |    2 +-
 builtin/ls-files.c |   11 ++++++++---
 cache.h            |    2 +-
 quote.c            |    8 ++++++--
 5 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index d647a31..a3380d9 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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, 0))
+	if (report_path_error(ps_matched, pathspec, NULL, -1))
 		return 1;
 
 	/* "checkout -m path" to recreate conflicted state */
diff --git a/builtin/commit.c b/builtin/commit.c
index e1af9b1..a16d00b 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 ? strlen(prefix) : 0);
+	return report_path_error(m, pattern, prefix, -1);
 }
 
 static void add_remove_files(struct string_list *list)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 1570123..72b986f 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -388,11 +388,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;
@@ -417,10 +420,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);
 		errors++;
 	}
+	strbuf_release(&sb);
 	return errors;
 }
 
@@ -611,7 +616,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_len);
+		bad = report_path_error(ps_matched, pathspec, prefix, prefix_len);
 		if (bad)
 			fprintf(stderr, "Did you forget to 'git add'?\n");
 
diff --git a/cache.h b/cache.h
index 9e12d55..86518fb 100644
--- a/cache.h
+++ b/cache.h
@@ -1188,7 +1188,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, int prefix_offset);
+int report_path_error(const char *ps_matched, const char **pathspec, const char *prefix, int prefix_len);
 void overlay_tree_on_cache(const char *tree_name, const char *prefix);
 
 char *alias_lookup(const char *alias);
diff --git a/quote.c b/quote.c
index 63d3b01..532fd3b 100644
--- a/quote.c
+++ b/quote.c
@@ -325,8 +325,12 @@ static const char *path_relative(const char *in, int len,
 
 	if (len < 0)
 		len = strlen(in);
-	if (prefix && prefix_len < 0)
-		prefix_len = strlen(prefix);
+	if (prefix_len < 0) {
+		if (prefix)
+			prefix_len = strlen(prefix);
+		else
+			prefix_len = 0;
+	}
 
 	off = 0;
 	i = 0;
-- 
1.7.3.1.105.g84915
Previous: Michael J GruberNext: Junio C Hamano
Message 11 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.