From 6d9429271013898df103f7e77ed0736cdfab01b8 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 10 May 2011 10:23:41 -0700 Subject: [PATCH 1/7] Revert "magic pathspec: add ":(icase)path" to match case insensitively" This reverts commit d0546e2d488b1ba185c430b638619ab1d91af509, which was only meant to be a Proof-of-concept used during the discussion. The real implementation of the feature needs to wait until we migrate all the code to use "struct pathspec", not "char **", to represent richer semantics given to pathspec. --- Documentation/glossary-content.txt | 7 ++----- setup.c | 31 ++++-------------------------- 2 files changed, 6 insertions(+), 32 deletions(-) diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt index 0ca029b738..e51d7e60eb 100644 --- a/Documentation/glossary-content.txt +++ b/Documentation/glossary-content.txt @@ -319,13 +319,10 @@ top `/`;; The magic word `top` (mnemonic: `/`) makes the pattern match from the root of the working tree, even when you are running the command from inside a subdirectory. -icase;; - The magic word `icase` (there is no mnemonic for it) makes the - pattern match case insensitively. E.g. `:(icase)makefile` matches - both `Makefile` and `makefile`. -- + -It is envisioned that we will support more types of magic in later +Currently only the slash `/` is recognized as the "magic signature", +but it is envisioned that we will support more types of magic in later versions of git. [[def_parent]]parent:: diff --git a/setup.c b/setup.c index 51e354ca07..5048252d78 100644 --- a/setup.c +++ b/setup.c @@ -136,12 +136,12 @@ void verify_non_filename(const char *prefix, const char *arg) * Possible future magic semantics include stuff like: * * { PATHSPEC_NOGLOB, '!', "noglob" }, + * { PATHSPEC_ICASE, '\0', "icase" }, * { PATHSPEC_RECURSIVE, '*', "recursive" }, * { PATHSPEC_REGEXP, '\0', "regexp" }, * */ #define PATHSPEC_FROMTOP (1<<0) -#define PATHSPEC_ICASE (1<<1) struct pathspec_magic { unsigned bit; @@ -149,7 +149,6 @@ struct pathspec_magic { const char *name; } pathspec_magic[] = { { PATHSPEC_FROMTOP, '/', "top" }, - { PATHSPEC_ICASE, '\0', "icase" }, }; /* @@ -169,8 +168,7 @@ const char *prefix_pathspec(const char *prefix, int prefixlen, const char *elt) { unsigned magic = 0; const char *copyfrom = elt; - const char *retval; - int i, free_source = 0; + int i; if (elt[0] != ':') { ; /* nothing to do */ @@ -224,31 +222,10 @@ const char *prefix_pathspec(const char *prefix, int prefixlen, const char *elt) copyfrom++; } - if (magic & PATHSPEC_ICASE) { - struct strbuf sb = STRBUF_INIT; - for (i = 0; copyfrom[i]; i++) { - int ch = copyfrom[i]; - if (('a' <= ch && ch <= 'z') || - ('A' <= ch && ch <= 'Z')) { - strbuf_addf(&sb, "[%c%c]", - tolower(ch), toupper(ch)); - } else { - strbuf_addch(&sb, ch); - } - } - if (sb.len) { - free_source = 1; - copyfrom = strbuf_detach(&sb, NULL); - } - } - if (magic & PATHSPEC_FROMTOP) - retval = xstrdup(copyfrom); + return xstrdup(copyfrom); else - retval = prefix_path(prefix, prefixlen, copyfrom); - if (free_source) - free((char *)copyfrom); - return retval; + return prefix_path(prefix, prefixlen, copyfrom); } const char **get_pathspec(const char *prefix, const char **pathspec) From b060ce7de42b357af013909039da3f08a68f3c0b Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 10 May 2011 12:07:12 -0700 Subject: [PATCH 2/7] pathspec: drop "lone : means no pathspec" from get_pathspec() We may want to give the pathspec subsystem such a feature, but not while we are still using get_pathspec() that returns a stupid "char **" that loses subtle nuances that existed in the input string. In the meantime, the callers of get_pathspec() that want to support it could do an equivalent before feeding their argv[] to the function themselves quite easily. Signed-off-by: Junio C Hamano --- setup.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/setup.c b/setup.c index 5048252d78..84f71d5ee0 100644 --- a/setup.c +++ b/setup.c @@ -197,9 +197,6 @@ const char *prefix_pathspec(const char *prefix, int prefixlen, const char *elt) } if (*copyfrom == ')') copyfrom++; - } else if (!elt[1]) { - /* Just ':' -- no element! */ - return NULL; } else { /* shorthand */ for (copyfrom = elt + 1; From 7c5f3cc4a5680e23b8aa378ed9b655a1779ee881 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 9 May 2011 21:34:04 -0700 Subject: [PATCH 3/7] grep: use get_pathspec() correctly When there is no remaining string in argv, get_pathspec(prefix, argv) will return a two-element array that has prefix as the first element, so there is no need to re-roll that logic in the code that uses get_pathspec(). Signed-off-by: Junio C Hamano --- builtin/grep.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 0bf8c0116a..222dd6d9af 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -956,13 +956,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) verify_filename(prefix, argv[j]); } - if (i < argc) - paths = get_pathspec(prefix, argv + i); - else if (prefix) { - paths = xcalloc(2, sizeof(const char *)); - paths[0] = prefix; - paths[1] = NULL; - } + paths = get_pathspec(prefix, argv + i); init_pathspec(&pathspec, paths); pathspec.max_depth = opt.max_depth; pathspec.recursive = 1; From 9619617d33c83ad873539384b3cbfd564053be76 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 10 May 2011 11:10:30 -0700 Subject: [PATCH 4/7] fix overstrict : diagnosis Given "git log :", we get a disambiguation message that tries to be helpful and yet totally misses the point, i.e. $ git log : fatal: Path '' does not exist (neither on disk nor in the index). $ git log :/ fatal: Path '/' exists on disk, but not in the index. An empty path nor anything that begins with '/' cannot possibly in the index, and it is wrong to guess that the user might have meant to access such an index entry. It should yield the same error message as "git log '*.c'", i.e. $ git log '*.c' fatal: ambiguous argument '*.c': unknown revision or path not in the working tree. Use '--' to separate paths from revisions Signed-off-by: Junio C Hamano --- sha1_name.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sha1_name.c b/sha1_name.c index faea58dc8c..90d8bfa20a 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -1173,7 +1173,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1, } pos++; } - if (!gently) + if (!gently && name[1] && name[1] != '/') diagnose_invalid_index_path(stage, prefix, cp); free(new_path); return -1; From 2e83b66c32c1d482575fd8caed80680a2f69c5f1 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 10 May 2011 12:02:54 -0700 Subject: [PATCH 5/7] fix overslow :/no-such-string-ever-existed diagnostics "git cmd :/no-such-string-ever-existed" runs an extra round of get_sha1() since 009fee4 (Detailed diagnosis when parsing an object name fails., 2009-12-07). Once without error diagnosis to see there is no commit with such a string in the log message (hence "it cannot be a ref"), and after seeing that :/no-such-string-ever-existed is not a filename (hence "it cannot be a path, either"), another time to give "better diagnosis". The thing is, the second time it runs, we already know that traversing the history all the way down to the root will _not_ find any matching commit. Rename misguided "gently" parameter, which is turned off _only_ when the "detailed diagnosis" codepath knows that it cannot be a ref and making the call only for the caller to die with a message. Flip its meaning (and adjust the callers) and call it "only_to_die", which is not a great name, but it describes far more clearly what the codepaths that switches their behaviour based on this variable do. On my box, the command spends ~1.8 seconds without the patch to make the report; with the patch it spends ~1.12 seconds. Signed-off-by: Junio C Hamano --- cache.h | 8 ++++---- setup.c | 2 +- sha1_name.c | 17 +++++++++-------- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/cache.h b/cache.h index be6ce7237d..a9e641967a 100644 --- a/cache.h +++ b/cache.h @@ -785,15 +785,15 @@ struct object_context { }; extern int get_sha1(const char *str, unsigned char *sha1); -extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int gently, const char *prefix); +extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int only_to_die, const char *prefix); static inline int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode) { - return get_sha1_with_mode_1(str, sha1, mode, 1, NULL); + return get_sha1_with_mode_1(str, sha1, mode, 0, NULL); } -extern int get_sha1_with_context_1(const char *name, unsigned char *sha1, struct object_context *orc, int gently, const char *prefix); +extern int get_sha1_with_context_1(const char *name, unsigned char *sha1, struct object_context *orc, int only_to_die, const char *prefix); static inline int get_sha1_with_context(const char *str, unsigned char *sha1, struct object_context *orc) { - return get_sha1_with_context_1(str, sha1, orc, 1, NULL); + return get_sha1_with_context_1(str, sha1, orc, 0, NULL); } extern int get_sha1_hex(const char *hex, unsigned char *sha1); extern char *sha1_to_hex(const unsigned char *sha1); /* static buffer result! */ diff --git a/setup.c b/setup.c index 84f71d5ee0..7fde4fac90 100644 --- a/setup.c +++ b/setup.c @@ -86,7 +86,7 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg) unsigned char sha1[20]; unsigned mode; /* try a detailed diagnostic ... */ - get_sha1_with_mode_1(arg, sha1, &mode, 0, prefix); + get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix); /* ... or fall back the most general message. */ die("ambiguous argument '%s': unknown revision or path not in the working tree.\n" "Use '--' to separate paths from revisions", arg); diff --git a/sha1_name.c b/sha1_name.c index 90d8bfa20a..ec836117d9 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -1080,11 +1080,12 @@ static void diagnose_invalid_index_path(int stage, } -int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, int gently, const char *prefix) +int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, + int only_to_die, const char *prefix) { struct object_context oc; int ret; - ret = get_sha1_with_context_1(name, sha1, &oc, gently, prefix); + ret = get_sha1_with_context_1(name, sha1, &oc, only_to_die, prefix); *mode = oc.mode; return ret; } @@ -1108,7 +1109,7 @@ static char *resolve_relative_path(const char *rel) int get_sha1_with_context_1(const char *name, unsigned char *sha1, struct object_context *oc, - int gently, const char *prefix) + int only_to_die, const char *prefix) { int ret, bracket_depth; int namelen = strlen(name); @@ -1130,7 +1131,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1, struct cache_entry *ce; char *new_path = NULL; int pos; - if (namelen > 2 && name[1] == '/') { + if (!only_to_die && namelen > 2 && name[1] == '/') { struct commit_list *list = NULL; for_each_ref(handle_one_ref, &list); return get_sha1_oneline(name + 2, sha1, list); @@ -1173,7 +1174,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1, } pos++; } - if (!gently && name[1] && name[1] != '/') + if (only_to_die && name[1] && name[1] != '/') diagnose_invalid_index_path(stage, prefix, cp); free(new_path); return -1; @@ -1189,7 +1190,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1, if (*cp == ':') { unsigned char tree_sha1[20]; char *object_name = NULL; - if (!gently) { + if (only_to_die) { object_name = xmalloc(cp-name+1); strncpy(object_name, name, cp-name); object_name[cp-name] = '\0'; @@ -1202,7 +1203,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1, if (new_filename) filename = new_filename; ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode); - if (!gently) { + if (only_to_die) { diagnose_invalid_sha1_path(prefix, filename, tree_sha1, object_name); free(object_name); @@ -1215,7 +1216,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1, free(new_filename); return ret; } else { - if (!gently) + if (only_to_die) die("Invalid object name '%s'.", object_name); } } From 0e539dca51c298ca2ee102e0ca118797f2da99eb Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 10 May 2011 12:05:01 -0700 Subject: [PATCH 6/7] rev/path disambiguation: further restrict "misspelled index entry" diag A colon followed by anything !isalnum() (e.g. ":/heh") at this point is known not to be an existing rev. Just give a generic "neither a rev nor a path" error message. Signed-off-by: Junio C Hamano --- setup.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/setup.c b/setup.c index 7fde4fac90..fd4ce59f28 100644 --- a/setup.c +++ b/setup.c @@ -85,8 +85,17 @@ static void NORETURN die_verify_filename(const char *prefix, const char *arg) { unsigned char sha1[20]; unsigned mode; - /* try a detailed diagnostic ... */ - get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix); + + /* + * Saying "'(icase)foo' does not exist in the index" when the + * user gave us ":(icase)foo" is just stupid. A magic pathspec + * begins with a colon and is followed by a non-alnum; do not + * let get_sha1_with_mode_1(only_to_die=1) to even trigger. + */ + if (!(arg[0] == ':' && !isalnum(arg[1]))) + /* try a detailed diagnostic ... */ + get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix); + /* ... or fall back the most general message. */ die("ambiguous argument '%s': unknown revision or path not in the working tree.\n" "Use '--' to separate paths from revisions", arg); From 6fd09f537c7a1b0a92587aa6f00ef7302a546a1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Sun, 8 May 2011 18:08:26 +0700 Subject: [PATCH 7/7] t3703, t4208: add test cases for magic pathspec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- Documentation/glossary-content.txt | 3 ++ t/t3703-add-magic-pathspec.sh | 54 ++++++++++++++++++++++++++++++ t/t4208-log-magic-pathspec.sh | 36 ++++++++++++++++++++ 3 files changed, 93 insertions(+) create mode 100755 t/t3703-add-magic-pathspec.sh create mode 100755 t/t4208-log-magic-pathspec.sh diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt index e51d7e60eb..8f62d1abee 100644 --- a/Documentation/glossary-content.txt +++ b/Documentation/glossary-content.txt @@ -324,6 +324,9 @@ top `/`;; Currently only the slash `/` is recognized as the "magic signature", but it is envisioned that we will support more types of magic in later versions of git. ++ +A pathspec with only a colon means "there is no pathspec". This form +should not be combined with other pathspec. [[def_parent]]parent:: A <> contains a (possibly empty) list diff --git a/t/t3703-add-magic-pathspec.sh b/t/t3703-add-magic-pathspec.sh new file mode 100755 index 0000000000..ce5585ebb1 --- /dev/null +++ b/t/t3703-add-magic-pathspec.sh @@ -0,0 +1,54 @@ +#!/bin/sh + +test_description='magic pathspec tests using git-add' + +. ./test-lib.sh + +test_expect_success 'setup' ' + mkdir sub anothersub && + : >sub/foo && + : >anothersub/foo +' + +test_expect_success 'add :/' " + cat >expected <<-EOF && + add 'anothersub/foo' + add 'expected' + add 'sub/actual' + add 'sub/foo' + EOF + (cd sub && git add -n :/ >actual) && + test_cmp expected sub/actual +" + +cat >expected <actual) && + test_cmp expected sub/actual +' + +test_expect_success 'add :/non-existent' ' + (cd sub && test_must_fail git add -n :/non-existent) +' + +cat >expected <":(icase)ha" && + test_must_fail git add -n ":(icase)ha" && + git add -n "./:(icase)ha" +' + +test_expect_success 'a file with the same (short) magic name exists' ' + mkdir ":" && + : >":/bar" && + test_must_fail git add -n :/bar && + git add -n "./:/bar" +' + +test_done diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh new file mode 100755 index 0000000000..2c482b622b --- /dev/null +++ b/t/t4208-log-magic-pathspec.sh @@ -0,0 +1,36 @@ +#!/bin/sh + +test_description='magic pathspec tests using git-log' + +. ./test-lib.sh + +test_expect_success 'setup' ' + test_commit initial && + test_tick && + git commit --allow-empty -m empty && + mkdir sub +' + +test_expect_success '"git log :/" should be ambiguous' ' + test_must_fail git log :/ 2>error && + grep ambiguous error +' + +test_expect_success '"git log :" should be ambiguous' ' + test_must_fail git log : 2>error && + grep ambiguous error +' + +test_expect_success 'git log -- :' ' + git log -- : +' + +test_expect_success 'git log HEAD -- :/' ' + cat >expected <<-EOF && + 24b24cf initial + EOF + (cd sub && git log --oneline HEAD -- :/ >../actual) && + test_cmp expected actual +' + +test_done