[PATCH] t3703: add test cases for pathspec magic

[PATCH] t3703: add test cases for pathspec magic

[Nguyễn Thái Ngọc Duy]

That was the intention, but it raises a question: what do we do if a
file happens to have the same name with the given magic pathspec, as
in the last two tests? People can fall into these cases with
"git add *" or something like that.

I'm tempted to reuse "--" the second time to say "no more magic from
here". But there may be better way to deal with it.

Also, long magic must be quoted or at least bash will complain. Is
there any other syntax option that can be used directly without quoting?
---
 t/t3703-add-magic-pathspec.sh |   68 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 68 insertions(+), 0 deletions(-)
 create mode 100755 t/t3703-add-magic-pathspec.sh

diff --git a/t/t3703-add-magic-pathspec.sh b/t/t3703-add-magic-pathspec.sh
new file mode 100755
index 0000000..76108ae
--- /dev/null
+++ b/t/t3703-add-magic-pathspec.sh
@@ -0,0 +1,68 @@
+#!/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 'colon alone magic can only used alone' '
+	test_must_fail git add -n sub/foo : &&
+	test_must_fail git add -n : sub/foo
+'
+
+cat >expected <<EOF
+add 'anothersub/foo'
+add 'expected'
+add 'sub/actual'
+add 'sub/foo'
+EOF
+
+test_expect_success 'add :' '
+	(cd sub && git add -n : >actual) &&
+	test_cmp expected sub/actual
+'
+
+test_expect_success 'add :/' '
+	(cd sub && git add -n :/ >actual) &&
+	test_cmp expected sub/actual
+'
+
+cat >expected <<EOF
+add 'anothersub/foo'
+EOF
+
+test_expect_success 'add :/anothersub' '
+	(cd sub && git add -n :/anothersub >actual) &&
+	test_cmp expected sub/actual
+'
+
+test_expect_success 'add :/non-existent' '
+	(cd sub && test_must_fail git add -n :/non-existent)
+'
+
+cat >expected <<EOF
+add 'sub/foo'
+EOF
+
+test_expect_success 'add :(icase)foo' '
+	(cd sub && git add -n ":(icase)FoO" >actual) &&
+	test_cmp expected sub/actual
+'
+
+test_expect_success 'a file with the same (long) magic name exists' '
+	: >":(case)ha" &&
+	git add -n ":(icase)ha"
+'
+
+test_expect_success 'a file with the same (short) magic name exists' '
+	mkdir ":" &&
+	: >":/bar" &&
+	git add -n :/bar
+'
+
+test_done
-- 
1.7.4.74.g639db

Re: [PATCH] t3703: add test cases for pathspec magic

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> That was the intention, but it raises a question: what do we do if a
> file happens to have the same name with the given magic pathspec, as
> in the last two tests?

I would have expected that in such a case the user would pass a "\:" or
even ":::" to match the file ":", and "\:(rubbish)" to match the file
":(rubbish)".  The whole ":" is special thing comes from the observation
that a path that begins with a colon is rare, so it is Ok to require the
user to do some more work (typing an extra backslash) when he really wants
to match with such a thing.

A script that takes a pathname that is meant to be a literal from the user
in its variable $x would pass ":(noglob)$x" when it wants to be strict.  A
script that lets the user say whatever and wants to pass would just pass
"$x" along the callchain.

I do not expect this to be an issue in practice, though.  Have you seen a
script that tries to quote all the possible globbing characters in "$x"
before calling into git with the current codebase without this magic?

Re: [PATCH] t3703: add test cases for pathspec magic

[Nguyen Thai Ngoc Duy]

2011/5/8 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> That was the intention, but it raises a question: what do we do if a
>> file happens to have the same name with the given magic pathspec, as
>> in the last two tests?
>
> I would have expected that in such a case the user would pass a "\:" or
> even ":::" to match the file ":", and "\:(rubbish)" to match the file
> ":(rubbish)".  The whole ":" is special thing comes from the observation
> that a path that begins with a colon is rare, so it is Ok to require the
> user to do some more work (typing an extra backslash) when he really wants
> to match with such a thing.
>
> A script that takes a pathname that is meant to be a literal from the user
> in its variable $x would pass ":(noglob)$x" when it wants to be strict.  A
> script that lets the user say whatever and wants to pass would just pass
> "$x" along the callchain.
>
> I do not expect this to be an issue in practice, though.  Have you seen a
> script that tries to quote all the possible globbing characters in "$x"
> before calling into git with the current codebase without this magic?

Point taken.
-- 
Duy

[PATCH] t3703, t4208: add test cases for magic pathspec

[Nguyễn Thái Ngọc Duy]

While at it, also document ":" syntax.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 v2.

 Documentation/glossary-content.txt |    3 +
 t/t3703-add-magic-pathspec.sh      |   78 ++++++++++++++++++++++++++++++++++++
 t/t4208-log-magic-pathspec.sh      |   40 ++++++++++++++++++
 3 files changed, 121 insertions(+), 0 deletions(-)
 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 0ca029b..02cea08 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -311,6 +311,9 @@ parenthesis `(`, a comma-separated list of zero or more "magic words",
 and a close parentheses `)`, and the remainder is the pattern to match
 against the path.
 +
+A pathspec with only a colon means "there is no pathspec". This form
+cannot be combined with other pathspec.
++
 The "magic signature" consists of an ASCII symbol that is not
 alphanumeric.
 +
diff --git a/t/t3703-add-magic-pathspec.sh b/t/t3703-add-magic-pathspec.sh
new file mode 100755
index 0000000..3d8c6b8
--- /dev/null
+++ b/t/t3703-add-magic-pathspec.sh
@@ -0,0 +1,78 @@
+#!/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_failure 'colon alone magic can only used alone' '
+	test_must_fail git add -n sub/foo : &&
+	test_must_fail git add -n : sub/foo
+'
+
+cat >expected <<EOF
+add 'anothersub/foo'
+add 'expected'
+add 'sub/actual'
+add 'sub/foo'
+EOF
+
+test_expect_success 'add :' '
+	(cd sub && git add -n : >actual) &&
+	test_cmp expected sub/actual
+'
+
+test_expect_success 'add :/' '
+	(cd sub && git add -n :/ >actual) &&
+	test_cmp expected sub/actual
+'
+
+cat >expected <<EOF
+add 'anothersub/foo'
+EOF
+
+test_expect_success 'add :/anothersub' '
+	(cd sub && git add -n :/anothersub >actual) &&
+	test_cmp expected sub/actual
+'
+
+test_expect_success 'add :/non-existent' '
+	(cd sub && test_must_fail git add -n :/non-existent)
+'
+
+cat >expected <<EOF
+add 'sub/foo'
+EOF
+
+test_expect_success 'add :(icase)foo' '
+	(cd sub && git add -n ":(icase)FoO" >actual) &&
+	test_cmp expected sub/actual
+'
+
+test_expect_success 'a file with the same (long) magic name exists' '
+	: >":(icase)ha" &&
+	test_must_fail git add -n ":(icase)ha" 2>error &&
+	git add -n "./:(icase)ha"
+'
+
+cat >expected <<EOF
+fatal: pathspec ':(icase)ha' did not match any files
+EOF
+
+test_expect_failure 'show pathspecs exactly what are typed in' '
+	test_cmp expected error
+'
+
+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 0000000..b296a74
--- /dev/null
+++ b/t/t4208-log-magic-pathspec.sh
@@ -0,0 +1,40 @@
+#!/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_failure 'git log :/ ambiguous with [ref]:/path' '
+	test_must_fail git log :/ 2>error &&
+	grep ambiguous error
+'
+
+test_expect_failure '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_expect_success 'git log HEAD -- :' '
+	cat >expected <<EOF &&
+41d179c empty
+24b24cf initial
+EOF
+	(cd sub && git log --oneline HEAD -- : >../actual) &&
+	test_cmp expected actual
+'
+
+test_done
-- 
1.7.4.74.g639db

Re: [PATCH] t3703, t4208: add test cases for magic pathspec

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> diff --git a/t/t3703-add-magic-pathspec.sh b/t/t3703-add-magic-pathspec.sh
> new file mode 100755
> index 0000000..3d8c6b8
> --- /dev/null
> +++ b/t/t3703-add-magic-pathspec.sh
> @@ -0,0 +1,78 @@
> +#!/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_failure 'colon alone magic can only used alone' '
> +	test_must_fail git add -n sub/foo : &&
> +	test_must_fail git add -n : sub/foo
> +'

I don't care too much about this case (it is a user error), but if you
promise you will turn this expect-failure to expect-success in a follow-up
patch, why not ;-)?

> +cat >expected <<EOF
> +add 'anothersub/foo'
> +add 'expected'
> +add 'sub/actual'
> +add 'sub/foo'
> +EOF
> +
> +test_expect_success 'add :' '
> +	(cd sub && git add -n : >actual) &&
> +	test_cmp expected sub/actual
> +'

Shouldn't

	$ git anycmd :

be equivalent to

	$ (cd $(git rev-parse --show-cdup)/. && git anycmd)

for any command?  I doubt this test is expecting the right outcome.
Shouldn't it result in "Nothing specified, nothing added."?

> +test_expect_success 'add :/' '
> +	(cd sub && git add -n :/ >actual) &&
> +	test_cmp expected sub/actual
> +'

This one is expecting the right thing.

> +test_expect_success 'add :/non-existent' '
> +	(cd sub && test_must_fail git add -n :/non-existent)
> +'

Just being curious. What should the error message say?  Can we make it to
say "fatal: pathspec 'non-ex' from root did not match any files"?

> +cat >expected <<EOF
> +fatal: pathspec ':(icase)ha' did not match any files
> +EOF

When writing a literal, make it a habit to quote EOF to reduce mental load
on the reader, like this:

	cat >expected <<\EOF
        ...
        EOF

so that the reader does not have to scan for $var in the text to make
sure.

> +test_expect_failure 'show pathspecs exactly what are typed in' '
> +	test_cmp expected error
> +'

Will this break under gettext-poison?

> diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh
> new file mode 100755
> index 0000000..b296a74
> --- /dev/null
> +++ b/t/t4208-log-magic-pathspec.sh
> @@ -0,0 +1,40 @@
> +#!/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_failure 'git log :/ ambiguous with [ref]:/path' '
> +	test_must_fail git log :/ 2>error &&
> +	grep ambiguous error
> +'
> +test_expect_failure 'git log :' '
> +	git log :
> +'

These two should expect exactly the same error, I think. ':', ':/' or
anything magic will not satisify verify_filename(), and needs a
double-dash before it.

We could improve the disambiguation heuristics so that when we do not have
a '--' on the command line:

 - make sure all the earlier ones are refs and they cannot be a path on
   the filesystem (otherwise we need a disambiguator "--").

 - the first non-ref argument and everything that follows must be either a
   ':' magic, a string with globbing character, or a path on the
   filesystem, and none of them can be a ref.

Do you want to take a stab at it?


    

Re: [PATCH] t3703, t4208: add test cases for magic pathspec

[Nguyen Thai Ngoc Duy]

2011/5/9 Junio C Hamano <gitster@pobox.com>:
>> +test_expect_failure 'colon alone magic can only used alone' '
>> +     test_must_fail git add -n sub/foo : &&
>> +     test_must_fail git add -n : sub/foo
>> +'
>
> I don't care too much about this case (it is a user error), but if you
> promise you will turn this expect-failure to expect-success in a follow-up
> patch, why not ;-)?

failed tests are (annoying enough) to be fixed, right? :)

>> +cat >expected <<EOF
>> +add 'anothersub/foo'
>> +add 'expected'
>> +add 'sub/actual'
>> +add 'sub/foo'
>> +EOF
>> +
>> +test_expect_success 'add :' '
>> +     (cd sub && git add -n : >actual) &&
>> +     test_cmp expected sub/actual
>> +'
>
> Shouldn't
>
>        $ git anycmd :
>
> be equivalent to
>
>        $ (cd $(git rev-parse --show-cdup)/. && git anycmd)
>
> for any command?  I doubt this test is expecting the right outcome.
> Shouldn't it result in "Nothing specified, nothing added."?

It's gray area. Yes I'd rather see that behavior, but then a lot of
code needs to be audited. Perhaps we should delay introducing ":"
until get_pathspec() learns to modify argc. IOW die() for now when
users write ":".

>> +test_expect_success 'add :/non-existent' '
>> +     (cd sub && test_must_fail git add -n :/non-existent)
>> +'
>
> Just being curious. What should the error message say?  Can we make it to
> say "fatal: pathspec 'non-ex' from root did not match any files"?

My opinion is in the next test. It should show exactly what users type
in, ie. "pathspec ':/non-existent' did not match any files". "'non-ex'
from root" is nice when there's only one magic. If users specify a few
more in one pathspec, the text may become too verbose.

>> +test_expect_failure 'show pathspecs exactly what are typed in' '
>> +     test_cmp expected error
>> +'
>
> Will this break under gettext-poison?

Never really known what that poison is. Will grep ":/non-existent" instead.

>> +test_expect_failure 'git log :/ ambiguous with [ref]:/path' '
>> +     test_must_fail git log :/ 2>error &&
>> +     grep ambiguous error
>> +'
>> +test_expect_failure 'git log :' '
>> +     git log :
>> +'
>
> These two should expect exactly the same error, I think. ':', ':/' or
> anything magic will not satisify verify_filename(), and needs a
> double-dash before it.

Yes. Error from the latter "fatal: Path '' does not exist (neither on
disk nor in the index)." makes me wonder, why does log accept a sha1
syntax that may resolve to non-commit object?

> We could improve the disambiguation heuristics so that when we do not have
> a '--' on the command line:
>
>  - make sure all the earlier ones are refs and they cannot be a path on
>   the filesystem (otherwise we need a disambiguator "--").
>
>  - the first non-ref argument and everything that follows must be either a
>   ':' magic, a string with globbing character, or a path on the
>   filesystem, and none of them can be a ref.
>
> Do you want to take a stab at it?

A bit busy these days. If you're interested, go ahead.
-- 
Duy

Re: [PATCH] t3703, t4208: add test cases for magic pathspec

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

>> Shouldn't
>>
>>        $ git anycmd :
>>
>> be equivalent to
>>
>>        $ (cd $(git rev-parse --show-cdup)/. && git anycmd)
>>
>> for any command?  I doubt this test is expecting the right outcome.
>> Shouldn't it result in "Nothing specified, nothing added."?
>
> It's gray area.

I do not see anything gray in it.

"git add :" in subdirectory is simply broken because we have used "No
remaining command line argument means no pathspec" when deciding to stop
and say "Nothing specified, nothing added".  That was a sensible
assumption back then, but it needs to be updated to take the new "The
remaining command line argument may say that there is no pathspec" reality
into account.

Re: [PATCH] t3703, t4208: add test cases for magic pathspec

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> 2011/5/9 Junio C Hamano <gitster@pobox.com>:
>>> +test_expect_failure 'colon alone magic can only used alone' '
>>> +     test_must_fail git add -n sub/foo : &&
>>> +     test_must_fail git add -n : sub/foo
>>> +'
>>
>> I don't care too much about this case (it is a user error), but if you
>> promise you will turn this expect-failure to expect-success in a follow-up
>> patch, why not ;-)?
>
> failed tests are (annoying enough) to be fixed, right? :)

Didn't I say it is not my itch already?

[PATCH 0/9] magic pathspec updates

[Junio C Hamano]

This is a series to update the magic pathspec topic, primarily the area
around the empty ':' pathspec.

The failing tests from Duy's are mostly due to the fact that this round is
not converting the codebase fully and still use char **pathspec in places.

As get_pathspec() gives only a munged string for pattern matching, while
losing the original pathspec information such as up to which point in the
resulting string came from the prefix (hence it must match literally even
under icase) or what the original string looked like, there is no way for
these tests, especially the ones to check failure mode and the information
in the error report, to pass.

As NEEDSWORK comment in prefix_pathspec() says, all the callers need to be
converted to use "struct pathspec" interface, so that they can preserve
the original information for error reporting purposes (and also to pass
to external subprocesses --- see how pathspec is used to spawn the "add -i"
helper for example).

Other than that, I think the current code is probably more or less safe to
dogfood with known rough edges.

Junio C Hamano (8):
  grep: use get_pathspec() correctly
  get_pathspec(): an empty ':' pathspec should stand alone
  count_pathspec(): return number of elements in pathspec
  add ":" is a pathspec that is too wide
  git rm ":" is like specifying nothing
  clean ":" is like specifying nothing
  mv ":" ":" is like moving nothing from nowhere to nowhere
  checkout ":" is not giving any pathspec

Nguyễn Thái Ngọc Duy (1):
  t3703, t4208: add test cases for magic pathspec

 Documentation/glossary-content.txt |    3 +
 builtin/add.c                      |    4 +-
 builtin/checkout.c                 |    9 +---
 builtin/clean.c                    |    1 +
 builtin/grep.c                     |    8 +---
 builtin/mv.c                       |    6 ++-
 builtin/rm.c                       |    4 +-
 cache.h                            |    3 +-
 setup.c                            |   18 ++++++++-
 t/t3703-add-magic-pathspec.sh      |   78 ++++++++++++++++++++++++++++++++++++
 t/t4208-log-magic-pathspec.sh      |   45 +++++++++++++++++++++
 11 files changed, 160 insertions(+), 19 deletions(-)
 create mode 100755 t/t3703-add-magic-pathspec.sh
 create mode 100755 t/t4208-log-magic-pathspec.sh

-- 
1.7.5.1.290.g1b565

[PATCH 1/9] grep: use get_pathspec() correctly

[Junio C Hamano]

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 <gitster@pobox.com>
---
 builtin/grep.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 0bf8c01..222dd6d 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;
-- 
1.7.5.1.290.g1b565

[PATCH 2/9] get_pathspec(): an empty ':' pathspec should stand alone

[Junio C Hamano]

"git cmd foo :" is a user error; diagnose it as such.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/setup.c b/setup.c
index 51e354c..c1be388 100644
--- a/setup.c
+++ b/setup.c
@@ -256,6 +256,7 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 	const char *entry = *pathspec;
 	const char **src, **dst;
 	int prefixlen;
+	int has_root_widen = 0;
 
 	if (!prefix && !entry)
 		return NULL;
@@ -272,10 +273,15 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 	dst = pathspec;
 	prefixlen = prefix ? strlen(prefix) : 0;
 	while (*src) {
-		*(dst++) = prefix_pathspec(prefix, prefixlen, *src);
+		const char *elem = prefix_pathspec(prefix, prefixlen, *src);
+		*(dst++) = elem;
+		if (!elem)
+			has_root_widen = 1;
 		src++;
 	}
 	*dst = NULL;
+	if (has_root_widen && src != pathspec + 1)
+		die("an empty ':' pathspec with other pathspecs");
 	if (!*pathspec)
 		return NULL;
 	return pathspec;
-- 
1.7.5.1.290.g1b565

[PATCH 3/9] count_pathspec(): return number of elements in pathspec

[Junio C Hamano]

When fed a non-empty argv[], get_pathspec(prefix, argv) used to always
return an array of string that has the same number of elements as argv[]
had, but with ":" (work at toplevel without any path limit) magic pathspec
it can return zero elements.  Passing the result from get_pathspec() to
this function will always give the correct number of pathspec elements.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h |    3 ++-
 setup.c |   10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/cache.h b/cache.h
index be6ce72..6170dce 100644
--- a/cache.h
+++ b/cache.h
@@ -425,7 +425,8 @@ extern void set_git_work_tree(const char *tree);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
-extern const char **get_pathspec(const char *prefix, const char **pathspec);
+extern const char **get_pathspec(const char *prefix, const char **argv);
+extern int count_pathspec(const char **pathspec);
 extern void setup_work_tree(void);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
diff --git a/setup.c b/setup.c
index c1be388..cab9ddd 100644
--- a/setup.c
+++ b/setup.c
@@ -287,6 +287,16 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 	return pathspec;
 }
 
+int count_pathspec(const char **pathspec)
+{
+	int i;
+	if (!pathspec)
+		return 0;
+	for (i = 0; pathspec[i]; i++)
+		; /* just counting */
+	return i;
+}
+
 /*
  * Test if it looks like we're at a git directory.
  * We want to see:
-- 
1.7.5.1.290.g1b565

Re: [PATCH 3/9] count_pathspec(): return number of elements in pathspec

[Nguyen Thai Ngoc Duy]

On Tue, May 10, 2011 at 12:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> When fed a non-empty argv[], get_pathspec(prefix, argv) used to always
> return an array of string that has the same number of elements as argv[]
> had, but with ":" (work at toplevel without any path limit) magic pathspec
> it can return zero elements.  Passing the result from get_pathspec() to
> this function will always give the correct number of pathspec elements.

And use the new function in builtin/rm.c too? A good cleanup while
you're touching cmd_rm()..
-- 
Duy

[PATCH 4/9] add ":" is a pathspec that is too wide

[Junio C Hamano]

"git cmd :" should be exactly the same as "git cmd" run from the root
level of the working tree.  "git add" without any pathspec should not
add anything.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/add.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index e127d5a..43a8aad 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -406,12 +406,12 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		 (!(addremove || take_worktree_changes)
 		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
 
-	if (require_pathspec && argc == 0) {
+	pathspec = validate_pathspec(argc, argv, prefix);
+	if (require_pathspec && (!argc || !pathspec)) {
 		fprintf(stderr, "Nothing specified, nothing added.\n");
 		fprintf(stderr, "Maybe you wanted to say 'git add .'?\n");
 		return 0;
 	}
-	pathspec = validate_pathspec(argc, argv, prefix);
 
 	if (read_cache() < 0)
 		die("index file corrupt");
-- 
1.7.5.1.290.g1b565

[PATCH 5/9] git rm ":" is like specifying nothing

[Junio C Hamano]

"git cmd :" should be exactly the same as running "git cmd" from the
root level of the working tree.  Do not decide solely on the value of
argc (i.e. remaining parameters after options and revs are parsed) to
see if the user limited the operation with paths.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/rm.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index ff491d7..c31f915 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -162,11 +162,13 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 		die("index file corrupt");
 
 	pathspec = get_pathspec(prefix, argv);
+	if (!pathspec)
+		die("removing nothing?");
 	refresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);
 
 	seen = NULL;
 	for (i = 0; pathspec[i] ; i++)
-		/* nothing */;
+		; /* nothing */
 	seen = xcalloc(i, 1);
 
 	for (i = 0; i < active_nr; i++) {
-- 
1.7.5.1.290.g1b565

[PATCH 6/9] clean ":" is like specifying nothing

[Junio C Hamano]

Adjust argc that is meant to correspond do the number of elements in
the pathspec array.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/clean.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 4a312ab..686cd6b 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -96,6 +96,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 		add_exclude(exclude_list.items[i].string, "", 0, dir.exclude_list);
 
 	pathspec = get_pathspec(prefix, argv);
+	argc = count_pathspec(pathspec);
 
 	fill_directory(&dir, pathspec);
 
-- 
1.7.5.1.290.g1b565

Re: [PATCH 6/9] clean ":" is like specifying nothing

[Thiago Farina]

On Tue, May 10, 2011 at 2:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Adjust argc that is meant to correspond do the number of elements in
s/do/to ?

> the pathspec array.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/clean.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index 4a312ab..686cd6b 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -96,6 +96,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>                add_exclude(exclude_list.items[i].string, "", 0, dir.exclude_list);
>
>        pathspec = get_pathspec(prefix, argv);
> +       argc = count_pathspec(pathspec);
>
>        fill_directory(&dir, pathspec);
>
> --
> 1.7.5.1.290.g1b565
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH 7/9] mv ":" ":" is like moving nothing from nowhere to nowhere

[Junio C Hamano]

Make sure the code can deal with get_pathspec() that returns nothing.
Strictly speaking it may be a user error, but that is not an excuse
to dump core.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/mv.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 93e8995..38af9f0 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -77,8 +77,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		die("index file corrupt");
 
 	source = copy_pathspec(prefix, argv, argc, 0);
-	modes = xcalloc(argc, sizeof(enum update_mode));
+	if (!source)
+		die("copying from nowhere?");
+	modes = xcalloc(count_pathspec(source), sizeof(enum update_mode));
 	dest_path = copy_pathspec(prefix, argv + argc, 1, 0);
+	if (!dest_path)
+		die("copying to nowhere?");
 
 	if (dest_path[0][0] == '\0')
 		/* special case: "." was normalized to "" */
-- 
1.7.5.1.290.g1b565

Re: [PATCH 7/9] mv ":" ":" is like moving nothing from nowhere to nowhere

[Nguyen Thai Ngoc Duy]

On Tue, May 10, 2011 at 12:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 93e8995..38af9f0 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -77,8 +77,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>                die("index file corrupt");
>
>        source = copy_pathspec(prefix, argv, argc, 0);
> -       modes = xcalloc(argc, sizeof(enum update_mode));
> +       if (!source)
> +               die("copying from nowhere?");
> +       modes = xcalloc(count_pathspec(source), sizeof(enum update_mode));
>        dest_path = copy_pathspec(prefix, argv + argc, 1, 0);
> +       if (!dest_path)
> +               die("copying to nowhere?");
>

This command is interesting. Note to self: adding support for "git mv
'*.c' to/there".

I think we could take get_pathspec() out of copy_pathspec() and call
it just once (in case get_pathspec() or its successor does fancy
things), something roughly like this

diff --git a/builtin/mv.c b/builtin/mv.c
index 40f33ca..a032789 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -32,7 +32,7 @@ static const char **copy_pathspec(const char
*prefix, const char **pathspec,
 			result[i] = base_name ? strdup(basename(it)) : it;
 		}
 	}
-	return get_pathspec(prefix, result);
+	return result;
 }

 static const char *add_slash(const char *path)
@@ -60,7 +60,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN('k', NULL, &ignore_errors, "skip move/rename errors"),
 		OPT_END(),
 	};
-	const char **source, **destination, **dest_path;
+	const char **pathspec, **source, **destination, **dest_path;
 	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
@@ -69,16 +69,20 @@ int cmd_mv(int argc, const char **argv, const char *prefix)

 	argc = parse_options(argc, argv, prefix, builtin_mv_options,
 			     builtin_mv_usage, 0);
-	if (--argc < 1)
+
+	pathspec = get_pathspec(prefix, argv);
+	argc = count_pathspec(pathspec);
+
+	if (--argc < 1 || !pathspec)
 		usage_with_options(builtin_mv_usage, builtin_mv_options);

 	newfd = hold_locked_index(&lock_file, 1);
 	if (read_cache() < 0)
 		die(_("index file corrupt"));

-	source = copy_pathspec(prefix, argv, argc, 0);
+	source = copy_pathspec(prefix, pathspec, argc, 0);
 	modes = xcalloc(argc, sizeof(enum update_mode));
-	dest_path = copy_pathspec(prefix, argv + argc, 1, 0);
+	dest_path = copy_pathspec(prefix, pathspec + argc, 1, 0);

 	if (dest_path[0][0] == '\0')
 		/* special case: "." was normalized to "" */
-- 
Duy

[PATCH 8/9] checkout ":" is not giving any pathspec

[Junio C Hamano]

"git cmd :" should be exactly the same as running "git cmd" from the
root level of the working tree.  Do not decide solely on the value of
argc (i.e. remaining parameters after options and revs are parsed),
but make sure we actually do have pathspec to switch operation modes.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/checkout.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2bf02f2..fe46725 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -890,6 +890,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	char *conflict_style = NULL;
 	int patch_mode = 0;
 	int dwim_new_local_branch = 1;
+	const char **pathspec;
 	struct option options[] = {
 		OPT__QUIET(&opts.quiet, "suppress progress reporting"),
 		OPT_STRING('b', NULL, &opts.new_branch, "branch",
@@ -1004,12 +1005,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	if (opts.track == BRANCH_TRACK_UNSPECIFIED)
 		opts.track = git_branch_track;
 
-	if (argc) {
-		const char **pathspec = get_pathspec(prefix, argv);
-
-		if (!pathspec)
-			die("invalid path specification");
-
+	pathspec = get_pathspec(prefix, argv);
+	if (argc && pathspec) {
 		if (patch_mode)
 			return interactive_checkout(new.name, pathspec, &opts);
 
-- 
1.7.5.1.290.g1b565

[PATCH 9/9] t3703, t4208: add test cases for magic pathspec

[Junio C Hamano]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

While at it, also document ":" syntax.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/glossary-content.txt |    3 +
 t/t3703-add-magic-pathspec.sh      |   78 ++++++++++++++++++++++++++++++++++++
 t/t4208-log-magic-pathspec.sh      |   45 +++++++++++++++++++++
 3 files changed, 126 insertions(+), 0 deletions(-)
 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 0ca029b..0318024 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -327,6 +327,9 @@ icase;;
 +
 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 <<def_commit_object,commit object>> 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 0000000..9e6708a
--- /dev/null
+++ b/t/t3703-add-magic-pathspec.sh
@@ -0,0 +1,78 @@
+#!/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 'colon alone magic should only used alone' '
+	test_must_fail git add -n sub/foo : &&
+	test_must_fail git add -n : sub/foo
+'
+
+test_expect_success 'add :' '
+	: >expected &&
+	(cd sub && git add -n : >actual) &&
+	test_cmp expected sub/actual
+'
+
+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 <<EOF
+add 'anothersub/foo'
+EOF
+
+test_expect_success 'add :/anothersub' '
+	(cd sub && git add -n :/anothersub >actual) &&
+	test_cmp expected sub/actual
+'
+
+test_expect_success 'add :/non-existent' '
+	(cd sub && test_must_fail git add -n :/non-existent)
+'
+
+cat >expected <<EOF
+add 'sub/foo'
+EOF
+
+test_expect_success 'add :(icase)foo' '
+	(cd sub && git add -n ":(icase)FoO" >actual) &&
+	test_cmp expected sub/actual
+'
+
+test_expect_success 'a file with the same (long) magic name exists' '
+	: >":(icase)ha" &&
+	test_must_fail git add -n ":(icase)ha" 2>error &&
+	git add -n "./:(icase)ha"
+'
+
+cat >expected <<EOF
+fatal: pathspec ':(icase)ha' did not match any files
+EOF
+
+test_expect_failure 'show pathspecs exactly what are typed in' '
+	test_cmp expected error
+'
+
+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 0000000..ff7f485
--- /dev/null
+++ b/t/t4208-log-magic-pathspec.sh
@@ -0,0 +1,45 @@
+#!/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_failure '"git log :/" should be ambiguous' '
+	test_must_fail git log :/ 2>error &&
+	grep ambiguous error
+'
+
+test_expect_failure '"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_expect_success 'git log HEAD -- :' '
+	cat >expected <<-EOF &&
+	41d179c empty
+	24b24cf initial
+	EOF
+	(cd sub && git log --oneline HEAD -- : >../actual) &&
+	test_cmp expected actual
+'
+
+test_done
-- 
1.7.5.1.290.g1b565

[PATCH jc/magic-pathspec] t3703: Skip tests using directory name ":" on Windows

[Johannes Sixt]

From: Johannes Sixt <j6t@kdbg.org>

":" is not allowed in file names on Windows. Detect this case and skip a
test if necessary.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t3703-add-magic-pathspec.sh |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/t3703-add-magic-pathspec.sh b/t/t3703-add-magic-pathspec.sh
index ce5585e..e508246 100755
--- a/t/t3703-add-magic-pathspec.sh
+++ b/t/t3703-add-magic-pathspec.sh
@@ -44,8 +44,12 @@ test_expect_success 'a file with the same (long) magic name exists' '
 	git add -n "./:(icase)ha"
 '
 
-test_expect_success 'a file with the same (short) magic name exists' '
-	mkdir ":" &&
+if mkdir ":" 2>/dev/null
+then
+	test_set_prereq COLON_DIR
+fi
+
+test_expect_success COLON_DIR 'a file with the same (short) magic name exists' '
 	: >":/bar" &&
 	test_must_fail git add -n :/bar &&
 	git add -n "./:/bar"
-- 
1.7.5.1.1644.g7f2ed

Re: [PATCH 9/9] t3703, t4208: add test cases for magic pathspec

[Ævar Arnfjörð Bjarmason]

On Tue, May 10, 2011 at 07:51, Junio C Hamano <gitster@pobox.com> wrote:

> +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
> +"

This needs to use test_i18ncmp.

Re: [PATCH 9/9] t3703, t4208: add test cases for magic pathspec

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, May 10, 2011 at 07:51, Junio C Hamano <gitster@pobox.com> wrote:
>
>> +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
>> +"
>
> This needs to use test_i18ncmp.

I don't think so (at least not yet).

Doesn't it come from read-cache.c::add_to_index()?

Re: [PATCH 9/9] t3703, t4208: add test cases for magic pathspec

[Ævar Arnfjörð Bjarmason]

On Sun, May 29, 2011 at 22:31, Junio C Hamano <gitster@pobox.com> wrote:

>> This needs to use test_i18ncmp.
>
> I don't think so (at least not yet).
>
> Doesn't it come from read-cache.c::add_to_index()?

Doh, this was because I had a local patch by Jonathan in my tree that
I forgot was there. Sorry about the noise.

Re: [PATCH 0/9] magic pathspec updates

[Nguyen Thai Ngoc Duy]

On Tue, May 10, 2011 at 12:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> This is a series to update the magic pathspec topic, primarily the area
> around the empty ':' pathspec.

I think you missed git-commit and git-reset. "git commit --include :"
may get past parse_and_validate_options() (ie. not dying) but later on
in prepare_index(), there's no pathspec. I did not check very careful
though.

> Other than that, I think the current code is probably more or less safe to
> dogfood with known rough edges.

Agreed.
-- 
Duy

Re: [PATCH 0/9] magic pathspec updates

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Tue, May 10, 2011 at 12:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> This is a series to update the magic pathspec topic, primarily the area
>> around the empty ':' pathspec.
>
> I think you missed git-commit and git-reset. "git commit --include :"
> may get past parse_and_validate_options() (ie. not dying) but later on
> in prepare_index(), there's no pathspec. I did not check very careful
> though.
>
>> Other than that, I think the current code is probably more or less safe to
>> dogfood with known rough edges.
>
> Agreed.

After re-reading the series one more time (including the ones that are
already in next), I think teaching get_pathspec() about a lone ":" was
probably a bad idea. The only reason we might want to say "there is no
pathspec whatsoever" is when a command wants to limit its operation to
the current subdirectory and it changes behaviour when run at the root
level with and without pathspec (e.g. history simplification).

Note that no such command exists in our vocabulary today, so you need
to imagine "git local-log" that acts like "git log" but by default
shows history simplified to explain only the subdirectory you are
currently in, or something.

Such an application can easily notice that argv[] has only a lone ":"
left and do the same thing it does when there is no pathspec, without
affecting other (existing) users of get_pathspec().

But the thing is, we do not have such an application like "local-log"
now, we know we can easily support such commands when needed without
teaching get_pathspec() about a lone ":", and we can be sure that
existing commands that know get_pathspec() will never give them an
empty pathspec if they feed a non-empty argv[] will not be broken by
not teaching get_pathspec() about a lone ":".

I am leaning toward ripping the lone ":" support from the code in
"next". I would also remove ":(icase)" from "next". It was only meant
to be a POC to show how far we could go only by futzing get_pathspec()
function, and was never meant to be a serious implementation of the
feature. It should be re-done after we do deeper conversion and use
the "struct pathspec" interface not "char **" interface.

Re: [PATCH 0/9] magic pathspec updates

[Nguyen Thai Ngoc Duy]

On Wed, May 11, 2011 at 12:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Note that no such command exists in our vocabulary today, so you need
> to imagine "git local-log" that acts like "git log" but by default
> shows history simplified to explain only the subdirectory you are
> currently in, or something.
>
> Such an application can easily notice that argv[] has only a lone ":"
> left and do the same thing it does when there is no pathspec, without
> affecting other (existing) users of get_pathspec().

Hmm.. if ":" (the semantics) is only used in few commands, perhaps an
option would be better than the cryptic ":".

> I am leaning toward ripping the lone ":" support from the code in
> "next". I would also remove ":(icase)" from "next". It was only meant
> to be a POC to show how far we could go only by futzing get_pathspec()
> function, and was never meant to be a serious implementation of the
> feature. It should be re-done after we do deeper conversion and use
> the "struct pathspec" interface not "char **" interface.

OK. But reserve the lone ":" syntax. It's precious. We may have a use
case for it some day.
-- 
Duy