{"thread":{"id":"17171","subject":"[PATCH 3/3] implement pattern matching in ce_path_match","startedAt":"2009-01-14T14:54:33Z","lastAt":"2009-01-16T02:51:07Z","messageCount":16,"participants":["Clemens Buchacher","Johannes Schindelin","Sverre Rabbelier","Samuel Tardieu","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"100417","messageId":"1231944876-29930-1-git-send-email-drizzd@aon.at","threadId":"17171","inReplyTo":null,"subject":"[PATCH 0/3] fix \"git add\" pattern matching","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-01-14T14:54:33Z","receivedAt":"2009-01-14T14:54:33Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Hi,\n\nJohannes Schneider reported that \"git add '*'\" did not add matching files.\nAnd indeed pattern matching for tracked files is broken since 1.6.1. I've\nnever used this feature myself, but I can imagine it's useful in some\nsituations. For example, you can do \"git add '*.c'\" to add all .c files. The\nequivalent command without pattern matching would be\n\"find . -name '*.c' | xargs git add\".\n\n[PATCH 1/3] clean up pathspec matching\n[PATCH 2/3] remove pathspec_match, use match_pathspec instead\n[PATCH 3/3] implement pattern matching in ce_path_match\n\nThis patch series fixes pattern matching for \"git add\". The first two\npatches are cleanups only. PATCH 3/3 then implements pattern matching in\nce_path_match. It is very intrusive in the sense that all commands using\nce_path_match will now have pattern matching. I suppose this is good in\ngeneral, but can also have undesired side-effects, as in \"git log --follow\n'*'\", for example. I'd appreciate some double-checking in that context.\n\nCheers,\nClemens\n"},{"id":"100416","messageId":"1231944876-29930-2-git-send-email-drizzd@aon.at","threadId":"17171","inReplyTo":"1231944876-29930-1-git-send-email-drizzd@aon.at","subject":"[PATCH 1/3] clean up pathspec matching","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-01-14T14:54:34Z","receivedAt":"2009-01-14T14:54:34Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"If pathspec already matched exactly, it cannot match any more.\nOriginally, we had to continue anyways, because we did not\ndifferentiate between exact, recursive and globbing matches.\n\nSigned-off-by: Clemens Buchacher \n---\n dir.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 7c59829..87a9758 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -118,7 +118,7 @@ int match_pathspec(const char **pathspec, const char *name, int namelen, int pre\n \n \tfor (retval = 0; (match = *pathspec++) != NULL; seen++) {\n \t\tint how;\n-\t\tif (retval && *seen == MATCHED_EXACTLY)\n+\t\tif (*seen == MATCHED_EXACTLY)\n \t\t\tcontinue;\n \t\tmatch += prefix;\n \t\thow = match_one(match, name, namelen);\n-- \n1.6.1\n"},{"id":"100415","messageId":"1231944876-29930-3-git-send-email-drizzd@aon.at","threadId":"17171","inReplyTo":"1231944876-29930-2-git-send-email-drizzd@aon.at","subject":"[PATCH 2/3] remove pathspec_match, use match_pathspec instead","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-01-14T14:54:35Z","receivedAt":"2009-01-14T14:54:35Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Both versions have the same functionality. This removes any\nredundancy.\n\nThis also adds makes two extensions to match_pathspec:\n\n- If pathspec is NULL, return 1. This reflects the behavior of git\n commands, for which no paths usually means \"match all paths\".\n\n- If seen is NULL, do not use it.\n\nSigned-off-by: Clemens Buchacher \n---\n builtin-checkout.c | 6 +++---\n builtin-commit.c | 2 +-\n builtin-ls-files.c | 40 ++--------------------------------------\n cache.h | 1 -\n dir.c | 19 +++++++++++--------\n 5 files changed, 17 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin-checkout.c b/builtin-checkout.c\nindex b5dd9c0..84a2825 100644\n--- a/builtin-checkout.c\n+++ b/builtin-checkout.c\n@@ -240,7 +240,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,\n \n \tfor (pos = 0; pos < active_nr; pos++) {\n \t\tstruct cache_entry *ce = active_cache[pos];\n-\t\tpathspec_match(pathspec, ps_matched, ce->name, 0);\n+\t\tmatch_pathspec(pathspec, ce->name, ce_namelen(ce), 0, ps_matched);\n \t}\n \n \tif (report_path_error(ps_matched, pathspec, 0))\n@@ -249,7 +249,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,\n \t/* Any unmerged paths? */\n \tfor (pos = 0; pos < active_nr; pos++) {\n \t\tstruct cache_entry *ce = active_cache[pos];\n-\t\tif (pathspec_match(pathspec, NULL, ce->name, 0)) {\n+\t\tif (match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, NULL)) {\n \t\t\tif (!ce_stage(ce))\n \t\t\t\tcontinue;\n \t\t\tif (opts->force) {\n@@ -274,7 +274,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,\n \tstate.refresh_cache = 1;\n \tfor (pos = 0; pos < active_nr; pos++) {\n \t\tstruct cache_entry *ce = active_cache[pos];\n-\t\tif (pathspec_match(pathspec, NULL, ce->name, 0)) {\n+\t\tif (match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, NULL)) {\n \t\t\tif (!ce_stage(ce)) {\n \t\t\t\terrs |= checkout_entry(ce, &state, NULL);\n \t\t\t\tcontinue;\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 7cf227a..913aa89 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -166,7 +166,7 @@ static int list_paths(struct string_list *list, const char *with_tree,\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tif (ce->ce_flags & CE_UPDATE)\n \t\t\tcontinue;\n-\t\tif (!pathspec_match(pattern, m, ce->name, 0))\n+\t\tif (!match_pathspec(pattern, ce->name, ce_namelen(ce), 0, m))\n \t\t\tcontinue;\n \t\tstring_list_insert(ce->name, list);\n \t}\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex f72eb85..3434031 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -36,42 +36,6 @@ static const char *tag_other = \"\";\n static const char *tag_killed = \"\";\n static const char *tag_modified = \"\";\n \n-\n-/*\n- * Match a pathspec against a filename. The first \"skiplen\" characters\n- * are the common prefix\n- */\n-int pathspec_match(const char **spec, char *ps_matched,\n-\t\t const char *filename, int skiplen)\n-{\n-\tconst char *m;\n-\n-\twhile ((m = *spec++) != NULL) {\n-\t\tint matchlen = strlen(m + skiplen);\n-\n-\t\tif (!matchlen)\n-\t\t\tgoto matched;\n-\t\tif (!strncmp(m + skiplen, filename + skiplen, matchlen)) {\n-\t\t\tif (m[skiplen + matchlen - 1] == '/')\n-\t\t\t\tgoto matched;\n-\t\t\tswitch (filename[skiplen + matchlen]) {\n-\t\t\tcase '/': case '\\0':\n-\t\t\t\tgoto matched;\n-\t\t\t}\n-\t\t}\n-\t\tif (!fnmatch(m + skiplen, filename + skiplen, 0))\n-\t\t\tgoto matched;\n-\t\tif (ps_matched)\n-\t\t\tps_matched++;\n-\t\tcontinue;\n-\tmatched:\n-\t\tif (ps_matched)\n-\t\t\t*ps_matched = 1;\n-\t\treturn 1;\n-\t}\n-\treturn 0;\n-}\n-\n static void show_dir_entry(const char *tag, struct dir_entry *ent)\n {\n \tint len = prefix_len;\n@@ -80,7 +44,7 @@ static void show_dir_entry(const char *tag, struct dir_entry *ent)\n \tif (len >= ent->len)\n \t\tdie(\"git ls-files: internal error - directory entry not superset of prefix\");\n \n-\tif (pathspec && !pathspec_match(pathspec, ps_matched, ent->name, len))\n+\tif (!match_pathspec(pathspec, ent->name, ent->len, len, ps_matched))\n \t\treturn;\n \n \tfputs(tag, stdout);\n@@ -156,7 +120,7 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)\n \tif (len >= ce_namelen(ce))\n \t\tdie(\"git ls-files: internal error - cache entry not superset of prefix\");\n \n-\tif (pathspec && !pathspec_match(pathspec, ps_matched, ce->name, len))\n+\tif (!match_pathspec(pathspec, ce->name, ce_namelen(ce), len, ps_matched))\n \t\treturn;\n \n \tif (tag && *tag && show_valid_bit &&\ndiff --git a/cache.h b/cache.h\nindex 2dbd546..eba8afc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -940,7 +940,6 @@ extern int ws_fix_copy(char *, const char *, int, unsigned, int *);\n extern int ws_blank_line(const char *line, int len, unsigned ws_rule);\n \n /* ls-files */\n-int pathspec_match(const char **spec, char *matched, const char *filename, int skiplen);\n int report_path_error(const char *ps_matched, const char **pathspec, int prefix_offset);\n void overlay_tree_on_cache(const char *tree_name, const char *prefix);\n \ndiff --git a/dir.c b/dir.c\nindex 87a9758..d3b92de 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -108,25 +108,28 @@ static int match_one(const char *match, const char *name, int namelen)\n * and a mark is left in seen[] array for pathspec element that\n * actually matched anything.\n */\n-int match_pathspec(const char **pathspec, const char *name, int namelen, int prefix, char *seen)\n+int match_pathspec(const char **pathspec, const char *name, int namelen,\n+\t\tint prefix, char *seen)\n {\n-\tint retval;\n-\tconst char *match;\n+\tint i, retval = 0;\n+\n+\tif (!pathspec)\n+\t\treturn 1;\n \n \tname += prefix;\n \tnamelen -= prefix;\n \n-\tfor (retval = 0; (match = *pathspec++) != NULL; seen++) {\n+\tfor (i = 0; pathspec[i] != NULL; i++) {\n \t\tint how;\n-\t\tif (*seen == MATCHED_EXACTLY)\n+\t\tconst char *match = pathspec[i] + prefix;\n+\t\tif (seen && seen[i] == MATCHED_EXACTLY)\n \t\t\tcontinue;\n-\t\tmatch += prefix;\n \t\thow = match_one(match, name, namelen);\n \t\tif (how) {\n \t\t\tif (retval < how)\n \t\t\t\tretval = how;\n-\t\t\tif (*seen < how)\n-\t\t\t\t*seen = how;\n+\t\t\tif (seen && seen[i] < how)\n+\t\t\t\tseen[i] = how;\n \t\t}\n \t}\n \treturn retval;\n-- \n1.6.1\n"},{"id":"100414","messageId":"1231944876-29930-4-git-send-email-drizzd@aon.at","threadId":"17171","inReplyTo":"1231944876-29930-3-git-send-email-drizzd@aon.at","subject":"[PATCH 3/3] implement pattern matching in ce_path_match","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-01-14T14:54:36Z","receivedAt":"2009-01-14T14:54:36Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"For tracked files, matching globbing pattern in \"git add\" was broken\nby 1e5f764c (builtin-add.c: optimize -A option and \"git add .\"), which\nuses ce_path_match instead of match_pathspec for tracked files.\nBut ce_path_match does not implement pattern matching.\n\nWith this patch ce_path_match uses match_pathspec in order to perform\npattern matching.\n\nThis also implies pattern matching for many other git commands, such\nas add -u, blame, log, etc. For \"git log --follow\", we have to\ndisallow globbing pattern, because they potentially match more than\none file.\n\nReported-by: Johannes Schneider \nSigned-off-by: Clemens Buchacher \n---\n builtin-log.c | 13 ++++++++++++-\n read-cache.c | 23 +----------------------\n t/t2200-add-update.sh | 10 ++++++++++\n 3 files changed, 23 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex c7aa48e..16e9207 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -25,6 +25,17 @@ static int default_show_root = 1;\n static const char *fmt_patch_subject_prefix = \"PATCH\";\n static const char *fmt_pretty;\n \n+static int has_special(const char *p)\n+{\n+\tint x;\n+\n+\twhile ((x = *p++) != '\\0')\n+\t\tif (isspecial(x))\n+\t\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\t struct rev_info *rev)\n {\n@@ -49,7 +60,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\trev->always_show_header = 0;\n \tif (DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES)) {\n \t\trev->always_show_header = 0;\n-\t\tif (rev->diffopt.nr_paths != 1)\n+\t\tif (rev->diffopt.nr_paths != 1 || has_special(rev->diffopt.paths[0]))\n \t\t\tusage(\"git logs can only follow renames on one pathname at a time\");\n \t}\n \tfor (i = 1; i < argc; i++) {\ndiff --git a/read-cache.c b/read-cache.c\nindex b1475ff..8a5f306 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -640,28 +640,7 @@ int ce_same_name(struct cache_entry *a, struct cache_entry *b)\n \n int ce_path_match(const struct cache_entry *ce, const char **pathspec)\n {\n-\tconst char *match, *name;\n-\tint len;\n-\n-\tif (!pathspec)\n-\t\treturn 1;\n-\n-\tlen = ce_namelen(ce);\n-\tname = ce->name;\n-\twhile ((match = *pathspec++) != NULL) {\n-\t\tint matchlen = strlen(match);\n-\t\tif (matchlen > len)\n-\t\t\tcontinue;\n-\t\tif (memcmp(name, match, matchlen))\n-\t\t\tcontinue;\n-\t\tif (matchlen && name[matchlen-1] == '/')\n-\t\t\treturn 1;\n-\t\tif (name[matchlen] == '/' || !name[matchlen])\n-\t\t\treturn 1;\n-\t\tif (!matchlen)\n-\t\t\treturn 1;\n-\t}\n-\treturn 0;\n+\treturn match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, NULL);\n }\n \n /*\ndiff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\nindex cd9231c..6d99461 100755\n--- a/t/t2200-add-update.sh\n+++ b/t/t2200-add-update.sh\n@@ -128,4 +128,14 @@ test_expect_success 'add -n -u should not add but just report' '\n \n '\n \n+test_expect_success 'wildcard update' '\n+\n+\t: >a.x &&\n+\tgit add \"*.x\" &&\n+\techo asdf >a.x &&\n+\tgit add -u \"*.x\" &&\n+\ttest -z \"`git ls-files -m a.x`\"\n+\n+'\n+\n test_done\n-- \n1.6.1\n"},{"id":"100418","messageId":"20090114152515.GA19305@localhost","threadId":"17171","inReplyTo":"1231944876-29930-4-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 3/3] implement pattern matching in ce_path_match","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-01-14T15:25:15Z","receivedAt":"2009-01-14T15:25:15Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Jan 14, 2009 at 03:54:36PM +0100, Clemens Buchacher wrote:\n> This also implies pattern matching for many other git commands, such\n> as add -u, blame, log, etc.\n\nOops, I thought I had removed that. AFAICT blame is not affected by this.\n"},{"id":"100421","messageId":"alpine.DEB.1.00.0901141637170.3586@pacific.mpi-cbg.de","threadId":"17171","inReplyTo":"1231944876-29930-3-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 2/3] remove pathspec_match, use match_pathspec instead","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-14T15:40:42Z","receivedAt":"2009-01-14T15:40:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 14 Jan 2009, Clemens Buchacher wrote:\n\n> Both versions have the same functionality. This removes any\n> redundancy.\n> \n> This also adds makes two extensions to match_pathspec:\n\ns/makes//\n\n> 5 files changed, 17 insertions(+), 51 deletions(-)\n\nNice. Does it still pass the test suite? (From my reading, it should, \nbut I do not quite have the time to run it right now.)\n\nCiao,\nDscho\n"},{"id":"100423","messageId":"alpine.DEB.1.00.0901141641500.3586@pacific.mpi-cbg.de","threadId":"17171","inReplyTo":"1231944876-29930-4-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 3/3] implement pattern matching in ce_path_match","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-14T15:44:36Z","receivedAt":"2009-01-14T15:44:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 14 Jan 2009, Clemens Buchacher wrote:\n\n> @@ -25,6 +25,17 @@ static int default_show_root = 1;\n> static const char *fmt_patch_subject_prefix = \"PATCH\";\n> static const char *fmt_pretty;\n> \n> +static int has_special(const char *p)\n> +{\n> +\tint x;\n> +\n> +\twhile ((x = *p++) != '\\0')\n> +\t\tif (isspecial(x))\n> +\t\t\treturn 1;\n> +\n> +\treturn 0;\n> +}\n\nI would prefer something like this:\n\n\tstatic int has_special(const char *p)\n\t{\n\t\twhile (*p)\n\t\t\tif (isspecial(*(p++)))\n\t\t\t\treturn 1;\n\t\treturn 0;\n\t}\n\nbut that is probably a matter of taste.\n\nCiao,\nDscho\n"},{"id":"100427","messageId":"bd6139dc0901140755n7e8cbf6eu7cf91279be463501@mail.gmail.com","threadId":"17171","inReplyTo":"alpine.DEB.1.00.0901141641500.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH 3/3] implement pattern matching in ce_path_match","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-01-14T15:55:45Z","receivedAt":"2009-01-14T15:55:45Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Wed, Jan 14, 2009 at 16:44, Johannes Schindelin\n wrote:\n>> +static int has_special(const char *p)\n>> +{\n>> + int x;\n>> +\n>> + while ((x = *p++) != '\\0')\n>> + if (isspecial(x))\n>> + return 1;\n>> +\n>> + return 0;\n>> +}\n>\n> I would prefer something like this:\n>\n> static int has_special(const char *p)\n> {\n> while (*p)\n> if (isspecial(*(p++)))\n> return 1;\n> return 0;\n> }\n>\n> but that is probably a matter of taste.\n\nFWIW, I think the above is a lot less readable due to the assignment\nin the while loop's conditional. Whereas in Dscho's version it is\nintuitively obvious what the termination condition of the while loop\nis.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"100430","messageId":"20090114160939.GA20307@localhost","threadId":"17171","inReplyTo":"alpine.DEB.1.00.0901141637170.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH 2/3] remove pathspec_match, use match_pathspec instead","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-01-14T16:09:39Z","receivedAt":"2009-01-14T16:09:39Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Jan 14, 2009 at 04:40:42PM +0100, Johannes Schindelin wrote:\n> > Both versions have the same functionality. This removes any\n> > redundancy.\n> >\n> > This also adds makes two extensions to match_pathspec:\n> \n> s/makes//\n\nThanks.\n\n> Nice. Does it still pass the test suite? (From my reading, it should, \n> but I do not quite have the time to run it right now.)\n\nIt sure does. I am not confident enough to send untested patches yet. :-)\n\nOn Wed, Jan 14, 2009 at 04:44:36PM +0100, Johannes Schindelin wrote:\n> I would prefer something like this:\n> \n> static int has_special(const char *p)\n> {\n> while (*p)\n> if (isspecial(*(p++)))\n> return 1;\n> return 0;\n> }\n\nOk, no problem.\n"},{"id":"100433","messageId":"2009-01-14-17-18-40+trackit+sam@rfc1149.net","threadId":"17171","inReplyTo":"alpine.DEB.1.00.0901141641500.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH 3/3] implement pattern matching in ce_path_match","fromName":"Samuel Tardieu","fromEmail":"sam@rfc1149.net","sentAt":"2009-01-14T16:18:39Z","receivedAt":"2009-01-14T16:18:39Z","isPatch":true,"sender":{"key":"sam@rfc1149.net","avatar":"https://avatars.githubusercontent.com/u/44656?v=4"},"body":">>>>> \"Johannes\" == Johannes Schindelin writes:\n\nDscho> I would prefer something like this:\n\nMy taste would favor:\n\n static int has_special(const char *p)\n {\n for (; *p; p++)\n if (isspecial(*p))\n return 1;\n return 0;\n }\n\nas it underlines the intent (loop over \"p\" characters and stop no\nlater than the end of the string) while avoiding using side effects in\nthe body to increment the pointer. This habit comes from Ada, where\nloop indices are considered read-only in the loop body.\n\nIt also eases further extensions such as\n\n static int has_special(const char *p)\n {\n for (; *p; p++)\n if (isspecial(*p) || isveryspecial(*p))\n return 1;\n return 0;\n }\n\nwithout having to move the \"++\" somewhere else.\n\nDscho> but that is probably a matter of taste.\n\nAgreed.\n\n Sam\n-- \nSamuel Tardieu -- sam@rfc1149.net -- http://www.rfc1149.net/\n"},{"id":"100436","messageId":"20090114165321.GB15758@coredump.intra.peff.net","threadId":"17171","inReplyTo":"2009-01-14-17-18-40+trackit+sam@rfc1149.net","subject":"Re: [PATCH 3/3] implement pattern matching in ce_path_match","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-14T16:53:21Z","receivedAt":"2009-01-14T16:53:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 14, 2009 at 05:18:39PM +0100, Samuel Tardieu wrote:\n\n> My taste would favor:\n> \n> static int has_special(const char *p)\n> {\n> for (; *p; p++)\n> if (isspecial(*p))\n> return 1;\n> return 0;\n> }\n\nThat was my first thought upon reading the other two, as well. And I\nagree with all of the reasoning you gave.\n\n-Peff\n"},{"id":"100458","messageId":"7vljtd20m6.fsf@gitster.siamese.dyndns.org","threadId":"17171","inReplyTo":"1231944876-29930-4-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 3/3] implement pattern matching in ce_path_match","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-14T18:39:29Z","receivedAt":"2009-01-14T18:39:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> With this patch ce_path_match uses match_pathspec in order to perform\n> pattern matching.\n\nWe have two conflicting definitions of pattern matching in our system.\nI'd make it more explicit which kind of pattern matching you are talking\nabout here.\n\nThe family of operations based on the diff-tree machinery (e.g. path\nlimited revision walking \"git log A..B -- dir1/dir2\") define the pattern\nmatching as \"leading path match (exact match is just a special case of\nthis)\". Other operations that work on paths in the work tree and the\nindex (e.g. grep, ls-files) uses \"leading path match, but fall back to\nglobbing\".\n\nIn the longer term we really should unify them by teaching the former to\nfall back to globbing without getting undue performance hit, and this\npatch may be a step in the right direction. There are optimizations that\nassume the \"leading path\" semantics to trim the input early and avoid\nopening and descending into a tree object if pathspec patterns cannot\npossibly match (see tree-diff.c::tree_entry_interesting() for an example),\nand we need to teach them to notice a glob wildcard in an earlier part of\na pathspec and to descend into some trees that they would have skipped\nwith the old definition of pathspec.\n\n> @@ -49,7 +60,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n> \t\trev->always_show_header = 0;\n> \tif (DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES)) {\n> \t\trev->always_show_header = 0;\n> -\t\tif (rev->diffopt.nr_paths != 1)\n> +\t\tif (rev->diffopt.nr_paths != 1 || has_special(rev->diffopt.paths[0]))\n> \t\t\tusage(\"git logs can only follow renames on one pathname at a time\");\n> \t}\n\nThe reason match_pathspec() first tries exact match and then falls back to\nglobbing is so that the user can say \"I have a file whose name ends with a\nquestion mark, please match it literally.\" This patch defeats it, but it\nprobably is a minor point.\n\n1/3 and 2/3 in the series looked good.\n\nThanks.\n"},{"id":"100462","messageId":"20090114192341.GA26703@localhost","threadId":"17171","inReplyTo":"7vljtd20m6.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] implement pattern matching in ce_path_match","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-01-14T19:23:41Z","receivedAt":"2009-01-14T19:23:41Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Jan 14, 2009 at 10:39:29AM -0800, Junio C Hamano wrote:\n> Clemens Buchacher writes:\n> \n> > With this patch ce_path_match uses match_pathspec in order to perform\n> > pattern matching.\n> \n> We have two conflicting definitions of pattern matching in our system.\n> I'd make it more explicit which kind of pattern matching you are talking\n> about here.\n\nRight, will fix.\n\n> In the longer term we really should unify them by teaching the former to\n> fall back to globbing without getting undue performance hit, and this\n> patch may be a step in the right direction. There are optimizations that\n> assume the \"leading path\" semantics to trim the input early and avoid\n> opening and descending into a tree object if pathspec patterns cannot\n> possibly match (see tree-diff.c::tree_entry_interesting() for an example),\n> and we need to teach them to notice a glob wildcard in an earlier part of\n> a pathspec and to descend into some trees that they would have skipped\n> with the old definition of pathspec.\n\nI see. I can probably fix that this weekend.\n\n> > @@ -49,7 +60,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n> > \t\trev->always_show_header = 0;\n> > \tif (DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES)) {\n> > \t\trev->always_show_header = 0;\n> > -\t\tif (rev->diffopt.nr_paths != 1)\n> > +\t\tif (rev->diffopt.nr_paths != 1 || has_special(rev->diffopt.paths[0]))\n> > \t\t\tusage(\"git logs can only follow renames on one pathname at a time\");\n> > \t}\n> \n> The reason match_pathspec() first tries exact match and then falls back to\n> globbing is so that the user can say \"I have a file whose name ends with a\n> question mark, please match it literally.\" This patch defeats it, but it\n> probably is a minor point.\n\nI was wondering actually if we should disallow such paths altogether,\nsince there would be no way to match only 'a?', if something like 'ab' also\nexists. So if you added 'a?' by accident, you cannot even remove it without\nalso removing 'ab'.\n\nI think we could at least add an option to disable globbing. Then we can\nalso disable the above check conditioned on that. If we allowed globbing\npattern for following renames wouldn't that result in following the first\nfile (or last in history) to match the pattern, which is potentially\nconfusing?\n"},{"id":"100497","messageId":"7vvdshzfpk.fsf@gitster.siamese.dyndns.org","threadId":"17171","inReplyTo":"20090114192341.GA26703@localhost","subject":"Re: [PATCH 3/3] implement pattern matching in ce_path_match","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-14T22:27:03Z","receivedAt":"2009-01-14T22:27:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> I think we could at least add an option to disable globbing. Then we can\n> also disable the above check conditioned on that. If we allowed globbing\n> pattern for following renames wouldn't that result in following the first\n> file (or last in history) to match the pattern, which is potentially\n> confusing?\n\nYeah, I agree that would be a reasonable thing to do. \n\nIn places we read paths from the index or from the work tree and add them\nas pathspec elements---you would want to mark them as non-globbing, too.\nWhich probably means that \"is it Ok to glob this\" setting has to be per\npathspec array elements.\n"},{"id":"100549","messageId":"20090115082017.GA7801@localhost","threadId":"17171","inReplyTo":"7vvdshzfpk.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] implement pattern matching in ce_path_match","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-01-15T08:20:18Z","receivedAt":"2009-01-15T08:20:18Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Jan 14, 2009 at 02:27:03PM -0800, Junio C Hamano wrote:\n> In places we read paths from the index or from the work tree and add them\n> as pathspec elements---you would want to mark them as non-globbing, too.\n> Which probably means that \"is it Ok to glob this\" setting has to be per\n> pathspec array elements.\n\nRight. This certainly complicates things. Also note that this invalidates\n1/3, because even if '?' matched exactly, it can still match '*', and vice\nversa. Depending on ordering one of these two cases would pose a problem.\n"},{"id":"100694","messageId":"7vzlhsneuc.fsf@gitster.siamese.dyndns.org","threadId":"17171","inReplyTo":"7vljtd20m6.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] implement pattern matching in ce_path_match","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-16T02:51:07Z","receivedAt":"2009-01-16T02:51:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Clemens Buchacher writes:\n>\n>> With this patch ce_path_match uses match_pathspec in order to perform\n>> pattern matching.\n>\n> We have two conflicting definitions of pattern matching in our system.\n> I'd make it more explicit which kind of pattern matching you are talking\n> about here.\n>\n> The family of operations based on the diff-tree machinery (e.g. path\n> limited revision walking \"git log A..B -- dir1/dir2\") define the pattern\n> matching as \"leading path match (exact match is just a special case of\n> this)\". Other operations that work on paths in the work tree and the\n> index (e.g. grep, ls-files) uses \"leading path match, but fall back to\n> globbing\".\n>\n> In the longer term we really should unify them by teaching the former to\n> fall back to globbing without getting undue performance hit, and this\n> patch may be a step in the right direction. There are optimizations that\n> assume the \"leading path\" semantics to trim the input early and avoid\n> opening and descending into a tree object if pathspec patterns cannot\n> possibly match (see tree-diff.c::tree_entry_interesting() for an example),\n> and we need to teach them to notice a glob wildcard in an earlier part of\n> a pathspec and to descend into some trees that they would have skipped\n> with the old definition of pathspec.\n\nActually there was an earlier attempt that resulted in the pathspec\nmatching tree traverser builtin-grep uses. Even though it has to work\nwith trees (when grepping inside a tree-ish) and has optimizations not to\nopen unnecessary subtrees similar to the one the diff-tree machinery has,\nit also knows how to handle globs. If we were to pick one of existing\nimplementations for the longer term unification, I think that is probably\nthe one we should build on top of.\n"}]}