# [PATCH 1/2] Update :/abc ambiguity check 5 messages from 2013-01-21 to 2013-01-22. Participants: Nguyễn Thái Ngọc Duy, Junio C Hamano, Duy Nguyen. Thread: https://gitlist.dev/t/32683 ## Nguyễn Thái Ngọc Duy, 2013-01-21 13:00 Subject: [PATCH 1/2] Update :/abc ambiguity check Message-ID: <1358773249-24384-1-git-send-email-pclouds@gmail.com> URL: https://gitlist.dev/e/1358773249-24384-1-git-send-email-pclouds%40gmail.com ``` :/abc may mean two things: - as a revision, it means the revision that has "abc" in commit message. - as a pathpec, it means "abc" from root. Currently we see ":/abc" as a rev (most of the time), but never see it as a pathspec even if "abc" exists and "git log :/abc" will gladly take ":/abc" as rev even it's ambiguous. This patch makes it: - ambiguous when "abc" exists on worktree - a rev if abc does not exist on worktree - a path if abc is not found in any commits (although better use "--" to avoid ambiguation because searching through commit DAG is expensive) A plus from this patch is, because ":/" never matches anything as a rev, it is never considered a valid rev and because root directory always exists, ":/" is always unambiguously seen as a pathspec. Signed-off-by: Nguyễn Thái Ngọc Duy --- This makes "git grep foo :/" work. For such an often used command, the less I type the better. The ".. is expensive" thing is not cool. But it hopefully does not happen often. I expect people rely on shell's tab completion more than :/typing-path-to-somewhere-manually. Copy/paste happens more often. I usually type ":" then copy a path from diff --git line. We should probably kill ":/abc as a rev" and replace it with @{/abc} or something less ambiguous. setup.c | 9 ++++++++- t/t4208-log-magic-pathspec.sh | 17 +++++++++++++++-- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/setup.c b/setup.c index f108c4b..47acc11 100644 --- a/setup.c +++ b/setup.c @@ -66,7 +66,14 @@ int check_filename(const char *prefix, const char *arg) const char *name; struct stat st; - name = prefix ? prefix_filename(prefix, strlen(prefix), arg) : arg; + if (!prefixcmp(arg, ":/")) { + if (arg[2] == '\0') /* ":/" is root dir, always exists */ + return 1; + name = arg + 2; + } else if (prefix) + name = prefix_filename(prefix, strlen(prefix), arg); + else + name = arg; if (!lstat(name, &st)) return 1; /* file exists */ if (errno == ENOENT || errno == ENOTDIR) diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh index 2c482b6..72300b5 100755 --- a/t/t4208-log-magic-pathspec.sh +++ b/t/t4208-log-magic-pathspec.sh @@ -11,11 +11,24 @@ test_expect_success 'setup' ' mkdir sub ' -test_expect_success '"git log :/" should be ambiguous' ' - test_must_fail git log :/ 2>error && +test_expect_success '"git log :/" should not be ambiguous' ' + git log :/ +' + +test_expect_success '"git log :/a" should be ambiguous (applied both rev and worktree)' ' + : >a && + test_must_fail git log :/a 2>error && grep ambiguous error ' +test_expect_success '"git log :/a -- " should not be ambiguous' ' + git log :/a -- +' + +test_expect_success '"git log -- :/a" should not be ambiguous' ' + git log -- :/a +' + test_expect_success '"git log :" should be ambiguous' ' test_must_fail git log : 2>error && grep ambiguous error -- 1.8.0.rc2.23.g1fb49df ``` ## Nguyễn Thái Ngọc Duy, 2013-01-21 13:00 Subject: [PATCH 2/2] grep: avoid accepting ambiguous revision Message-ID: <1358773249-24384-2-git-send-email-pclouds@gmail.com> URL: https://gitlist.dev/e/1358773249-24384-2-git-send-email-pclouds%40gmail.com In-Reply-To: <1358773249-24384-1-git-send-email-pclouds@gmail.com> ``` Signed-off-by: Nguyễn Thái Ngọc Duy --- builtin/grep.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/grep.c b/builtin/grep.c index 0e1b6c8..8025964 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -823,6 +823,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix) struct object *object = parse_object(sha1); if (!object) die(_("bad object %s"), arg); + if (!seen_dashdash) + verify_non_filename(prefix, arg); add_object_array(object, arg, &list); continue; } -- 1.8.0.rc2.23.g1fb49df ``` ## Junio C Hamano, 2013-01-21 19:27 Subject: Re: [PATCH 1/2] Update :/abc ambiguity check Message-ID: <7vsj5ufia6.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vsj5ufia6.fsf%40alter.siamese.dyndns.org In-Reply-To: <1358773249-24384-1-git-send-email-pclouds@gmail.com> ``` Nguyễn Thái Ngọc Duy writes: > :/abc may mean two things: > > - as a revision, it means the revision that has "abc" in commit > message. > > - as a pathpec, it means "abc" from root. > > Currently we see ":/abc" as a rev (most of the time), but never see it > as a pathspec even if "abc" exists and "git log :/abc" will gladly > take ":/abc" as rev even it's ambiguous. This patch makes it: > > - ambiguous when "abc" exists on worktree > - a rev if abc does not exist on worktree > - a path if abc is not found in any commits (although better use The "any commits" above sounds very scary. Are you really going to check against all the commits? > "--" to avoid ambiguation because searching through commit DAG is > expensive) > > A plus from this patch is, because ":/" never matches anything as a > rev, it is never considered a valid rev and because root directory > always exists, ":/" is always unambiguously seen as a pathspec. That is the primary plus in practice, I think, and it is a big one. When naming a directory that belongs to a different subdirectory hierarchy, typing ":/that/directory/name" is not any easier than having your shell help you complete "../../that/directory/name"; I suspect nobody uses the relative-to-root notation to name anything but the root in real life. ``` ## Duy Nguyen, 2013-01-22 02:46 Subject: Re: [PATCH 1/2] Update :/abc ambiguity check Message-ID: URL: https://gitlist.dev/e/CACsJy8B1uxbJvP%2B0ZEx3br9_Qr9ZX7num8bcgd5sFS7XnvGNpw%40mail.gmail.com In-Reply-To: <7vsj5ufia6.fsf@alter.siamese.dyndns.org> ``` On Tue, Jan 22, 2013 at 2:27 AM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> :/abc may mean two things: >> >> - as a revision, it means the revision that has "abc" in commit >> message. >> >> - as a pathpec, it means "abc" from root. >> >> Currently we see ":/abc" as a rev (most of the time), but never see it >> as a pathspec even if "abc" exists and "git log :/abc" will gladly >> take ":/abc" as rev even it's ambiguous. This patch makes it: >> >> - ambiguous when "abc" exists on worktree >> - a rev if abc does not exist on worktree >> - a path if abc is not found in any commits (although better use > > The "any commits" above sounds very scary. Are you really going to > check against all the commits? If I remember correctly :/ will search through commit chains until it finds a commit that matches. So :/non-existent-string definitely searches through all commits. >> "--" to avoid ambiguation because searching through commit DAG is >> expensive) >> >> A plus from this patch is, because ":/" never matches anything as a >> rev, it is never considered a valid rev and because root directory >> always exists, ":/" is always unambiguously seen as a pathspec. > > That is the primary plus in practice, I think, and it is a big one. > > When naming a directory that belongs to a different subdirectory > hierarchy, typing ":/that/directory/name" is not any easier than > having your shell help you complete "../../that/directory/name"; I > suspect nobody uses the relative-to-root notation to name anything > but the root in real life. As I noted in the patch comment, I do copy/paste repo-absolute paths from a diff quite often (just skip the "a" and "b" prefix). Sometimes I hope "git diff" has an option to produce relative paths.. -- Duy ``` ## Junio C Hamano, 2013-01-22 04:10 Subject: Re: [PATCH 1/2] Update :/abc ambiguity check Message-ID: <7vlibldfhx.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vlibldfhx.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Duy Nguyen writes: >>> ... take ":/abc" as rev even it's ambiguous. This patch makes it: >>> >>> - ambiguous when "abc" exists on worktree >>> - a rev if abc does not exist on worktree >>> - a path if abc is not found in any commits (although better use >> >> The "any commits" above sounds very scary. Are you really going to >> check against all the commits? > > If I remember correctly :/ will search through commit chains until it > finds a commit that matches. So :/non-existent-string definitely > searches through all commits. That is the real work the user asked us to do, so it is not a wasted latency. The description looked as if you were doing extra work only for disambiguation, which triggered my "Huh?" meter. ```