Re: [PATCH v2] ls-files: fix pathspec display on error
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:51:43
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH v2] ls-files: fix pathspec display on error · Clemens Buchacher <hidden>
Clemens Buchacher [off-list ref] writes:
... No changes except to address your comments. Thanks for reviewing.
Thank *you* for re-rolling.
quoted hunk
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;
quoted hunk
@@ -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 ) '