[PATCH 0/4] Redoing the "add -u" migration plan

[PATCH 0/4] Redoing the "add -u" migration plan

[Junio C Hamano]

So here is the new migration plan to make "add -u" without pathspec to
default to the tree wide operation at 1.8.0 boundary.

The first patch is more or less the same as the "heads-up" version I sent
earlier to implement the magic ":/" pathspec at a wrong level as a hack,
but with some documentation updates.

It should apply on top of a91df69 (the parent of the first commit in the
"jc/add-u-migration" series).  Then merge the 33c33ca (the first commit in
the "jc/add-u-migration" series) to the result, and then apply the
remainder of this series.

The second patch gets rid of treewideupdate configuration variable, as we
no longer rely on user preference for this migration plan, and rewords the
warning message.  I did it this way only because the first commit in the
old series is already in 'next'; I will redo the series after 1.7.5 ships
so that we do not have to have this patch, nor "a configuration appears
and then disappears".

The third (step 2) patch is what should happen at 1.8.0 boundary by
flipping the default, but still keeps the warning for people who missed
the late 1.7.X series.

The last (step 3) patch is to remove the warning long after 1.8.0 boundary
when everybody got used to the new behaviour.

Junio C Hamano (4):
  magic pathspec: add tentative ":/path/from/top/level" pathspec support
  add -u: get rid of "treewideupdate" configuration
  add: make "add -u/-A" update full tree without pathspec (step 2)
  add: make "add -u/-A" update full tree without pathspec (step 3)

[PATCH 1/4] magic pathspec: add tentative ":/path/from/top/level" pathspec support

[Junio C Hamano]

Support ":/" magic string that can be prefixed to a pathspec element to
say "this names the path from the top-level of the working tree", when
you are in the subdirectory.

For example, you should be able to say:

    $ edit Makefile ;# top-level
    $ cd Documentation
    $ edit git.txt ;# in the subdirectory

and then do one of three things, still inside the subdirectory:

    $ git add -u .  ;# add only Documentation/git.txt
    $ git add -u :/ ;# add everything, including paths outside Documentation
    $ git add -u    ;# whatever the default setting is.

To truly support magic pathspec, the API needs to be restructured so that
get_pathspec() and init_pathspec() are unified into one call.  Currently,
the former just prefixes the user supplied pathspec with the current
subdirectory path, and the latter takes the output from the former and
pre-parses them into a bit richer structure for easier handling.  They
should become a single API function that takes the current subdirectory
path and the remainder of argv[] (after parsing --options and revision
arguments from the command line) and returns an array of parsed pathspec
elements, and "magic" should become attributes of struct pathspec_item.

This patch implements only "top" magic because it is the only kind of
magic that can be hacked into the system without such a refactoring.
Other types of magic that are envisioned (e.g. "icase") needs to be able
to express more than what a simple string can encode and needs to wait.

The syntax for magic pathspec prefix is designed to be extensible yet
simple to type to invoke a simple magic like "from the top".  The parser
for the magic prefix is hooked into get_pathspec() function in this patch,
and it needs to be moved when we refactor the API.

But we have to start from somewhere.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/glossary-content.txt |   31 +++++++++++-
 setup.c                            |   98 +++++++++++++++++++++++++++++++++++-
 2 files changed, 126 insertions(+), 3 deletions(-)

diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index 33716a3..e51d7e6 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -277,7 +277,8 @@ This commit is referred to as a "merge commit", or sometimes just a
        Pattern used to specify paths.
 +
 Pathspecs are used on the command line of "git ls-files", "git
-ls-tree", "git grep", "git checkout", and many other commands to
+ls-tree", "git add", "git grep", "git diff", "git checkout",
+and many other commands to
 limit the scope of operations to some subset of the tree or
 worktree.  See the documentation of each command for whether
 paths are relative to the current directory or toplevel.  The
@@ -296,6 +297,34 @@ For example, Documentation/*.jpg will match all .jpg files
 in the Documentation subtree,
 including Documentation/chapter_1/figure_1.jpg.
 
++
+A pathspec that begins with a colon `:` has special meaning.  In the
+short form, the leading colon `:` is followed by zero or more "magic
+signature" letters (which optionally is terminated by another colon `:`),
+and the remainder is the pattern to match against the path. The optional
+colon that terminates the "magic signature" can be omitted if the pattern
+begins with a character that cannot be a "magic signature" and is not a
+colon.
++
+In the long form, the leading colon `:` is followed by a open
+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.
++
+The "magic signature" consists of an ASCII symbol that is not
+alphanumeric.
++
+--
+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.
+--
++
+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::
 	A <<def_commit_object,commit object>> contains a (possibly empty) list
 	of the logical predecessor(s) in the line of development, i.e. its
diff --git a/setup.c b/setup.c
index 03cd84f..820ed05 100644
--- a/setup.c
+++ b/setup.c
@@ -126,6 +126,101 @@ void verify_non_filename(const char *prefix, const char *arg)
 	    "Use '--' to separate filenames from revisions", arg);
 }
 
+/*
+ * Magic pathspec
+ *
+ * NEEDSWORK: These need to be moved to dir.h or even to a new
+ * pathspec.h when we restructure get_pathspec() users to use the
+ * "struct pathspec" interface.
+ *
+ * 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)
+
+struct pathspec_magic {
+	unsigned bit;
+	char mnemonic; /* this cannot be ':'! */
+	const char *name;
+} pathspec_magic[] = {
+	{ PATHSPEC_FROMTOP, '/', "top" },
+};
+
+/*
+ * Take an element of a pathspec and check for magic signatures.
+ * Append the result to the prefix.
+ *
+ * For now, we only parse the syntax and throw out anything other than
+ * "top" magic.
+ *
+ * NEEDSWORK: This needs to be rewritten when we start migrating
+ * get_pathspec() users to use the "struct pathspec" interface.  For
+ * example, a pathspec element may be marked as case-insensitive, but
+ * the prefix part must always match literally, and a single stupid
+ * string cannot express such a case.
+ */
+const char *prefix_pathspec(const char *prefix, int prefixlen, const char *elt)
+{
+	unsigned magic = 0;
+	const char *copyfrom = elt;
+	int i;
+
+	if (elt[0] != ':') {
+		; /* nothing to do */
+	} else if (elt[1] == '(') {
+		/* longhand */
+		const char *nextat;
+		for (copyfrom = elt + 2;
+		     *copyfrom && *copyfrom != ')';
+		     copyfrom = nextat) {
+			size_t len = strcspn(copyfrom, ",)");
+			if (copyfrom[len] == ')')
+				nextat = copyfrom + len;
+			else
+				nextat = copyfrom + len + 1;
+			if (!len)
+				continue;
+			for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
+				if (strlen(pathspec_magic[i].name) == len &&
+				    !strncmp(pathspec_magic[i].name, copyfrom, len)) {
+					magic |= pathspec_magic[i].bit;
+					break;
+				}
+			if (ARRAY_SIZE(pathspec_magic) <= i)
+				die("Invalid pathspec magic '%.*s' in '%s'",
+				    (int) len, copyfrom, elt);
+		}
+		if (*copyfrom == ')')
+			copyfrom++;
+	} else {
+		/* shorthand */
+		for (copyfrom = elt + 1;
+		     *copyfrom && *copyfrom != ':';
+		     copyfrom++) {
+			char ch = *copyfrom;
+			for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
+				if (pathspec_magic[i].mnemonic == ch) {
+					magic |= pathspec_magic[i].bit;
+					break;
+				}
+			if (ARRAY_SIZE(pathspec_magic) <= i)
+				break;
+		}
+		if (*copyfrom == ':')
+			copyfrom++;
+	}
+
+	if (magic & PATHSPEC_FROMTOP)
+		return xstrdup(copyfrom);
+	else
+		return prefix_path(prefix, prefixlen, copyfrom);
+}
+
 const char **get_pathspec(const char *prefix, const char **pathspec)
 {
 	const char *entry = *pathspec;
@@ -147,8 +242,7 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 	dst = pathspec;
 	prefixlen = prefix ? strlen(prefix) : 0;
 	while (*src) {
-		const char *p = prefix_path(prefix, prefixlen, *src);
-		*(dst++) = p;
+		*(dst++) = prefix_pathspec(prefix, prefixlen, *src);
 		src++;
 	}
 	*dst = NULL;
-- 
1.7.5.rc1

[PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Junio C Hamano]

Thanks to the magic ":/" pathspec, it is much easier to invoke both
tree-wide operation and limited-to-cwd operation on demand from the
command line.  What remains is the downside of the configuration variable,
namely, that it makes git behave differently depending on who you are and
in which repository you are using it, hence making it harder to help
and/or teach others.

Remove the configuration variable, and adjust the warning message.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/add.c         |   26 ++++++--------------------
 t/t2200-add-update.sh |   29 +++++++++++++++++++----------
 2 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 595f5cc..f58d1cf 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -310,7 +310,6 @@ static const char ignore_error[] =
 
 static int verbose = 0, show_only = 0, ignored_too = 0, refresh_only = 0;
 static int ignore_add_errors, addremove, intent_to_add, ignore_missing = 0;
-static int default_tree_wide_update = -1;
 
 static struct option builtin_add_options[] = {
 	OPT__DRY_RUN(&show_only, "dry run"),
@@ -336,10 +335,6 @@ static int add_config(const char *var, const char *value, void *cb)
 		ignore_add_errors = git_config_bool(var, value);
 		return 0;
 	}
-	if (!strcasecmp(var, "add.treewideupdate")) {
-		default_tree_wide_update = git_config_bool(var, value);
-		return 0;
-	}
 	return git_default_config(var, value, cb);
 }
 
@@ -368,15 +363,10 @@ static const char *warn_add_uA_180_migration_msg[] = {
 	"In release 1.8.0, running 'git add -u' (or 'git add -A') from",
 	"a subdirectory without giving any pathspec WILL take effect",
 	"on the whole working tree, not just the part under the current",
-	"directory. You can set add.treewideupdate configuration variable",
-	"to 'false' to keep the current behaviour.",
-	"You can set the configuration variable to 'true' to make the",
-	"'git add -u/-A' command without pathspec take effect on the whole",
-	"working tree now. If you do so, you can use '.' at the end of",
-	"the command, e.g. 'git add -u .' when you want to limit the",
-	"operation to the current directory.",
-	"This warning will be issued until you set the configuration variable",
-	"to either 'true' or 'false'."
+	"directory. Please make it a habit to add '.' when you want to",
+	"limit the operation to the current directory and below.",
+	"You can use ':/' at the end of the command to affect the operation",
+	"on the whole working tree.",
 };
 
 static int warn_180_migration(void)
@@ -419,12 +409,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		die("Option --ignore-missing can only be used together with --dry-run");
 	if ((addremove || take_worktree_changes) && !argc) {
 		whole_tree_add = 1;
-		if (prefix) {
-			if (default_tree_wide_update < 0)
-				default_tree_wide_update = warn_180_migration();
-			if (!default_tree_wide_update)
-				whole_tree_add = 0;
-		}
+		if (prefix)
+			whole_tree_add = warn_180_migration();
 	}
 
 	add_new_files = !take_worktree_changes && !refresh_only;
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index 7ac8b70..f7711ba 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -80,10 +80,9 @@ test_expect_success 'change gets noticed' '
 
 '
 
-test_expect_success 'update from a subdirectory without pathspec (no config)' '
+test_expect_success 'update from a subdirectory without pathspec' '
 	# This test needs to be updated to expect the whole tree
 	# update after 1.8.0 migration.
-	test_might_fail git config --remove add.treewideupdate &&
 	test_might_fail git reset check dir1 &&
 	echo changed >check &&
 	(
@@ -97,15 +96,13 @@ test_expect_success 'update from a subdirectory without pathspec (no config)' '
 	grep warning expect.warning
 '
 
-test_expect_success 'update from a subdirectory without pathspec (local)' '
-	test_when_finished "git config --remove add.treewideupdate; :" &&
-	git config add.treewideupdate false &&
+test_expect_success 'update from a subdirectory with local pathspec' '
 	test_might_fail git reset check dir1 &&
 	echo changed >check &&
 	(
 		cd dir1 &&
 		echo even more >sub2 &&
-		git add -u 2>../expect.warning
+		git add -u . 2>../expect.warning
 	) &&
 	git diff-files --name-only dir1 check >actual &&
 	echo check >expect &&
@@ -113,15 +110,27 @@ test_expect_success 'update from a subdirectory without pathspec (local)' '
 	! grep warning expect.warning
 '
 
-test_expect_success 'update from a subdirectory without pathspec (global)' '
-	test_when_finished "git config --remove add.treewideupdate; :" &&
-	git config add.treewideupdate true &&
+test_expect_success 'update from a subdirectory with magic pathspec (mnemonic)' '
 	test_might_fail git reset check dir1 &&
 	echo changed >check &&
 	(
 		cd dir1 &&
 		echo even more >sub2 &&
-		git add -u 2>../expect.warning
+		git add -u :/ 2>../expect.warning
+	) &&
+	git diff-files --name-only dir1 check >actual &&
+	: >expect &&
+	test_cmp expect actual &&
+	! grep warning expect.warning
+'
+
+test_expect_success 'update from a subdirectory with magic pathspec (longhand)' '
+	test_might_fail git reset check dir1 &&
+	echo changed >check &&
+	(
+		cd dir1 &&
+		echo even more >sub2 &&
+		git add -u ":(top)" 2>../expect.warning
 	) &&
 	git diff-files --name-only dir1 check >actual &&
 	: >expect &&
-- 
1.7.5.rc1

[PATCH 3/4] add: make "add -u/-A" update full tree without pathspec (step 2)

[Junio C Hamano]

Flip the default behaviour when "git add -u/-A" is run without a
pathspec from a subdirectory to tree-wide, and reword the advice
message.

We will need to keep the advice message for a while to help people
who skipped the 1.8.0 boundary.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/add.c         |    8 ++++----
 t/t2200-add-update.sh |    2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index f58d1cf..6e6cdc0 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -360,13 +360,13 @@ static int add_files(struct dir_struct *dir, int flags)
 }
 
 static const char *warn_add_uA_180_migration_msg[] = {
-	"In release 1.8.0, running 'git add -u' (or 'git add -A') from",
-	"a subdirectory without giving any pathspec WILL take effect",
+	"Since release 1.8.0, running 'git add -u' (or 'git add -A')",
+	"from a subdirectory without giving any pathspec takes effect",
 	"on the whole working tree, not just the part under the current",
 	"directory. Please make it a habit to add '.' when you want to",
 	"limit the operation to the current directory and below.",
 	"You can use ':/' at the end of the command to affect the operation",
-	"on the whole working tree.",
+	"on the whole working tree, if you want to be explicit.",
 };
 
 static int warn_180_migration(void)
@@ -374,7 +374,7 @@ static int warn_180_migration(void)
 	int i;
 	for (i = 0; i < ARRAY_SIZE(warn_add_uA_180_migration_msg); i++)
 		warning("%s", warn_add_uA_180_migration_msg[i]);
-	return 0; /* default to "no" (not tree-wide, i.e. local) */
+	return 1; /* default to "true" (tree-wide, i.e. not local) */
 }
 
 int cmd_add(int argc, const char **argv, const char *prefix)
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index f7711ba..a601394 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -91,7 +91,7 @@ test_expect_success 'update from a subdirectory without pathspec' '
 		git add -u 2>../expect.warning
 	) &&
 	git diff-files --name-only dir1 check >actual &&
-	echo check >expect &&
+	: >expect &&
 	test_cmp expect actual &&
 	grep warning expect.warning
 '
-- 
1.7.5.rc1

[PATCH 4/4] add: make "add -u/-A" update full tree without pathspec (step 3)

[Junio C Hamano]

Now long after 1.8.0 happened, people should have got used to the
new default behaviour and it is no longer necessary to give the
migration advice anymore.

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

diff --git a/builtin/add.c b/builtin/add.c
index 6e6cdc0..3564d7e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -359,24 +359,6 @@ static int add_files(struct dir_struct *dir, int flags)
 	return exit_status;
 }
 
-static const char *warn_add_uA_180_migration_msg[] = {
-	"Since release 1.8.0, running 'git add -u' (or 'git add -A')",
-	"from a subdirectory without giving any pathspec takes effect",
-	"on the whole working tree, not just the part under the current",
-	"directory. Please make it a habit to add '.' when you want to",
-	"limit the operation to the current directory and below.",
-	"You can use ':/' at the end of the command to affect the operation",
-	"on the whole working tree, if you want to be explicit.",
-};
-
-static int warn_180_migration(void)
-{
-	int i;
-	for (i = 0; i < ARRAY_SIZE(warn_add_uA_180_migration_msg); i++)
-		warning("%s", warn_add_uA_180_migration_msg[i]);
-	return 1; /* default to "true" (tree-wide, i.e. not local) */
-}
-
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
 	int exit_status = 0;
@@ -407,11 +389,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		die("-A and -u are mutually incompatible");
 	if (!show_only && ignore_missing)
 		die("Option --ignore-missing can only be used together with --dry-run");
-	if ((addremove || take_worktree_changes) && !argc) {
+	if ((addremove || take_worktree_changes) && !argc)
 		whole_tree_add = 1;
-		if (prefix)
-			whole_tree_add = warn_180_migration();
-	}
 
 	add_new_files = !take_worktree_changes && !refresh_only;
 	require_pathspec = !take_worktree_changes;
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index a601394..3ad6bff 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -81,8 +81,6 @@ test_expect_success 'change gets noticed' '
 '
 
 test_expect_success 'update from a subdirectory without pathspec' '
-	# This test needs to be updated to expect the whole tree
-	# update after 1.8.0 migration.
 	test_might_fail git reset check dir1 &&
 	echo changed >check &&
 	(
@@ -93,7 +91,7 @@ test_expect_success 'update from a subdirectory without pathspec' '
 	git diff-files --name-only dir1 check >actual &&
 	: >expect &&
 	test_cmp expect actual &&
-	grep warning expect.warning
+	! grep warning expect.warning
 '
 
 test_expect_success 'update from a subdirectory with local pathspec' '
-- 
1.7.5.rc1

Re: [PATCH 1/4] magic pathspec: add tentative ":/path/from/top/level" pathspec support

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> This patch implements only "top" magic because it is the only kind of
> magic that can be hacked into the system without such a refactoring.
> Other types of magic that are envisioned (e.g. "icase") needs to be able
> to express more than what a simple string can encode and needs to wait.

Actually "icase" could be implemented inside get_pathspec() by doing
something like the attached patch.

But "noglob" needs support from the "struct pathspec_item" infrastructure.
It is insufficient to parse the magic signature at get_pathspec() level,
as I do not see a way to encode these magic in the match string.

I suspect that Duy's favourite "negate" cannot be done if we discard the
magic information at the get_pathspec() level, either.

---
 Documentation/glossary-content.txt |    7 +++++--
 setup.c                            |   31 +++++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index e51d7e6..0ca029b 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -319,10 +319,13 @@ 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`.
 --
 +
-Currently only the slash `/` is recognized as the "magic signature",
-but it is envisioned that we will support more types of magic in later
+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 820ed05..e66ffbe 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,6 +149,7 @@ struct pathspec_magic {
 	const char *name;
 } pathspec_magic[] = {
 	{ PATHSPEC_FROMTOP, '/', "top" },
+	{ PATHSPEC_ICASE, '\0', "icase" },
 };
 
 /*
@@ -168,7 +169,8 @@ const char *prefix_pathspec(const char *prefix, int prefixlen, const char *elt)
 {
 	unsigned magic = 0;
 	const char *copyfrom = elt;
-	int i;
+	const char *retval;
+	int i, free_source = 0;
 
 	if (elt[0] != ':') {
 		; /* nothing to do */
@@ -215,10 +217,31 @@ 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)
-		return xstrdup(copyfrom);
+		retval = xstrdup(copyfrom);
 	else
-		return prefix_path(prefix, prefixlen, copyfrom);
+		retval = prefix_path(prefix, prefixlen, copyfrom);
+	if (free_source)
+		free((char *)copyfrom);
+	return retval;
 }
 
 const char **get_pathspec(const char *prefix, const char **pathspec)

Re: [PATCH 1/4] magic pathspec: add tentative ":/path/from/top/level" pathspec support

[Nguyen Thai Ngoc Duy]

2011/4/7 Junio C Hamano <gitster@pobox.com>:
> +                       if (('a' <= ch && ch <= 'z') ||
> +                           ('A' <= ch && ch <= 'Z')) {
> +                               strbuf_addf(&sb, "[%c%c]",
> +                                           tolower(ch), toupper(ch));

Nice try. You know you are going to pay a high performance price for
that, don't you ;) Maybe also worth mentioning in document that this
applies to ASCII charset only (as opposed to Unicode).
-- 
Duy

Re: [PATCH 1/4] magic pathspec: add tentative ":/path/from/top/level" pathspec support

[Nguyen Thai Ngoc Duy]

On Thu, Apr 7, 2011 at 8:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> +The "magic signature" consists of an ASCII symbol that is not
> +alphanumeric.

Except dot, as Michael pointed out in another email, so we can write
":/.foo" instead of ":/:.foo". I'm tempted to rule out wildcards as
well (star, backslash, question mark and square brackets)

Also the patch does not catch this (ie. not die() on unrecognized signature).
-- 
Duy

Re: [PATCH 1/4] magic pathspec: add tentative ":/path/from/top/level" pathspec support

[Junio C Hamano]

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

> Also the patch does not catch this (ie. not die() on unrecognized signature).

Of course it doesn't.

I didn't say "everything that is not alphanumeric is magic". I only said
"all the magic are not alphanumeric".  Notice the difference.


This is so that you can say ":/.gitignore" and do not have to say
":/:.gitignore".

I am also tempted to special case a ':' followed by a zero magic as if it
specifies the top magic, e.g. ":Makefile" is the same as ":(top)Makefile",
aka ":/Makefile".  It is not in the published code, but the short-cut
would help ":.gitignore".

You don't require, nor allowed to have, a colon after ')' when writing a
long hand, by the way.  There is no reason to require one for disambiguation.

Re: [PATCH 1/4] magic pathspec: add tentative ":/path/from/top/level" pathspec support

[Junio C Hamano]

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

> 2011/4/7 Junio C Hamano <gitster@pobox.com>:
>> +                       if (('a' <= ch && ch <= 'z') ||
>> +                           ('A' <= ch && ch <= 'Z')) {
>> +                               strbuf_addf(&sb, "[%c%c]",
>> +                                           tolower(ch), toupper(ch));
>
> Nice try. You know you are going to pay a high performance price for
> that, don't you ;) Maybe also worth mentioning in document that this
> applies to ASCII charset only (as opposed to Unicode).

You know this is a throw-away patch, just to illustrate that some things
are doable with a hack to add more code to get_pathspec(), while others
would need a bigger restructuring, don't you?

Besides, _if_ the user wants to do something costly, as long as the
implementation does not harm common cases, it _still_ is better to make
the code do the work, no?

Re: [PATCH 1/4] magic pathspec: add tentative ":/path/from/top/level" pathspec support

[Nguyen Thai Ngoc Duy]

On Fri, Apr 8, 2011 at 1:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> 2011/4/7 Junio C Hamano <gitster@pobox.com>:
>>> +                       if (('a' <= ch && ch <= 'z') ||
>>> +                           ('A' <= ch && ch <= 'Z')) {
>>> +                               strbuf_addf(&sb, "[%c%c]",
>>> +                                           tolower(ch), toupper(ch));
>>
>> Nice try. You know you are going to pay a high performance price for
>> that, don't you ;) Maybe also worth mentioning in document that this
>> applies to ASCII charset only (as opposed to Unicode).
>
> You know this is a throw-away patch, just to illustrate that some things
> are doable with a hack to add more code to get_pathspec(), while others
> would need a bigger restructuring, don't you?
>
> Besides, _if_ the user wants to do something costly, as long as the
> implementation does not harm common cases, it _still_ is better to make
> the code do the work, no?

I was thinking of the latter rather than the former. And I agree. Just
wondering how slow it (fnmatch in general) can be compared to plain
prefix. I guess we will know soon if this patch gets in and users
start to use it.
-- 
Duy

Re: [PATCH 1/4] magic pathspec: add tentative ":/path/from/top/level" pathspec support

[Nguyen Thai Ngoc Duy]

On Thu, Apr 7, 2011 at 11:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> Also the patch does not catch this (ie. not die() on unrecognized signature).
>
> Of course it doesn't.
>
> I didn't say "everything that is not alphanumeric is magic". I only said
> "all the magic are not alphanumeric".  Notice the difference.
>
>
> This is so that you can say ":/.gitignore" and do not have to say
> ":/:.gitignore".

But then, say people have a file named @foo at top dir. They can write
":/@foo" to address the file. Some time later we decide to use '@' as
magic, how can we re-train user's fingers?

> I am also tempted to special case a ':' followed by a zero magic as if it
> specifies the top magic, e.g. ":Makefile" is the same as ":(top)Makefile",
> aka ":/Makefile".  It is not in the published code, but the short-cut
> would help ":.gitignore".

If you're not too tempted to do it, then reserve the case (ie. die()).
Although I quite like it, one less keystroke for me.
-- 
Duy

Re: [PATCH 1/4] magic pathspec: add tentative ":/path/from/top/level" pathspec support

[Junio C Hamano]

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

> On Thu, Apr 7, 2011 at 11:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> This is so that you can say ":/.gitignore" and do not have to say
>> ":/:.gitignore".
>
> But then, say people have a file named @foo at top dir. They can write
> ":/@foo" to address the file. Some time later we decide to use '@' as
> magic, how can we re-train user's fingers?

You don't.  The primary goal of short form is to be short to type from the
command line, and if you are in doubt, you can always disambiguate by
saying ":/:@foo", and you can use the terminating colon even if you are
sure "@" is not a magic in your version of git.

Scripts can and should use the long form for readability and compatibility.

> Although I quite like it, one less keystroke for me.

Exactly.  It is Ok if the short form is a bit more complex to explain than
the long form (which should be very easy to explain).  The goal of the
short form is to make the end result is short to type, once the user
understands the rules.

Re: [PATCH 1/4] magic pathspec: add tentative ":/path/from/top/level" pathspec support

[Nguyen Thai Ngoc Duy]

On Fri, Apr 8, 2011 at 10:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> On Thu, Apr 7, 2011 at 11:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> This is so that you can say ":/.gitignore" and do not have to say
>>> ":/:.gitignore".
>>
>> But then, say people have a file named @foo at top dir. They can write
>> ":/@foo" to address the file. Some time later we decide to use '@' as
>> magic, how can we re-train user's fingers?
>
> You don't.  The primary goal of short form is to be short to type from the
> command line, and if you are in doubt, you can always disambiguate by
> saying ":/:@foo", and you can use the terminating colon even if you are
> sure "@" is not a magic in your version of git.

If you allow me to use ":/@foo", I would (because it's convenient).
And over time it will be carved in my muscle memory. Doubts and
surprises after that are not good. Suppose I usually do "git co
:/@foo", then '@' in later versions means many files, not just '@foo'
at top. The '@' magic surprise would upset (enrage in the first few
minutes maybe) me. My argument would be "it used to work fine"
(against "you should use 'git co :/:@foo'" because it's less
convenient that way).
-- 
Duy

Re: [PATCH 1/4] magic pathspec: add tentative ":/path/from/top/level" pathspec support

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> On Thu, Apr 7, 2011 at 11:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> This is so that you can say ":/.gitignore" and do not have to say
>>> ":/:.gitignore".
>>
>> But then, say people have a file named @foo at top dir. They can write
>> ":/@foo" to address the file. Some time later we decide to use '@' as
>> magic, how can we re-train user's fingers?
>
> You don't.  The primary goal of short form is to be short to type from the
> command line, and if you are in doubt, you can always disambiguate by
> saying ":/:@foo", and you can use the terminating colon even if you are
> sure "@" is not a magic in your version of git.

Actually, after thinking a bit more about it, I changed my mind.

It is not such a big deal to require the terminating colon for a pathname
that begins with a nonalnum (except for '.' and we might find some others
perhaps '_'), as they are rare.

I agree that we should reserve most of the nonalnum ASCII as potential
magic, and require the terminating colon if the user wants to start the
pattern part with a potential magic signature letter and error out if we
see a magic that we do not support yet.  The reason I said "most of" is
that we should exclude some non-alnum letters that often begin a filename.

Off the top of my head, we should exclude "." (period -- dot-files are
common), "_" (underscore), and possibly "+" (plus) and "=" (equal), as I
saw these used to start filenames, but the latter two are rather rare and
I don't mind requiring the terminating colon after the magic signature.

Re: [PATCH 1/4] magic pathspec: add tentative ":/path/from/top/level" pathspec support

[Nguyen Thai Ngoc Duy]

On Fri, Apr 8, 2011 at 11:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Off the top of my head, we should exclude "." (period -- dot-files are
> common), "_" (underscore), and possibly "+" (plus) and "=" (equal), as I
> saw these used to start filenames, but the latter two are rather rare and
> I don't mind requiring the terminating colon after the magic signature.

Totally agree on dot and underscore. I don't really care the other
two. While '*' is not actually part of path name, '*.[ch]' is commonly
used, and I usually have to quote once to avoid bash expansion
already. So put asterisk in exclude list too, to avoid another quote?
-- 
Duy

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Jeff King]

On Wed, Apr 06, 2011 at 06:16:34PM -0700, Junio C Hamano wrote:

> Thanks to the magic ":/" pathspec, it is much easier to invoke both
> tree-wide operation and limited-to-cwd operation on demand from the
> command line.

I am mildly negative on this patch. Having the config variable helps two
types of users:

  1. Ones who see the warning for new behavior, say "great, I've been
     informed and am ready to use it", and don't want to see the message
     again. They are stuck typing "./" or ":/" every time, or end up
     getting spammed by the migration message.

  2. Users who prefer the current behavior and would rather keep it. We
     give them no out except to type "./" every time. Changing the
     default is one thing; an irate user can see the change and fix it.
     But to give them no way of changing the default back seems
     unnecessarily limiting.

> What remains is the downside of the configuration variable,
> namely, that it makes git behave differently depending on who you are and
> in which repository you are using it, hence making it harder to help
> and/or teach others.

I have never been a fan of this reasoning. Sure, it is slightly harder
to help people when the system is configurable. But dropping
configurability comes at the cost of people who are using the system
day-to-day. And isn't making it pleasant to use every day more important
than the minority of times you are telling somebody else how to use it?

Besides which, if you are helping somebody remotely or sitting at an
unfamiliar git installation, it still wouldn't be safe to recommend
pathspec-less "git add -u" without first checking which version of git
the person is running (though to be fair, in 2 or 3 years it will be
reasonable to assume a certain behavior, and a config option would still
exist).

-Peff

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> What remains is the downside of the configuration variable,
>> namely, that it makes git behave differently depending on who you are and
>> in which repository you are using it, hence making it harder to help
>> and/or teach others.
>
> I have never been a fan of this reasoning. Sure, it is slightly harder
> to help people when the system is configurable. But dropping
> configurability comes at the cost of people who are using the system
> day-to-day. And isn't making it pleasant to use every day more important
> than the minority of times you are telling somebody else how to use it?

I probably should stated it differently.  I dropped it during this round
because they are _not_ needed to help the transition, but it is a possible
additional feature.

Let me try to explain the train of throught a bit better.

People seem to expect "add -u" without any pathspec to work on the whole
working tree structure even when you are in a subdirectory, similar to
"git commit -a".  We will be changing "add -u" in the longer term to do
so.

After the transition happens, people can easily say "add -u ." to limit it
to the current subdirectory, and people can say "add -u ." even today to
be explicit (which would help them transitioning).

Before the transition, however, there is no pleasant way to cause "add -u"
to add everything when you work in a subdirectory, short of saying:

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

With ":/", we gain an easy way, "git add -u :/", to say "whole tree".
That is the only thing this series does.

Up to this part, I think we both agree are good thing.

Now, imagine you were born in a world where we had the :/ magic from day
one of git.  Different commands "git add -u", "git grep", "git ls-files"
without pathspec operate differently, and for some reason (e.g. usability,
similarity to other people's corresponding command, or historical reasons)
some of them operate only within the current subdirectory while others
operate on the whole tree.  The default behaviour might even be different
between versions of git or user configuration.

Because we assume that you already have both "." and ":/" to easily be
explicit in that world, "teachers and helpers may have to scratch there
head wasting their time" is no longer an issue.  If you are teaching
others, you ought to know about "." and ":/", and whether we add the
configuration mechanism or not, you ought to know that you should be
explicit to protect yourself from the differences between 1.7.X and 1.8.0
versions in the first place.

So I agree that "costs teachers and helpers" is much less of an issue.

You can certainly introduce configuration variables e.g. "addu.treewide",
"grep.treewide", "lsfiles.treewide" (or even "core.treewide" to cover them
all) to change the default behaviour for each user or each repository.

The real question would become: if it is worth the maintenance cost of
additional code to help users who want to avoid typing "." (or ":/") all
the time in the environment they control.  I don't know the answer to this
question yet.

A good new is that we had to conflate the discussion with "but there is no
pleasant way to tell 'default to local' commands to work on the whole
tree" justification for configuration variables before ":/".  With ":/",
that excuse will be gone and the discussion can be much more simplified.

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Jeff King]

On Fri, Apr 08, 2011 at 12:27:05PM -0700, Junio C Hamano wrote:

> > I have never been a fan of this reasoning. Sure, it is slightly harder
> > to help people when the system is configurable. But dropping
> > configurability comes at the cost of people who are using the system
> > day-to-day. And isn't making it pleasant to use every day more important
> > than the minority of times you are telling somebody else how to use it?
> 
> I probably should stated it differently.  I dropped it during this round
> because they are _not_ needed to help the transition, but it is a possible
> additional feature.

But in my earlier email, one of the users who is helped by this is one
who wants to silence the migration warning. So it is somewhat related to
the transition.

If we were in a world where "." and ":/" had always worked and there was
no variable, would I write a patch for the variable? Probably not,
especially because I think the full-tree behavior is what I would set it
to (and that is going to become the default).

But we don't live in that world. We are making a transition, and so it
may be worth it to help:

  1. People who want the new behavior _now_ without extra typing.

  2. Placate people who say "...but I liked the old behavior better".

I am in group (1). I don't know if people in group (2) need placating or
not, but I have grown to assume there will always be people to complain
about a change. :)

-Peff

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But we don't live in that world. We are making a transition, and so it
> may be worth it to help:
>
>   1. People who want the new behavior _now_ without extra typing.
>
>   2. Placate people who say "...but I liked the old behavior better".
>
> I am in group (1). I don't know if people in group (2) need placating or
> not, but I have grown to assume there will always be people to complain
> about a change. :)

Ok, first things first.  Any new feature we add we would need to keep
forever.  I would rather not to see us having to maintain the default
behaviour configuration variable forever into the future for these
commands (not just add-u, but also grep, ls-files, ...).

Now, having said that, we could vastly simplify things.

I think the real cause of this confusion is because we have been dead-set
assuming that we _have_ to change the default.  The logic goes like this:

 - We will change the default, but we do not want to harm existing users;
   therefore

 - We need to introduce the variable to keep the old default not to harm
   existing users, but it would add to the uncertainty of the default when
   you do not know which version of git you are using. You now have to
   also worry about unknown configurations, so we need to train people to
   explicitly say '.' or ':/'; therefore

 - We need to issue warnings to train the user, but we do not want to show
   warnings to users who already learned the change is coming; therefore

 - We do need the configuration variable, and everybody needs to set it to
   squelch the warning. Otherwise "add -u" without any pathspec is too
   noisy to be usable.

But I am not happy with the last step of the above deduction for two
reasons.

The configuration will only help people who have total control of the
version of git they have on their boxes.  They can say "Ok, I know what I
want, and I know which default the version of git I installed on this box,
so I set the variable and can forget about it forever, and keep typing
'git add -u' without any pathspec."

But that would not work for people who need to work on multiple boxes,
some of the boxes still running late 1.7.X while others running 1.8.0 or
later, and the version of git they will be running is under sysadmin's
control (your "a site that runs unknown version" example).  These people
need to train their fingers to be explicit when working in a subdirectory
and running the command without a pathspec during the transition period
anyway.  It is a _good_ thing not to have any way to squelch the warning
without being explicit in that sense, _if_ we are going to change the
default.

Another reason, which is worse, is that it would make it _harder_ to help
migrating scripts, if the presense of the configuration variable squelched
the warning.  "Warn when run without pathspec from a subdirectory" would
issue the warning every time until the invocation is changed to an
explicit one, and your script that runs 'add -u' without pathspec will
keep emitting the message until all such invocations are fixed.

So let's step back a bit.

How about we'd just add ':/' to make it equally easy to switch between
"local only" vs "tree-wide" in 1.7.6 release, and be done with it.  We
don't change the default for any of the commands at all.

No need to issue warnings because there won't be any backward imcompatible
change.  The users fingers do not need any re-training.  There is no need
to rewrite scripts, either.

One good attribute of the combination of short-and-sweet ':/' and existing
short-and-sweet '.' is that they make the default immaterial.

Since more than a year ago, I've been saying that the ideal is to make the
default not matter:

  http://thread.gmane.org/gmane.comp.version-control.git/133570/focus=133683

If the default does not matter, why change it?  It just causes us more
headaches for dubious gain, no?

IIRC, I think the two reasons why we started discussing of "add -u" and
friends were that (1) some commands default to whole-tree while others
limit to $cwd --- inconsistency is bad; and (2) when the user wants to do
a full tree "add -u", there is no way other than counting the current
depth and typing "../" that many times.

But when we looked at the current set of commands that limit them to the $cwd,
we found that "add -u" was the only one that may make sense to switch the
default, meaning that the "consistency" was not something we would even
want to shoot for.  For example, we want our "git grep -e pattern" to
mimic "grep -r -e pattern .".

And the second reason is going away, thanks to Michael and Duy's ':/'.

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Jeff King]

On Fri, Apr 08, 2011 at 03:22:20PM -0700, Junio C Hamano wrote:

> So let's step back a bit.
> 
> How about we'd just add ':/' to make it equally easy to switch between
> "local only" vs "tree-wide" in 1.7.6 release, and be done with it.  We
> don't change the default for any of the commands at all.

Yeah, I am beginning to think that is a sensible route. And it commits
us to nothing, so if we decide much later that a change of default is
sensible, that is still open to us.

> Since more than a year ago, I've been saying that the ideal is to make the
> default not matter:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/133570/focus=133683
> 
> If the default does not matter, why change it?  It just causes us more
> headaches for dubious gain, no?

I'm not sure how much you can achieve the "make it not matter". The
shorthands go a long way, but I still want git to read my mind about
which I wanted to use (and the closest approximation of that, from my
experience, would be a per-repo variable). However, having the
shorthands mean that we can try them out in the real world and revisit
the topic in a year if people still care.

> IIRC, I think the two reasons why we started discussing of "add -u" and
> friends were that (1) some commands default to whole-tree while others
> limit to $cwd --- inconsistency is bad; and (2) when the user wants to do
> a full tree "add -u", there is no way other than counting the current
> depth and typing "../" that many times.
> 
> But when we looked at the current set of commands that limit them to the $cwd,
> we found that "add -u" was the only one that may make sense to switch the
> default, meaning that the "consistency" was not something we would even
> want to shoot for.  For example, we want our "git grep -e pattern" to
> mimic "grep -r -e pattern .".

I am not sure of that. I thought there was interest in full-tree grep
(OK, _I_ had some interst in it).  But the same transition pain
arguments apply there, and we should be able to do "git grep pattern :/"
soon, right?

-Peff

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I am not sure of that. I thought there was interest in full-tree grep
> (OK, _I_ had some interst in it).  But the same transition pain
> arguments apply there, and we should be able to do "git grep pattern :/"
> soon, right?

I never tested it myself, but the earlier "support :/ at a wrong level
get_pathspec()" patch should take care of "git grep" as well.  It is
equivalent to the "alternative approach" Michael posted as RFC as a
follow-up to his earlier "grep --full-tree" thread.

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> ... I thought there was interest in full-tree grep
>> (OK, _I_ had some interst in it).  But the same transition pain
>> arguments apply there, and we should be able to do "git grep pattern :/"
>> soon, right?
>
> I never tested it myself, but the earlier "support :/ at a wrong level
> get_pathspec()" patch should take care of "git grep" as well.  It is
> equivalent to the "alternative approach" Michael posted as RFC as a
> follow-up to his earlier "grep --full-tree" thread.

It appears that we might want to further tweak the code that tries to
disambiguate between revs and paths (we error out when argv[i] does not
name a rev and lstat(argv[i]) fails), but other than that, this command
sequence

    $ cd Documentation
    $ git grep -e purple -- :

seems to hit ../contrib/emacs/git.el and ../gitk-git/gitk correctly.

Of course, from the same directory:

    $ git grep -e purple -- :/*.el

hits ../contrib/emacs/git.el as expected.

The following patch will apply on top of 8a42c98 (magic pathspec: add
tentative ":/path/from/top/level" pathspec support, 2011-04-06).

Per our discussion, I think 'add -u' migration topics should be scrapped
for now, and rethought after giving time for people to get familiar with
the new :/ facility.

Thanks.

-- >8 --
Subject: [PATCH] magic pathspec: futureproof shorthand form

The earlier design was to take whatever non-alnum that the short format
parser happens to support, leaving the rest as part of the pattern, so a
version of git that knows '*' magic and a version that does not would have
behaved differently when given ":*Makefile".  The former would have
applied the '*' magic to the pattern "Makefile", while the latter would
used no magic to the pattern "*Makefile".

Instead, just reserve all non-alnum ASCII letters that are neither glob
nor regexp special as potential magic signature, and when we see a magic
that is not supported, die with an error message, just like the longhand
codepath does.

With this, ":%#!*Makefile" will always mean "%#!" magic applied to the
pattern "*Makefile", no matter what version of git is used (it is a
different matter if the version of git supports all of these three magic
matching rules).

Also make ':' without anything else to mean "there is no pathspec".  This
would allow differences between "git log" and "git log ." run from the top
level of the working tree (the latter simplifies no-op commits away from
the history) to be expressed from a subdirectory by saying "git log :".

Helped-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ctype.c           |   15 ++++++++-------
 git-compat-util.h |    2 ++
 setup.c           |    9 ++++++++-
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/ctype.c b/ctype.c
index de60027..b5d856f 100644
--- a/ctype.c
+++ b/ctype.c
@@ -10,17 +10,18 @@ enum {
 	A = GIT_ALPHA,
 	D = GIT_DIGIT,
 	G = GIT_GLOB_SPECIAL,	/* *, ?, [, \\ */
-	R = GIT_REGEX_SPECIAL	/* $, (, ), +, ., ^, {, | */
+	R = GIT_REGEX_SPECIAL,	/* $, (, ), +, ., ^, {, | */
+	P = GIT_PATHSPEC_MAGIC  /* other non-alnum, except for ] and } */
 };
 
 unsigned char sane_ctype[256] = {
 	0, 0, 0, 0, 0, 0, 0, 0, 0, S, S, 0, 0, S, 0, 0,		/*   0.. 15 */
 	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,		/*  16.. 31 */
-	S, 0, 0, 0, R, 0, 0, 0, R, R, G, R, 0, 0, R, 0,		/*  32.. 47 */
-	D, D, D, D, D, D, D, D, D, D, 0, 0, 0, 0, 0, G,		/*  48.. 63 */
-	0, A, A, A, A, A, A, A, A, A, A, A, A, A, A, A,		/*  64.. 79 */
-	A, A, A, A, A, A, A, A, A, A, A, G, G, 0, R, 0,		/*  80.. 95 */
-	0, A, A, A, A, A, A, A, A, A, A, A, A, A, A, A,		/*  96..111 */
-	A, A, A, A, A, A, A, A, A, A, A, R, R, 0, 0, 0,		/* 112..127 */
+	S, P, P, P, R, P, P, P, R, R, G, R, P, P, R, P,		/*  32.. 47 */
+	D, D, D, D, D, D, D, D, D, D, P, P, P, P, P, G,		/*  48.. 63 */
+	P, A, A, A, A, A, A, A, A, A, A, A, A, A, A, A,		/*  64.. 79 */
+	A, A, A, A, A, A, A, A, A, A, A, G, G, 0, R, P,		/*  80.. 95 */
+	P, A, A, A, A, A, A, A, A, A, A, A, A, A, A, A,		/*  96..111 */
+	A, A, A, A, A, A, A, A, A, A, A, R, R, 0, P, 0,		/* 112..127 */
 	/* Nothing in the 128.. range */
 };
diff --git a/git-compat-util.h b/git-compat-util.h
index 49b50ee..d88cf8a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -462,6 +462,7 @@ extern unsigned char sane_ctype[256];
 #define GIT_ALPHA 0x04
 #define GIT_GLOB_SPECIAL 0x08
 #define GIT_REGEX_SPECIAL 0x10
+#define GIT_PATHSPEC_MAGIC 0x20
 #define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
 #define isascii(x) (((x) & ~0x7f) == 0)
 #define isspace(x) sane_istest(x,GIT_SPACE)
@@ -472,6 +473,7 @@ extern unsigned char sane_ctype[256];
 #define is_regex_special(x) sane_istest(x,GIT_GLOB_SPECIAL | GIT_REGEX_SPECIAL)
 #define tolower(x) sane_case((unsigned char)(x), 0x20)
 #define toupper(x) sane_case((unsigned char)(x), 0)
+#define is_pathspec_magic(x) sane_istest(x,GIT_PATHSPEC_MAGIC)
 
 static inline int sane_case(int x, int high)
 {
diff --git a/setup.c b/setup.c
index 820ed05..5048252 100644
--- a/setup.c
+++ b/setup.c
@@ -197,19 +197,26 @@ 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;
 		     *copyfrom && *copyfrom != ':';
 		     copyfrom++) {
 			char ch = *copyfrom;
+
+			if (!is_pathspec_magic(ch))
+				break;
 			for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
 				if (pathspec_magic[i].mnemonic == ch) {
 					magic |= pathspec_magic[i].bit;
 					break;
 				}
 			if (ARRAY_SIZE(pathspec_magic) <= i)
-				break;
+				die("Unimplemented pathspec magic '%c' in '%s'",
+				    ch, elt);
 		}
 		if (*copyfrom == ':')
 			copyfrom++;
-- 
1.7.5.rc1

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Nguyen Thai Ngoc Duy]

On Fri, Apr 08, 2011 at 04:18:46PM -0700, Junio C Hamano wrote:
> It appears that we might want to further tweak the code that tries to
> disambiguate between revs and paths (we error out when argv[i] does not
> name a rev and lstat(argv[i]) fails)

Something like below? The additional goodness is, instead of writing

git grep foo -- '*.a'

I can now write a shorter version

git grep foo :./*.a

Perhaps we can use the first pathspec with magic as a mark of
pathspec arguments, equivalent to "--"

--8<--
diff --git a/setup.c b/setup.c
index 03cd84f..a00a23f 100644
--- a/setup.c
+++ b/setup.c
@@ -73,6 +73,8 @@ int check_filename(const char *prefix, const char *arg)
 	const char *name;
 	struct stat st;
 
+	if (*arg == ':')	/* pathspec magic */
+		return 1;
 	name = prefix ? prefix_filename(prefix, strlen(prefix), arg) : arg;
 	if (!lstat(name, &st))
 		return 1; /* file exists */
--8<--
-- 
Duy

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Junio C Hamano]

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

> On Fri, Apr 08, 2011 at 04:18:46PM -0700, Junio C Hamano wrote:
>> It appears that we might want to further tweak the code that tries to
>> disambiguate between revs and paths (we error out when argv[i] does not
>> name a rev and lstat(argv[i]) fails)
>
> Something like below?

That's too loose for my taste.  At that point in the codepath, don't we
sometimes expect the argv[i] might name a blob in the index?

By "we might want to further...", I meant "when we properly redesign the
get_pathspec vs init_pathspec combination".


There are places in the current code that call verify_filename() to make
sure that argv[i] is a pathspec after making sure argv[i] cannot be an
object name.  Currently it just does lstat() on it to see if it names a
path.

Instead, when we refactor get/init-pathspec API, we could expose an
interface to turn one element from argv[] into a "struct pathspec_item".
Then we can try to feed argv[i] to that string-to-pathspec_item function,
and consider that argv[i] _is_ a proper pathspec only if it parses
correctly *and* if it matches either an item in the current working tree.

That would be a moral equivalent of the current verify_filename() check
but is far more precise one; e.g. the current code rejects

	git grep -e foo '*.c' ;# bad

because '*.c' is not an object name, but lstat("*.c") fails, and you need
to disambiguate with '--'.  If you rewrite the verify_filename() in the
way I outlined above, you wouldn't have to.

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Nguyen Thai Ngoc Duy]

On Sat, Apr 9, 2011 at 6:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Also make ':' without anything else to mean "there is no pathspec".  This
> would allow differences between "git log" and "git log ." run from the top
> level of the working tree (the latter simplifies no-op commits away from
> the history) to be expressed from a subdirectory by saying "git log :".

The intention is good, but reality may need more work. I assume that
"git add -u :" is equivalent to "git add -u" (or "git add -u ." to be
precise). Unfortunately, cmd_add() checks argc for no arguments to
turn "add -u <nothing>" to "add -u .", not the result from
get_pathspec(). It can be fixed. Just heads up as there can be similar
traps elsewhere.
-- 
Duy

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Nguyen Thai Ngoc Duy]

On Sat, Apr 9, 2011 at 11:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Instead, when we refactor get/init-pathspec API, we could expose an
> interface to turn one element from argv[] into a "struct pathspec_item".
> Then we can try to feed argv[i] to that string-to-pathspec_item function,
> and consider that argv[i] _is_ a proper pathspec only if it parses
> correctly *and* if it matches either an item in the current working tree.
>
> That would be a moral equivalent of the current verify_filename() check
> but is far more precise one; e.g. the current code rejects
>
>        git grep -e foo '*.c' ;# bad
>
> because '*.c' is not an object name, but lstat("*.c") fails, and you need
> to disambiguate with '--'.  If you rewrite the verify_filename() in the
> way I outlined above, you wouldn't have to.

I considered that, but we may need to go through the whole worktree
just to verify "*.c" matches something. The worst case scenario can be
expensive (eg. doing that on a-forest-with-no-c-file gentoo-x86.git).
Alternative approach is recognize "*.c" has wildcard and let it pass
without actually matching.
-- 
Duy

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Junio C Hamano]

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

> The intention is good, but reality may need more work.

Yes, I knew about "add -u", as I was touching in its neighbourhood
recently.  The way "git grep" does this may be more appropriate as a short
term solution.

The updated init_pathspec() should at least take (prefix, argc, argv[]),
and in addition a hint as to what to do when there is no pathspec from the
command line (i.e. argc == 0), so that it can behave differently when the
user gave only ":".

By the way, the field in "struct pathspec_item" would need to be updated,
and the matcher would need to be changed, so that each item knows up to
which part of the "match" string came from the prefix (and remainder is a
user supplied pattern).  Then from a subdirectory a?a/bbb,

  - "c" should parse into prefix "a?a/bbb/" plus pattern "c"

  - ":../c" should become prefix "a?a/" plus pattern "c"

and the matcher should match the prefix part _literally_ without
fnmatch(3), while using whatever magic (e.g. use_wildcard) to match the
pattern part.  I think we currently match the whole thing with fnmatch(3),
which in practice may be OK only because not many people use glob
characters in their directory names, but what the current matcher does
logically is wrong.

Of course, both of the above are tasks after 1.7.5 ships, but I thought I
should mention them now, as you seem to be already thinking about the
future.

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Nguyen Thai Ngoc Duy]

On Sat, Apr 9, 2011 at 12:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> By the way, the field in "struct pathspec_item" would need to be updated,
> and the matcher would need to be changed, so that each item knows up to
> which part of the "match" string came from the prefix (and remainder is a
> user supplied pattern).  Then from a subdirectory a?a/bbb,
>
>  - "c" should parse into prefix "a?a/bbb/" plus pattern "c"
>
>  - ":../c" should become prefix "a?a/" plus pattern "c"
>
> and the matcher should match the prefix part _literally_ without
> fnmatch(3), while using whatever magic (e.g. use_wildcard) to match the
> pattern part.  I think we currently match the whole thing with fnmatch(3),
> which in practice may be OK only because not many people use glob
> characters in their directory names, but what the current matcher does
> logically is wrong.

OK. Let's add nomagic_len (or plain_len) to pathspec_item for that. I
was thinking of noglob_len but changed my mind because the same can
also be applied for icase magic. We don't want to do strcasecmp on
prefix.

> Of course, both of the above are tasks after 1.7.5 ships, but I thought I
> should mention them now, as you seem to be already thinking about the
> future.
-- 
Duy

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Nguyen Thai Ngoc Duy]

On Sat, Apr 9, 2011 at 11:58 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> On Sat, Apr 9, 2011 at 6:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Also make ':' without anything else to mean "there is no pathspec".  This
>> would allow differences between "git log" and "git log ." run from the top
>> level of the working tree (the latter simplifies no-op commits away from
>> the history) to be expressed from a subdirectory by saying "git log :".
>
> The intention is good, but reality may need more work.

Wait, what if I say "git grep -- : foo : bar"? I take it we should
reject on this case?
-- 
Duy

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Junio C Hamano]

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

> On Sat, Apr 9, 2011 at 11:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> ... If you rewrite the verify_filename() in the
>> way I outlined above, you wouldn't have to.
>
> I considered that, but we may need to go through the whole worktree
> just to verify "*.c" matches something.

You are absolutely right.  After all, this is only meant to be a quick
sanity check to ensure that we got the user's intention absolutely right
when the user did not disambiguate with '--'.  When we see all earlier
ones are object names (and cannot be filenames) and all later ones have
working tree entries (and cannot be object names), we are sure that we got
the missing '--' boundary right.

And we error out when there is any doubt, and that is a good feature.
Even when there is no glob involved, e.g. "git log builtin-add.c", we fail
the parsing, because there is no builtin-add.c in the filesystem in
today's checkou.  That behaviour is coming exactly from that reasoning: we
may be sure that "builtin-add.c" cannot be an object name, but we don't
know if it is a typo of builtin/add.c (or builtin-add.o, etc.), and we
don't want to make the user wait while we are guessing.

So let's forget about the suggestion.  Thanks for injecting sanity to the
discussion and stopping me from going overboard.

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Junio C Hamano]

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

> On Sat, Apr 9, 2011 at 11:58 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
>> On Sat, Apr 9, 2011 at 6:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Also make ':' without anything else to mean "there is no pathspec".  This
>>> would allow differences between "git log" and "git log ." run from the top
>>> level of the working tree (the latter simplifies no-op commits away from
>>> the history) to be expressed from a subdirectory by saying "git log :".
>>
>> The intention is good, but reality may need more work.
>
> Wait, what if I say "git grep -- : foo : bar"? I take it we should
> reject on this case?

The patch probably would stuff NULL, "foo", NULL, "bar" in the array, and
the first NULL would make the caller turn the array itself into NULL; it
is a user error I didn't have to bother while the topic is still in a PoC
stage.

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Nguyen Thai Ngoc Duy]

On Sat, Apr 9, 2011 at 6:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Subject: [PATCH] magic pathspec: futureproof shorthand form
>
> ...
>
> Also make ':' without anything else to mean "there is no pathspec".  This
> would allow differences between "git log" and "git log ." run from the top
> level of the working tree (the latter simplifies no-op commits away from
> the history) to be expressed from a subdirectory by saying "git log :".

I need someone to enlighten me again. Why do we need ":" for "no
pathspec" when we can simply specify no pathspec for the same effect?

"git log" and "git log ." at top worktree are not different because
any changes in the tree will make top tree object different, hence no
pruning (unless someone commits the same tree, which is really rare).
So

 - "git log" in subdirectory is exactly the same as "git log" at top.
 - "git log :/" in subdir can do whatever "git log ." at top can.
 - "git log ." in subdir will prune commits that does not change
subdir (current behavior)

I don't (or no longer) see the point of reserving ":" for "no pathspec".
-- 
Duy

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Junio C Hamano]

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

> I need someone to enlighten me again. Why do we need ":" for "no
> pathspec" when we can simply specify no pathspec for the same effect?

Futureproofing.  Currently "log" family defaults to "tree-wide" when there
is no pathspec, but imagine what happens when a command like "log" that
knows how to simplify history defaults to "current directory".  You cannot
override it by "git that-cmd :/", which would give it a single tree-wide
pathspec, not "no pathspec".  It will still cull empty commits.

Re: [PATCH 2/4] add -u: get rid of "treewideupdate" configuration

[Nguyen Thai Ngoc Duy]

On Tue, May 3, 2011 at 10:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> I need someone to enlighten me again. Why do we need ":" for "no
>> pathspec" when we can simply specify no pathspec for the same effect?
>
> Futureproofing.  Currently "log" family defaults to "tree-wide" when there
> is no pathspec, but imagine what happens when a command like "log" that
> knows how to simplify history defaults to "current directory".  You cannot
> override it by "git that-cmd :/", which would give it a single tree-wide
> pathspec, not "no pathspec".  It will still cull empty commits.

OK, empty commits. I see. Will test it that way.
-- 
Duy