{"thread":{"id":"32683","subject":"[PATCH 1/2] Update :/abc ambiguity check","startedAt":"2013-01-21T13:00:48Z","lastAt":"2013-01-22T04:10:50Z","messageCount":5,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"207394","messageId":"1358773249-24384-1-git-send-email-pclouds@gmail.com","threadId":"32683","inReplyTo":null,"subject":"[PATCH 1/2] Update :/abc ambiguity check","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-21T13:00:48Z","receivedAt":"2013-01-21T13:00:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":":/abc may mean two things:\n\n- as a revision, it means the revision that has \"abc\" in commit\n message.\n\n- as a pathpec, it means \"abc\" from root.\n\nCurrently we see \":/abc\" as a rev (most of the time), but never see it\nas a pathspec even if \"abc\" exists and \"git log :/abc\" will gladly\ntake \":/abc\" as rev even it's ambiguous. This patch makes it:\n\n- ambiguous when \"abc\" exists on worktree\n- a rev if abc does not exist on worktree\n- a path if abc is not found in any commits (although better use\n \"--\" to avoid ambiguation because searching through commit DAG is\n expensive)\n\nA plus from this patch is, because \":/\" never matches anything as a\nrev, it is never considered a valid rev and because root directory\nalways exists, \":/\" is always unambiguously seen as a pathspec.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n This makes \"git grep foo :/\" work. For such an often used command,\n the less I type the better.\n\n The \".. is expensive\" thing is not cool. But it hopefully does not\n happen often. I expect people rely on shell's tab completion more\n than :/typing-path-to-somewhere-manually. Copy/paste happens more\n often. I usually type \":\" then copy a path from diff --git line.\n\n We should probably kill \":/abc as a rev\" and replace it with @{/abc}\n or something less ambiguous.\n\n setup.c | 9 ++++++++-\n t/t4208-log-magic-pathspec.sh | 17 +++++++++++++++--\n 2 files changed, 23 insertions(+), 3 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex f108c4b..47acc11 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -66,7 +66,14 @@ int check_filename(const char *prefix, const char *arg)\n \tconst char *name;\n \tstruct stat st;\n \n-\tname = prefix ? prefix_filename(prefix, strlen(prefix), arg) : arg;\n+\tif (!prefixcmp(arg, \":/\")) {\n+\t\tif (arg[2] == '\\0') /* \":/\" is root dir, always exists */\n+\t\t\treturn 1;\n+\t\tname = arg + 2;\n+\t} else if (prefix)\n+\t\tname = prefix_filename(prefix, strlen(prefix), arg);\n+\telse\n+\t\tname = arg;\n \tif (!lstat(name, &st))\n \t\treturn 1; /* file exists */\n \tif (errno == ENOENT || errno == ENOTDIR)\ndiff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\nindex 2c482b6..72300b5 100755\n--- a/t/t4208-log-magic-pathspec.sh\n+++ b/t/t4208-log-magic-pathspec.sh\n@@ -11,11 +11,24 @@ test_expect_success 'setup' '\n \tmkdir sub\n '\n \n-test_expect_success '\"git log :/\" should be ambiguous' '\n-\ttest_must_fail git log :/ 2>error &&\n+test_expect_success '\"git log :/\" should not be ambiguous' '\n+\tgit log :/\n+'\n+\n+test_expect_success '\"git log :/a\" should be ambiguous (applied both rev and worktree)' '\n+\t: >a &&\n+\ttest_must_fail git log :/a 2>error &&\n \tgrep ambiguous error\n '\n \n+test_expect_success '\"git log :/a -- \" should not be ambiguous' '\n+\tgit log :/a --\n+'\n+\n+test_expect_success '\"git log -- :/a\" should not be ambiguous' '\n+\tgit log -- :/a\n+'\n+\n test_expect_success '\"git log :\" should be ambiguous' '\n \ttest_must_fail git log : 2>error &&\n \tgrep ambiguous error\n-- \n1.8.0.rc2.23.g1fb49df\n"},{"id":"207395","messageId":"1358773249-24384-2-git-send-email-pclouds@gmail.com","threadId":"32683","inReplyTo":"1358773249-24384-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/2] grep: avoid accepting ambiguous revision","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-21T13:00:49Z","receivedAt":"2013-01-21T13:00:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/grep.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 0e1b6c8..8025964 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -823,6 +823,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tstruct object *object = parse_object(sha1);\n \t\t\tif (!object)\n \t\t\t\tdie(_(\"bad object %s\"), arg);\n+\t\t\tif (!seen_dashdash)\n+\t\t\t\tverify_non_filename(prefix, arg);\n \t\t\tadd_object_array(object, arg, &list);\n \t\t\tcontinue;\n \t\t}\n-- \n1.8.0.rc2.23.g1fb49df\n"},{"id":"207432","messageId":"7vsj5ufia6.fsf@alter.siamese.dyndns.org","threadId":"32683","inReplyTo":"1358773249-24384-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/2] Update :/abc ambiguity check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-21T19:27:45Z","receivedAt":"2013-01-21T19:27:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> :/abc may mean two things:\n>\n> - as a revision, it means the revision that has \"abc\" in commit\n> message.\n>\n> - as a pathpec, it means \"abc\" from root.\n>\n> Currently we see \":/abc\" as a rev (most of the time), but never see it\n> as a pathspec even if \"abc\" exists and \"git log :/abc\" will gladly\n> take \":/abc\" as rev even it's ambiguous. This patch makes it:\n>\n> - ambiguous when \"abc\" exists on worktree\n> - a rev if abc does not exist on worktree\n> - a path if abc is not found in any commits (although better use\n\nThe \"any commits\" above sounds very scary. Are you really going to\ncheck against all the commits?\n\n> \"--\" to avoid ambiguation because searching through commit DAG is\n> expensive)\n>\n> A plus from this patch is, because \":/\" never matches anything as a\n> rev, it is never considered a valid rev and because root directory\n> always exists, \":/\" is always unambiguously seen as a pathspec.\n\nThat is the primary plus in practice, I think, and it is a big one.\n\nWhen naming a directory that belongs to a different subdirectory\nhierarchy, typing \":/that/directory/name\" is not any easier than\nhaving your shell help you complete \"../../that/directory/name\"; I\nsuspect nobody uses the relative-to-root notation to name anything\nbut the root in real life.\n"},{"id":"207466","messageId":"CACsJy8B1uxbJvP+0ZEx3br9_Qr9ZX7num8bcgd5sFS7XnvGNpw@mail.gmail.com","threadId":"32683","inReplyTo":"7vsj5ufia6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Update :/abc ambiguity check","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-22T02:46:13Z","receivedAt":"2013-01-22T02:46:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 22, 2013 at 2:27 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> :/abc may mean two things:\n>>\n>> - as a revision, it means the revision that has \"abc\" in commit\n>> message.\n>>\n>> - as a pathpec, it means \"abc\" from root.\n>>\n>> Currently we see \":/abc\" as a rev (most of the time), but never see it\n>> as a pathspec even if \"abc\" exists and \"git log :/abc\" will gladly\n>> take \":/abc\" as rev even it's ambiguous. This patch makes it:\n>>\n>> - ambiguous when \"abc\" exists on worktree\n>> - a rev if abc does not exist on worktree\n>> - a path if abc is not found in any commits (although better use\n>\n> The \"any commits\" above sounds very scary. Are you really going to\n> check against all the commits?\n\nIf I remember correctly :/ will search through commit chains until it\nfinds a commit that matches. So :/non-existent-string definitely\nsearches through all commits.\n\n>> \"--\" to avoid ambiguation because searching through commit DAG is\n>> expensive)\n>>\n>> A plus from this patch is, because \":/\" never matches anything as a\n>> rev, it is never considered a valid rev and because root directory\n>> always exists, \":/\" is always unambiguously seen as a pathspec.\n>\n> That is the primary plus in practice, I think, and it is a big one.\n>\n> When naming a directory that belongs to a different subdirectory\n> hierarchy, typing \":/that/directory/name\" is not any easier than\n> having your shell help you complete \"../../that/directory/name\"; I\n> suspect nobody uses the relative-to-root notation to name anything\n> but the root in real life.\n\nAs I noted in the patch comment, I do copy/paste repo-absolute paths\nfrom a diff quite often (just skip the \"a\" and \"b\" prefix). Sometimes\nI hope \"git diff\" has an option to produce relative paths..\n-- \nDuy\n"},{"id":"207468","messageId":"7vlibldfhx.fsf@alter.siamese.dyndns.org","threadId":"32683","inReplyTo":"CACsJy8B1uxbJvP+0ZEx3br9_Qr9ZX7num8bcgd5sFS7XnvGNpw@mail.gmail.com","subject":"Re: [PATCH 1/2] Update :/abc ambiguity check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-22T04:10:50Z","receivedAt":"2013-01-22T04:10:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>>> ... take \":/abc\" as rev even it's ambiguous. This patch makes it:\n>>>\n>>> - ambiguous when \"abc\" exists on worktree\n>>> - a rev if abc does not exist on worktree\n>>> - a path if abc is not found in any commits (although better use\n>>\n>> The \"any commits\" above sounds very scary. Are you really going to\n>> check against all the commits?\n>\n> If I remember correctly :/ will search through commit chains until it\n> finds a commit that matches. So :/non-existent-string definitely\n> searches through all commits.\n\nThat is the real work the user asked us to do, so it is not a wasted\nlatency. The description looked as if you were doing extra work\nonly for disambiguation, which triggered my \"Huh?\" meter.\n"}]}