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

STALE3735d

7 messages, 3 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:50:59

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:50:59

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 <redacted>
---
 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 3/4] add: make "add -u/-A" update full tree without pathspec (step 2)

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:50:59

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 <redacted>
---
 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)

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:50:59

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 <redacted>
---
 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

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:50:59

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 <redacted>
---
 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

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

From: Nguyen Thai Ngoc Duy <hidden>
Date: 2016-06-15 22:51:00

On Thu, Apr 7, 2011 at 8:16 AM, Junio C Hamano [off-list ref] 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 2/4] add -u: get rid of "treewideupdate" configuration

From: Jeff King <hidden>
Date: 2016-06-15 22:51:00

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help