{"thread":{"id":"42233","subject":"[RFC PATCH] pathspec: get non matching arguments without reporting.","startedAt":"2016-05-05T22:59:57Z","lastAt":"2016-05-05T23:22:33Z","messageCount":3,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"285642","messageId":"1462489197-30616-1-git-send-email-sbeller@google.com","threadId":"42233","inReplyTo":null,"subject":"[RFC PATCH] pathspec: get non matching arguments without reporting.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-05T22:59:57Z","receivedAt":"2016-05-05T22:59:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When giving more than just pathspec arguments (e.g. submodule labels),\nwe need a way to check all non-matching arguments for the pathspec parsing\nif these are the submodule labels or typos.\n\nThis patch prepares for that use case by splitting up `report_path_error`\ninto a new checking function `unmatched_pathspec_items` and a\nreporting callback.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n  So I imagine the unmatched_pathspec_items to be used later similar as\n  in report_path_error, maybe just like this:\n\nvoid submodule_list_fn(const struct pathspec *pathspec,\n\t\t\t  int pathspec_index,\n\t\t\t  void *data_cb)\n{\n\tif (submodule_label_exist(pathspec->items[pathspec_index].original))\n\t\t// ok, record in data_cb\n\telse\n\t\terror(\" '%s' is neither recognized as matching pathspec nor did it\n\t\t\tmatch any submodule label\",\n\t\t\tpathspec->items[pathspec_index].original);\n}\n\nsubmodule_list(const char *ps_matched,\n\t\t const struct pathspec *pathspec,\n\t\t const char *prefix)\n{\n\tstruct string_list detected_labels;\n\tunmatched_pathspec_items(ps_matched, pathspec, prefix,\n\t\t\t\tsubmodule_list_fn, &detected_labels);\n\t\n\tforeach (submodule s) { \n\t\tif matches_pathspec(s, pathspec) or label_match(s, detected_labels)\n\t\t\t// do a thing\n\t\telse\n\t\t\tcontinue\n\t}\n}\n\nWhat do you think of such a design? Is it worth carrying and polishing or\nwould there be another way to do it which matches the pathspec mechanism better?\n\nThanks,\nStefan\n\n dir.c | 36 ++++++++++++++++++++++++++----------\n dir.h |  8 ++++++++\n 2 files changed, 34 insertions(+), 10 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex a4a9d9f..bc8b199 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -394,14 +394,13 @@ int match_pathspec(const struct pathspec *ps,\n \treturn negative ? 0 : positive;\n }\n \n-int report_path_error(const char *ps_matched,\n-\t\t      const struct pathspec *pathspec,\n-\t\t      const char *prefix)\n+void unmatched_pathspec_items(const char *ps_matched,\n+\t\t\t     const struct pathspec *pathspec,\n+\t\t\t     const char *prefix,\n+\t\t\t     unmatched_pathspec_items_fn fn,\n+\t\t\t     void *data_cb)\n {\n-\t/*\n-\t * Make sure all pathspec matched; otherwise it is an error.\n-\t */\n-\tint num, errors = 0;\n+\tint num;\n \tfor (num = 0; num < pathspec->nr; num++) {\n \t\tint other, found_dup;\n \n@@ -428,10 +427,27 @@ int report_path_error(const char *ps_matched,\n \t\tif (found_dup)\n \t\t\tcontinue;\n \n-\t\terror(\"pathspec '%s' did not match any file(s) known to git.\",\n-\t\t      pathspec->items[num].original);\n-\t\terrors++;\n+\t\tfn(pathspec, num, data_cb);\n \t}\n+}\n+\n+void report_path_error_fn(const struct pathspec *pathspec,\n+\t\t\t  int pathspec_index,\n+\t\t\t  void *data_cb)\n+{\n+\tint *errors = data_cb;\n+\terror(\"pathspec '%s' did not match any file(s) known to git.\",\n+\t      pathspec->items[pathspec_index].original);\n+\t(*errors)++;\n+}\n+\n+int report_path_error(const char *ps_matched,\n+\t\t      const struct pathspec *pathspec,\n+\t\t      const char *prefix)\n+{\n+\tint errors = 0;\n+\tunmatched_pathspec_items(ps_matched, pathspec, prefix,\n+\t\t\t\treport_path_error_fn, &errors);\n \treturn errors;\n }\n \ndiff --git a/dir.h b/dir.h\nindex cd46f30..ea222eb 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -211,6 +211,14 @@ extern char *common_prefix(const struct pathspec *pathspec);\n extern int match_pathspec(const struct pathspec *pathspec,\n \t\t\t  const char *name, int namelen,\n \t\t\t  int prefix, char *seen, int is_dir);\n+typedef void (*unmatched_pathspec_items_fn)(const struct pathspec *pathspec,\n+\t\t\t\t\t    int pathspec_index,\n+\t\t\t\t\t    void *data_cb);\n+void unmatched_pathspec_items(const char *ps_matched,\n+\t\t\t     const struct pathspec *pathspec,\n+\t\t\t     const char *prefix,\n+\t\t\t     unmatched_pathspec_items_fn fn,\n+\t\t\t     void *data_cb);\n extern int report_path_error(const char *ps_matched, const struct pathspec *pathspec, const char *prefix);\n extern int within_depth(const char *name, int namelen, int depth, int max_depth);\n \n-- \n2.8.0.1.g3af9c03\n"},{"id":"285645","messageId":"xmqq7ff885hj.fsf@gitster.mtv.corp.google.com","threadId":"42233","inReplyTo":"1462489197-30616-1-git-send-email-sbeller@google.com","subject":"Re: [RFC PATCH] pathspec: get non matching arguments without reporting.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T23:11:52Z","receivedAt":"2016-05-05T23:11:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> When giving more than just pathspec arguments (e.g. submodule labels),\n> we need a way to check all non-matching arguments for the pathspec parsing\n> if these are the submodule labels or typos.\n>\n> This patch prepares for that use case by splitting up `report_path_error`\n> into a new checking function `unmatched_pathspec_items` and a\n> reporting callback.\n\nI seem to recall that there is a longstanding plan to move the\n\"seen[]\" array that is separate and outside from the pathspec\nstructure into the pathspec structure.  Wouldn't that be a more\nsensible approach to solve the same thing?\n"},{"id":"285647","messageId":"xmqq37pw84zq.fsf@gitster.mtv.corp.google.com","threadId":"42233","inReplyTo":"xmqq7ff885hj.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] pathspec: get non matching arguments without reporting.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T23:22:33Z","receivedAt":"2016-05-05T23:22:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> When giving more than just pathspec arguments (e.g. submodule labels),\n>> we need a way to check all non-matching arguments for the pathspec parsing\n>> if these are the submodule labels or typos.\n>>\n>> This patch prepares for that use case by splitting up `report_path_error`\n>> into a new checking function `unmatched_pathspec_items` and a\n>> reporting callback.\n>\n> I seem to recall that there is a longstanding plan to move the\n> \"seen[]\" array that is separate and outside from the pathspec\n> structure into the pathspec structure.  Wouldn't that be a more\n> sensible approach to solve the same thing?\n\nEven with such a refactoring, the need to split the \"preprocessing\"\n(mainly \"dedup\") still remains if you want to silence this codepath,\nso I think this is OK from pathspec/dir.c point of view.\n\nBut I do not agree with the first paragraph at all.  You'd be using\nseparate syntax like '*label', ':name', etc. for the purpose of\nenumerating submodules, so instead of blindly passing argv[] to\npathspec code, I expected you to do a preprocessing of argv[]\nupfront, sifting them into three bins (labels, names and paths), and\ngiving only the last ones to the pathspec machinery.\n\nAnd if you did that, report_path_error() would never have to worry\nabout \"ah, this thing does not exist in the working tree or in the\nindex but that is natural because it is a submodule label\" at all,\nno?\n"}]}