threads / patch / 32683

patch, 2 partsUpdate :/abc ambiguity check

Subject: [PATCH 1/2] Update :/abc ambiguity check

## tl;dr

5 messages between Jan 21, 2013 and Jan 22, 2013. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Nguyễn Thái Ngọc Duy· Jan 21, 2013, 13:00 UTC · lore
:/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 <pclouds@gmail.com>
---
 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(-)
Show changes to 2 files +23 −3

setup.c, t/t4208-log-magic-pathspec.sh

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· Jan 21, 2013, 13:00 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 2/2] grep: avoid accepting ambiguous revision

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c | 2 ++
 1 file changed, 2 insertions(+)
Show changes to builtin/grep.c +2 −0
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· Jan 21, 2013, 19:27 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 1/2] Update :/abc ambiguity check

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 14 quoted lines
> :/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?

Show 6 quoted lines
>   "--" 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· Jan 22, 2013, 02:46 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] Update :/abc ambiguity check

On Tue, Jan 22, 2013 at 2:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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.

Show 14 quoted lines
>>   "--" 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· Jan 22, 2013, 04:10 UTC · re: Duy Nguyen · lore

Re: [PATCH 1/2] Update :/abc ambiguity check

Duy Nguyen <pclouds@gmail.com> writes:
Show 12 quoted lines
>>> ... 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.

← back to recent threads