[PATCH 0/2] "git add -u/-A" from future

[PATCH 0/2] "git add -u/-A" from future

[Junio C Hamano]

Here are two future steps to update the behaviour of "add -u/-A" run
without pathspec towards Git 2.0; the first step may probably be
optional, but it is included for completeness.

Junio C Hamano (2):
  require pathspec for "git add -u/-A"
  git add: -u/-A now affects the entire working tree

 Documentation/git-add.txt |  8 ++++----
 builtin/add.c             | 49 ++++-------------------------------------------
 2 files changed, 8 insertions(+), 49 deletions(-)

-- 
1.8.2-rc3-196-gfcda97c

[PATCH 1/2] require pathspec for "git add -u/-A"

[Junio C Hamano]

As promised in 0fa2eb530fb7 (add: warn when -u or -A is used without
pathspec, 2013-01-28), "git add -u/-A" that is run without pathspec
in a subdirectory will stop working sometime before Git 2.0, to wean
users off of the old default, in preparation for adopting the new
default in Git 2.0.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-add.txt | 12 ++++++++----
 builtin/add.c             | 38 ++++++++++++++++++--------------------
 2 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 388a225..1ea1d39 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -107,10 +107,14 @@ apply to the index. See EDITING PATCHES below.
 	from the index if the corresponding files in the working tree
 	have been removed.
 +
-If no <pathspec> is given, the current version of Git defaults to
-"."; in other words, update all tracked files in the current directory
-and its subdirectories. This default will change in a future version
-of Git, hence the form without <pathspec> should not be used.
+If no <pathspec> is given when `-u` or `-A` option is used, Git used
+to update all tracked files in the current directory and its
+subdirectories. We would eventually want to change this to operate
+on the entire working tree, not limiting it to the current
+directory, to make it consistent with `git commit -a` and other
+commands.  In order to avoid harming users who are used to the old
+default, Git *errors out* when no <pathspec> is given to train their
+fingers to explicitly type `git add -u .` when they mean it.
 
 -A::
 --all::
diff --git a/builtin/add.c b/builtin/add.c
index 0dd014e..4b9d57c 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -321,30 +321,28 @@ static int add_files(struct dir_struct *dir, int flags)
 	return exit_status;
 }
 
-static void warn_pathless_add(const char *option_name, const char *short_name) {
+static void die_on_pathless_add(const char *option_name, const char *short_name)
+{
 	/*
 	 * To be consistent with "git add -p" and most Git
 	 * commands, we should default to being tree-wide, but
 	 * this is not the original behavior and can't be
 	 * changed until users trained themselves not to type
-	 * "git add -u" or "git add -A". For now, we warn and
-	 * keep the old behavior. Later, this warning can be
-	 * turned into a die(...), and eventually we may
-	 * reallow the command with a new behavior.
+	 * "git add -u" or "git add -A". In the previous release,
+	 * we kept the old behavior but gave a big warning.
 	 */
-	warning(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
-		  "subdirectory of the tree will change in Git 2.0 and should not be used anymore.\n"
-		  "To add content for the whole tree, run:\n"
-		  "\n"
-		  "  git add %s :/\n"
-		  "  (or git add %s :/)\n"
-		  "\n"
-		  "To restrict the command to the current directory, run:\n"
-		  "\n"
-		  "  git add %s .\n"
-		  "  (or git add %s .)\n"
-		  "\n"
-		  "With the current Git version, the command is restricted to the current directory."),
+	die(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
+	      "subdirectory of the tree will change in Git 2.0 and should not be "
+	      "used anymore.\n"
+	      "To add content for the whole tree, run:\n"
+	      "\n"
+	      "  git add %s :/\n"
+	      "  (or git add %s :/)\n"
+	      "\n"
+	      "To restrict the command to the current directory, run:\n"
+	      "\n"
+	      "  git add %s .\n"
+	      "  (or git add %s .)"),
 		option_name, short_name,
 		option_name, short_name,
 		option_name, short_name);
@@ -392,8 +390,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (option_with_implicit_dot && !argc) {
 		static const char *here[2] = { ".", NULL };
 		if (prefix)
-			warn_pathless_add(option_with_implicit_dot,
-					  short_option_with_implicit_dot);
+			die_on_pathless_add(option_with_implicit_dot,
+					    short_option_with_implicit_dot);
 		argc = 1;
 		argv = here;
 	}
-- 
1.8.2-rc3-196-gfcda97c

[PATCH 2/2] git add: -u/-A now affects the entire working tree

[Junio C Hamano]

As promised in 0fa2eb530fb7 (add: warn when -u or -A is used without
pathspec, 2013-01-28), in Git 2.0, "git add -u/-A" that is run
without pathspec in a subdirectory updates all updated paths in the
entire working tree, not just the current directory and its
subdirectories.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-add.txt | 12 ++++--------
 builtin/add.c             | 47 ++++-------------------------------------------
 2 files changed, 8 insertions(+), 51 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 1ea1d39..15a8859 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -107,14 +107,10 @@ apply to the index. See EDITING PATCHES below.
 	from the index if the corresponding files in the working tree
 	have been removed.
 +
-If no <pathspec> is given when `-u` or `-A` option is used, Git used
-to update all tracked files in the current directory and its
-subdirectories. We would eventually want to change this to operate
-on the entire working tree, not limiting it to the current
-directory, to make it consistent with `git commit -a` and other
-commands.  In order to avoid harming users who are used to the old
-default, Git *errors out* when no <pathspec> is given to train their
-fingers to explicitly type `git add -u .` when they mean it.
+If no <pathspec> is given when `-u` or `-A` option is used, all
+tracked files in the entire working tree are updated (old versions
+of Git used to limit the update to the current directory and its
+subdirectories).
 
 -A::
 --all::
diff --git a/builtin/add.c b/builtin/add.c
index 4b9d57c..6cd063f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -321,33 +321,6 @@ static int add_files(struct dir_struct *dir, int flags)
 	return exit_status;
 }
 
-static void die_on_pathless_add(const char *option_name, const char *short_name)
-{
-	/*
-	 * To be consistent with "git add -p" and most Git
-	 * commands, we should default to being tree-wide, but
-	 * this is not the original behavior and can't be
-	 * changed until users trained themselves not to type
-	 * "git add -u" or "git add -A". In the previous release,
-	 * we kept the old behavior but gave a big warning.
-	 */
-	die(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
-	      "subdirectory of the tree will change in Git 2.0 and should not be "
-	      "used anymore.\n"
-	      "To add content for the whole tree, run:\n"
-	      "\n"
-	      "  git add %s :/\n"
-	      "  (or git add %s :/)\n"
-	      "\n"
-	      "To restrict the command to the current directory, run:\n"
-	      "\n"
-	      "  git add %s .\n"
-	      "  (or git add %s .)"),
-		option_name, short_name,
-		option_name, short_name,
-		option_name, short_name);
-}
-
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
 	int exit_status = 0;
@@ -358,8 +331,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int add_new_files;
 	int require_pathspec;
 	char *seen = NULL;
-	const char *option_with_implicit_dot = NULL;
-	const char *short_option_with_implicit_dot = NULL;
 
 	git_config(add_config, NULL);
 
@@ -379,21 +350,11 @@ 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) {
-		option_with_implicit_dot = "--all";
-		short_option_with_implicit_dot = "-A";
-	}
-	if (take_worktree_changes) {
-		option_with_implicit_dot = "--update";
-		short_option_with_implicit_dot = "-u";
-	}
-	if (option_with_implicit_dot && !argc) {
-		static const char *here[2] = { ".", NULL };
-		if (prefix)
-			die_on_pathless_add(option_with_implicit_dot,
-					    short_option_with_implicit_dot);
+
+	if ((addremove || take_worktree_changes) && !argc) {
+		static const char *whole[2] = { ":/", NULL };
 		argc = 1;
-		argv = here;
+		argv = whole;
 	}
 
 	add_new_files = !take_worktree_changes && !refresh_only;
-- 
1.8.2-rc3-196-gfcda97c

Re: [PATCH 1/2] require pathspec for "git add -u/-A"

[Matthieu Moy]

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

> As promised in 0fa2eb530fb7 (add: warn when -u or -A is used without
> pathspec, 2013-01-28), "git add -u/-A" that is run without pathspec
> in a subdirectory will stop working sometime before Git 2.0, to wean
> users off of the old default, in preparation for adopting the new
> default in Git 2.0.

I originally thought this step was necessary, but I changed my mind. The
warning is big enough and doesn't need to be turned into an error.

If this step is applied, it should be applied at 2.0, not before, as
this is the big incompatible change. Re-introducing a new behavior won't
harm users OTOH.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 1/2] require pathspec for "git add -u/-A"

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> As promised in 0fa2eb530fb7 (add: warn when -u or -A is used without
>> pathspec, 2013-01-28), "git add -u/-A" that is run without pathspec
>> in a subdirectory will stop working sometime before Git 2.0, to wean
>> users off of the old default, in preparation for adopting the new
>> default in Git 2.0.
>
> I originally thought this step was necessary, but I changed my mind. The
> warning is big enough and doesn't need to be turned into an error.

I tend to agree.

The plan requires the warning to be big enough and warning period to
be long enough so that by the time Git 2.0 is released, no existing
users will run "git add -u/-A" without pathspec expecting it to
limit the operation to the current directory, so an extra step to
error out such a command invocation is simply redundant.  If it is
not redundant, that would only mean that the warning period was not
long enough.  The only effect the extra "error it out" step would
have is to hurt the people who jump on Git bandwagon after such
release ships, as they do not have any reason to retrain their
fingers---they instead can just get used to the new behaviour right
away.

So let's squash these two steps into one and keep that in 'next'
until 2.0 ships.

Thanks for an injection of sanity.

Re: [PATCH 1/2] require pathspec for "git add -u/-A"

[Matthieu Moy]

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

> So let's squash these two steps into one and keep that in 'next'
> until 2.0 ships.

OK, then we may update the comment describing the plan (for people
digging in the code to find out what the plan is). Small patch serie
follows with this (will probably give conflict with your patch, feel
free to drop if resolving them is too painful given the benefit) and
another minor improvement.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH 1/2] add: update pathless 'add [-u|-A]' warning to reflect change of plan

[Matthieu Moy]

We originally thought the transition would need a period where "git add
[-u|-A]" without pathspec would be forbidden, but the warning is big
enough to scare people and teach them not to use it (or, if so, to
understand the consequences).

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 builtin/add.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 0dd014e..ab1c9e8 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -328,9 +328,9 @@ static void warn_pathless_add(const char *option_name, const char *short_name) {
 	 * this is not the original behavior and can't be
 	 * changed until users trained themselves not to type
 	 * "git add -u" or "git add -A". For now, we warn and
-	 * keep the old behavior. Later, this warning can be
-	 * turned into a die(...), and eventually we may
-	 * reallow the command with a new behavior.
+	 * keep the old behavior. Later, the behavior can be changed
+	 * to tree-wide, keeping the warning for a while, and
+	 * eventually we can drop the warning.
 	 */
 	warning(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
 		  "subdirectory of the tree will change in Git 2.0 and should not be used anymore.\n"
-- 
1.8.2.rc3.16.g0a33571.dirty

[PATCH 2/2] add: add a newline at the end of pathless 'add [-u|-A]' warning

[Matthieu Moy]

When the commands give an actual output (e.g. when ran with -v), the
output is visually mixed with the warning. The newline makes the actual
output more visible.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index ab1c9e8..620bf00 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -344,7 +344,7 @@ static void warn_pathless_add(const char *option_name, const char *short_name) {
 		  "  git add %s .\n"
 		  "  (or git add %s .)\n"
 		  "\n"
-		  "With the current Git version, the command is restricted to the current directory."),
+		  "With the current Git version, the command is restricted to the current directory.\n"),
 		option_name, short_name,
 		option_name, short_name,
 		option_name, short_name);
-- 
1.8.2.rc3.16.g0a33571.dirty

Re: [PATCH 2/2] add: add a newline at the end of pathless 'add [-u|-A]' warning

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> When the commands give an actual output (e.g. when ran with -v), the
> output is visually mixed with the warning. The newline makes the actual
> output more visible.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---

It would have been easier to immediately understand what is going on
if you said "blank line" instead of "newline" ;-)

An obvious issues is what if user does not run with "-v" or if "-v"
produces no results.  We will be left with an extra blank line at
the end.

I suspect that the true reason why the warning does not stand out
and other output looks mixed in it may be because we only prefix the
first line with the "warning: " label.  In the longer term, I have a
feeling that we should be showing something like this instead:

    $ cd t && echo >>t0000*.sh && git add -u -v
    warning: The behavior of 'git add --update (or -u)' with no path ar...
    warning: subdirectory of the tree will change in Git 2.0 and should...
    warning: To add content for the whole tree, run:
    warning:
    warning:  git add --update :/
    warning:   (or git add -u :/)
    warning:
    warning: To restrict the command to the current directory, run:
    warning:
    warning:   git add --update .
    warning:   (or git add -u .)
    warning:
    warning: With the current Git version, the command is restricted to...
    add 't/t0000-basic.sh'

using a logic similar to what strbuf_add_commented_lines() and
strbuf_add_lines() use.

>  builtin/add.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index ab1c9e8..620bf00 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -344,7 +344,7 @@ static void warn_pathless_add(const char *option_name, const char *short_name) {
>  		  "  git add %s .\n"
>  		  "  (or git add %s .)\n"
>  		  "\n"
> -		  "With the current Git version, the command is restricted to the current directory."),
> +		  "With the current Git version, the command is restricted to the current directory.\n"),
>  		option_name, short_name,
>  		option_name, short_name,
>  		option_name, short_name);

Re: [PATCH 1/2] require pathspec for "git add -u/-A"

[Jeff King]

On Sun, Mar 10, 2013 at 04:49:09PM +0100, Matthieu Moy wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > As promised in 0fa2eb530fb7 (add: warn when -u or -A is used without
> > pathspec, 2013-01-28), "git add -u/-A" that is run without pathspec
> > in a subdirectory will stop working sometime before Git 2.0, to wean
> > users off of the old default, in preparation for adopting the new
> > default in Git 2.0.
> 
> I originally thought this step was necessary, but I changed my mind. The
> warning is big enough and doesn't need to be turned into an error.
> 
> If this step is applied, it should be applied at 2.0, not before, as
> this is the big incompatible change. Re-introducing a new behavior won't
> harm users OTOH.

FWIW, I agree with this. The warning is enough without an error period
(unless the intent was to switch to the error and stay there forever,
but I do not think that is the proposal).

-Peff

PS I wonder how others are finding the warning? I'm finding it slightly
   annoying. Perhaps I just haven't retrained my fingers yet. But one
   problem with that retraining is that I type "git add -u" from the
   root _most_ of the time, and it just works. But occasionally, I get
   the "hey, do not do that" warning, because I'm in a subdir, even
   though I would be happy to have the full-tree behavior. I guess we
   already rejected the idea of being able to shut off the warning and
   just get the new behavior, in favor of having people specify it
   manually each time?

Re: [PATCH 1/2] require pathspec for "git add -u/-A"

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> PS I wonder how others are finding the warning? I'm finding it slightly
>    annoying. Perhaps I just haven't retrained my fingers yet. But one
>    problem with that retraining is that I type "git add -u" from the
>    root _most_ of the time, and it just works. But occasionally, I get
>    the "hey, do not do that" warning, because I'm in a subdir, even
>    though I would be happy to have the full-tree behavior.

Same here. Not terribly disturbing, but I did get the warning
occasionally, even though I'm the author of the patch introducing
it ;-).

> I guess we already rejected the idea of being able to shut off the
> warning and just get the new behavior, in favor of having people
> specify it manually each time?

Somehow, but we may find a way to do so, as long as it temporary (i.e.
something that will have no effect after the transition period), and
that is is crystal clear that it's temporary.

All proposals up to now were rejected because of the risk of confusing
users (either shutting the warning blindly, or letting people think they
could keep the current behavior after Git 2.0).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 1/2] require pathspec for "git add -u/-A"

[Jeff King]

On Tue, Mar 12, 2013 at 02:58:44PM +0100, Matthieu Moy wrote:

> > I guess we already rejected the idea of being able to shut off the
> > warning and just get the new behavior, in favor of having people
> > specify it manually each time?
> 
> Somehow, but we may find a way to do so, as long as it temporary (i.e.
> something that will have no effect after the transition period), and
> that is is crystal clear that it's temporary.

Yeah, I think this is easy as long as it is "enable the new behavior
now" and not "toggle between new and old behavior". That is, a boolean
rather than a selector, with a note that it will go away at the behavior
switch.

The only downside I see is that a user may switch it on now, saying
"Yes, I understand and prefer the new behavior", but some script they
run might not expect it. We can warn against that in the documentation,
but that may or may not be enough.

Here's a series which does that; if it's the direction we want to go, I
think we'd want to rebase Junio's "now add -u is full-tree" patch on
top.

  [1/2]: t2200: check that "add -u" limits itself to subdirectory
  [2/2]: add: respect add.updateroot config option

-Peff

[PATCH 1/2] t2200: check that "add -u" limits itself to subdirectory

[Jeff King]

This behavior is due to change in the future, but let's test
it anyway. That helps make sure we do not accidentally
switch the behavior too soon while we are working in the
area, and it means that we can easily verify the change when
we do make it.

Signed-off-by: Jeff King <peff@peff.net>
---
We didn't seem to be testing this transition at all. I think it's sane
to do so now, and Junio's "now it is 2.0, let's switch" patch should
update the test.

 t/t2200-add-update.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index 4cdebda..fe4f548 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -80,6 +80,17 @@ test_expect_success 'change gets noticed' '
 
 '
 
+# Note that this is scheduled to change in Git 2.0, when
+# "git add -u" will become full-tree by default.
+test_expect_success 'update did not touch files at root' '
+	cat >expect <<-\EOF &&
+	check
+	top
+	EOF
+	git diff-files --name-only >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success SYMLINKS 'replace a file with a symlink' '
 
 	rm foo &&
-- 
1.8.2.rc2.7.gef06216

[PATCH 2/2] add: respect add.updateroot config option

[Jeff King]

The `git add -u` command operates on the current subdir of
the repository, but is transitioning to operate on the whole
repository (see commit 0fa2eb5 for details). During the
transition period, we issue a warning. For users who have
read and accepted the warning, there is no way to jump
directly to the future behavior and silence the warning. The
config option added by this patch gives them such an option.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/config.txt | 19 +++++++++++++++++++
 builtin/add.c            | 10 ++++++++--
 t/t2200-add-update.sh    | 14 ++++++++++++++
 3 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bbba728..be0f6fc 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -657,6 +657,25 @@ add.ignoreErrors::
 	convention for configuration variables.  Newer versions of Git
 	honor `add.ignoreErrors` as well.
 
+add.updateroot::
+	Historically, when the `git add -u` and `git add -A` commands
+	are run from a subdirectory of the repository and are not given
+	any pathspec, they have operated only on the subdirectory from
+	which they were called. In a future version of git, this behavior
+	will be switched to operate on the full repository instead. In
+	the meantime, issuing `git add -u` (or `-A`) from a subdirectory
+	continues to work on the subdirectory, but will now issue a
+	warning. If you are ready to move to the new behavior now,
+	accepting that you may be breaking existing scripts which expect
+	the old behavior, you can do so by setting `add.updateroot` to
+	`true`.
++
+Note that this is meant to let early adopters move to the new behavior
+immediately; it is not a toggle switch that will work forever.  After
+git transitions to the new behavior, this option will become a no-op;
+`git add -u` will always update the whole tree, and `git add -u .` will
+remain the correct way to limit to the current directory.
+
 alias.*::
 	Command aliases for the linkgit:git[1] command wrapper - e.g.
 	after defining "alias.last = cat-file commit HEAD", the invocation
diff --git a/builtin/add.c b/builtin/add.c
index ab1c9e8..95fe35e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -22,6 +22,7 @@ static int take_worktree_changes;
 };
 static int patch_interactive, add_interactive, edit_interactive;
 static int take_worktree_changes;
+static int update_root;
 
 struct update_callback_data {
 	int flags;
@@ -297,6 +298,10 @@ static int add_config(const char *var, const char *value, void *cb)
 		ignore_add_errors = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "add.updateroot")) {
+		update_root = git_config_bool(var, value);
+		return 0;
+	}
 	return git_default_config(var, value, cb);
 }
 
@@ -390,12 +395,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		short_option_with_implicit_dot = "-u";
 	}
 	if (option_with_implicit_dot && !argc) {
+		static const char *root[2] = { ":/", NULL };
 		static const char *here[2] = { ".", NULL };
-		if (prefix)
+		if (!update_root && prefix)
 			warn_pathless_add(option_with_implicit_dot,
 					  short_option_with_implicit_dot);
 		argc = 1;
-		argv = here;
+		argv = update_root ? root : here;
 	}
 
 	add_new_files = !take_worktree_changes && !refresh_only;
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index fe4f548..b9215d3 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -91,6 +91,20 @@ test_expect_success 'update did not touch files at root' '
 	test_cmp expect actual
 '
 
+test_expect_success 'update with add.updateroot set' '
+	(
+		cd dir1 &&
+		echo more >>sub2 &&
+		git -c add.updateroot=true add -u
+	)
+'
+
+test_expect_success 'all paths are updated' '
+	>expect &&
+	git diff-files --name-status >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success SYMLINKS 'replace a file with a symlink' '
 
 	rm foo &&
-- 
1.8.2.rc2.7.gef06216

Re: [PATCH 1/2] t2200: check that "add -u" limits itself to subdirectory

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> This behavior is due to change in the future, but let's test
> it anyway.

Thanks. This should be merged regardless of PATCH 2/2 I think.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 2/2] add: respect add.updateroot config option

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> For users who have read and accepted the warning, there is no way to
> jump directly to the future behavior and silence the warning.

I think the idea makes sense. The transition period is necessary for
people who use different versions of Git (which includes anybody
writting and distributing scripts), but for poor mortals who only use a
single version of Git, it's nice to be able to jump to the future
behavior once for all as soon as possible.

Your patch doesn't advertise the option in the warning message, which I
think is good. You may mention it the commit message that this is a
deliberate choice.

> +add.updateroot::

Detail: option names are normally camelCased => updateRoot.

I think the option name needs a bit more thinking. Without reading the
doc,

[add]
	updateRoot = false

would be a very tempting thing to try. Perhaps explicitly warning when
add.updateroot is set to false would avoid possible confusion.

In case you had missed it, here's a previous piece of discussion on the
subject:

http://thread.gmane.org/gmane.comp.version-control.git/216818/focus=216846

I liked David's suggestion of using future.* to mean "start using
behavior from the future right now".

> +	which they were called. In a future version of git, this behavior

Capital "Git".

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 2/2] add: respect add.updateroot config option

[Jeff King]

On Wed, Mar 13, 2013 at 10:07:34AM +0100, Matthieu Moy wrote:

> Jeff King <peff@peff.net> writes:
> 
> > For users who have read and accepted the warning, there is no way to
> > jump directly to the future behavior and silence the warning.
> 
> I think the idea makes sense. The transition period is necessary for
> people who use different versions of Git (which includes anybody
> writting and distributing scripts), but for poor mortals who only use a
> single version of Git, it's nice to be able to jump to the future
> behavior once for all as soon as possible.

I think the biggest risk is from people who think they are safe to jump,
and then find out that some script they depend on is not ready. Even if
they do not even realize they are relying on it. Part of the point of
the transition period is to get script authors to update their scripts,
and to let the new versions trickle down to the users.

> Your patch doesn't advertise the option in the warning message, which I
> think is good. You may mention it the commit message that this is a
> deliberate choice.

Yes, it was deliberate. I can add a note.

> > +add.updateroot::
> 
> Detail: option names are normally camelCased => updateRoot.

Good point, thanks.

> I think the option name needs a bit more thinking. Without reading the
> doc,
> 
> [add]
> 	updateRoot = false
> 
> would be a very tempting thing to try. Perhaps explicitly warning when
> add.updateroot is set to false would avoid possible confusion.

Yeah, that occurred to me, too, hence the note in the doc. Since it
isn't advertised elsewhere, I had hoped that anybody who discovered it
would see the note. I suppose we can warn when we see add.updateRoot set
to anything but true. That feels a bit hacky, as it's possible the user
could be overriding an earlier setting (although that is getting kind of
crazy).

> I liked David's suggestion of using future.* to mean "start using
> behavior from the future right now".

I do like that idea, as it makes the meaning of the variable more clear.

I dunno. I am not all that excited about this patch, due to all of the
caveats that we need to communicate to the user. The current warning has
annoyed me a few times, but perhaps it is still too soon, and my fingers
and brain just need retraining. I think a config option could help some
people, but maybe it will end up hurting more.  I'm inclined at this
point to table the patch for now and see how I feel in a few weeks.

I do think patch 1/2 makes sense regardless.

-Peff

Re: [PATCH 2/2] add: respect add.updateroot config option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Your patch doesn't advertise the option in the warning message, which I
>> think is good. You may mention it the commit message that this is a
>> deliberate choice.
>
> Yes, it was deliberate. I can add a note.
>
>> > +add.updateroot::
>> 
>> Detail: option names are normally camelCased => updateRoot.
>
> Good point, thanks.
>
>> I think the option name needs a bit more thinking. Without reading the
>> doc,
>> 
>> [add]
>> 	updateRoot = false
>> 
>> would be a very tempting thing to try. Perhaps explicitly warning when
>> add.updateroot is set to false would avoid possible confusion.

This is essentially the same as Matthieu's add.use2dot0Behavior but
with that "it is an error to explicitly set it to false" twist, it
alleviates one of the worries. It still has the same "the user saw
it mentioned on stackoverflow and sets it without understanding that
the behaviour is getting changed" effect.

Also it won't give chance for scripts to get fixed, even though I
suspect that instances of "add -u/-A" without pathspec end users
write in their custom scripts almost always would want to be
tree-wide and it is a bug that they do not pass ":/" to them.

The "future.*" (I'd rather call that "migration.*") approach with
the "never set it to false" twist is probably OK from the "complaint
avoidance" perspective.  The user cannot later complain "I tried to
squelch the advice but at the same time I ended up adding updated
contents outside my diretory", without getting told "That is the
documented behaviour, isn't it?"  But I still think it is much less
nice from "avoid hurting the users" perspective.  If the way to
squelch the user learns were to say "git add -u .", where the user
explicitly says "take the updated contents from this directory and
below", there is no room for confusion.  We won't hear complaints
either way, but with the "future.*" the reason why we won't hear
complaints is because the users do not have excuse to complain.
With the "require explicit .", it is because the users won't get
hurt in the first place.

> I dunno. I am not all that excited about this patch, due to all of the
> caveats that we need to communicate to the user. The current warning has
> annoyed me a few times, but perhaps it is still too soon, and my fingers
> and brain just need retraining. I think a config option could help some
> people, but maybe it will end up hurting more.  I'm inclined at this
> point to table the patch for now and see how I feel in a few weeks.
>
> I do think patch 1/2 makes sense regardless.

These two concluding paragraphs match my current thinking.

Thanks.

Re: [PATCH 1/2] t2200: check that "add -u" limits itself to subdirectory

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We didn't seem to be testing this transition at all. I think it's sane
> to do so now, and Junio's "now it is 2.0, let's switch" patch should
> update the test.

Yes, but I am not sure if this is testing the right thing.

> +# Note that this is scheduled to change in Git 2.0, when
> +# "git add -u" will become full-tree by default.
> +test_expect_success 'update did not touch files at root' '
> +	cat >expect <<-\EOF &&
> +	check
> +	top
> +	EOF
> +	git diff-files --name-only >actual &&
> +	test_cmp expect actual
> +'

The last "git add -u" we have beforet his block is this test piece:

 test_expect_success 'update from a subdirectory' '
        (
                cd dir1 &&
                echo more >sub2 &&
                git add -u sub2
        )
 '

That is not "git add -u" without pathspec, which is the only thing
we are transitioning at Git 2.0 boundary.

Re: [PATCH 1/2] t2200: check that "add -u" limits itself to subdirectory

[Jeff King]

On Wed, Mar 13, 2013 at 10:44:25AM -0700, Junio C Hamano wrote:

> > +# Note that this is scheduled to change in Git 2.0, when
> > +# "git add -u" will become full-tree by default.
> > +test_expect_success 'update did not touch files at root' '
> > +	cat >expect <<-\EOF &&
> > +	check
> > +	top
> > +	EOF
> > +	git diff-files --name-only >actual &&
> > +	test_cmp expect actual
> > +'
> 
> The last "git add -u" we have beforet his block is this test piece:
> 
>  test_expect_success 'update from a subdirectory' '
>         (
>                 cd dir1 &&
>                 echo more >sub2 &&
>                 git add -u sub2
>         )
>  '
> 
> That is not "git add -u" without pathspec, which is the only thing
> we are transitioning at Git 2.0 boundary.

Oops, you're right. I just saw the "cd" and totally missed the pathspec.
The correct test should be:

-- >8 --
Subject: [PATCH] t2200: check that "add -u" limits itself to subdirectory

This behavior is due to change in the future, but let's test
it anyway. That helps make sure we do not accidentally
switch the behavior too soon while we are working in the
area, and it means that we can easily verify the change when
we do make it.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t2200-add-update.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index 4cdebda..c317254 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -80,6 +80,22 @@ test_expect_success 'change gets noticed' '
 
 '
 
+# Note that this is scheduled to change in Git 2.0, when
+# "git add -u" will become full-tree by default.
+test_expect_success 'non-limited update in subdir leaves root alone' '
+	(
+		cd dir1 &&
+		echo even more >>sub2 &&
+		git add -u
+	) &&
+	cat >expect <<-\EOF &&
+	check
+	top
+	EOF
+	git diff-files --name-only >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success SYMLINKS 'replace a file with a symlink' '
 
 	rm foo &&
-- 
1.8.2.rc2.7.gef06216

Re: [PATCH 2/2] add: respect add.updateroot config option

[Matthieu Moy]

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

> It still has the same "the user saw
> it mentioned on stackoverflow and sets it without understanding that
> the behaviour is getting changed" effect.

I'm not so worried about this point, as the mechanism is temporary, and
it will take time before answers on stackoverflow reach the mass. The
initial state is that the option is not discoverable without reading the
documentation, and Jeff's documentation is very clear that this is
temporary. People reading the doc are usually not the kind of people
giving dumb answers on the web.

And even if some people set the option without understanding the
consequences, it's not that terrible. They'll just get the transition
period shortened.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH 0/4] make pathless 'add [-u|-A]' warning less noisy

[Jonathan Nieder]

Hi,

Jeff King wrote:

>                                                          The
> config option added by this patch gives them such an option.

I suspect the need for this config option is a sign that the warning
is too eager.  After all, the whole idea of the change being safe is
that it shouldn't make a difference the way people usually use git,
no?

In other words, how about the following patches?  With them applied,
hopefully no one would mind even if the warning becomes a fatal error.

Looking forward to your thoughts,

Jonathan Nieder (4):
  add: make pathless 'add [-u|-A]' warning a file-global function
  add: make warn_pathless_add() a no-op after first call
  add -u: only show pathless 'add -u' warning when changes exist outside cwd
  add -A: only show pathless 'add -A' warning when changes exist outside cwd

 builtin/add.c | 142 ++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 99 insertions(+), 43 deletions(-)

[PATCH 1/4] add: make pathless 'add [-u|-A]' warning a file-global function

[Jonathan Nieder]

Currently warn_pathless_add() is only called directly by cmd_add(),
but that is about to change.  Move its definition higher in the file
and pass the "--update" or "--all" option name used in its message
through globals instead of function arguments to make it easier to
call without passing values that will not change through the call
chain.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/add.c | 69 +++++++++++++++++++++++++++++++----------------------------
 1 file changed, 36 insertions(+), 33 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index ab1c9e8f..a4028eea 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -28,6 +28,41 @@ struct update_callback_data {
 	int add_errors;
 };
 
+static const char *option_with_implicit_dot;
+static const char *short_option_with_implicit_dot;
+
+static void warn_pathless_add(void)
+{
+	assert(option_with_implicit_dot && short_option_with_implicit_dot);
+
+	/*
+	 * To be consistent with "git add -p" and most Git
+	 * commands, we should default to being tree-wide, but
+	 * this is not the original behavior and can't be
+	 * changed until users trained themselves not to type
+	 * "git add -u" or "git add -A". For now, we warn and
+	 * keep the old behavior. Later, the behavior can be changed
+	 * to tree-wide, keeping the warning for a while, and
+	 * eventually we can drop the warning.
+	 */
+	warning(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
+		  "subdirectory of the tree will change in Git 2.0 and should not be used anymore.\n"
+		  "To add content for the whole tree, run:\n"
+		  "\n"
+		  "  git add %s :/\n"
+		  "  (or git add %s :/)\n"
+		  "\n"
+		  "To restrict the command to the current directory, run:\n"
+		  "\n"
+		  "  git add %s .\n"
+		  "  (or git add %s .)\n"
+		  "\n"
+		  "With the current Git version, the command is restricted to the current directory."),
+		option_with_implicit_dot, short_option_with_implicit_dot,
+		option_with_implicit_dot, short_option_with_implicit_dot,
+		option_with_implicit_dot, short_option_with_implicit_dot);
+}
+
 static int fix_unmerged_status(struct diff_filepair *p,
 			       struct update_callback_data *data)
 {
@@ -321,35 +356,6 @@ static int add_files(struct dir_struct *dir, int flags)
 	return exit_status;
 }
 
-static void warn_pathless_add(const char *option_name, const char *short_name) {
-	/*
-	 * To be consistent with "git add -p" and most Git
-	 * commands, we should default to being tree-wide, but
-	 * this is not the original behavior and can't be
-	 * changed until users trained themselves not to type
-	 * "git add -u" or "git add -A". For now, we warn and
-	 * keep the old behavior. Later, the behavior can be changed
-	 * to tree-wide, keeping the warning for a while, and
-	 * eventually we can drop the warning.
-	 */
-	warning(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
-		  "subdirectory of the tree will change in Git 2.0 and should not be used anymore.\n"
-		  "To add content for the whole tree, run:\n"
-		  "\n"
-		  "  git add %s :/\n"
-		  "  (or git add %s :/)\n"
-		  "\n"
-		  "To restrict the command to the current directory, run:\n"
-		  "\n"
-		  "  git add %s .\n"
-		  "  (or git add %s .)\n"
-		  "\n"
-		  "With the current Git version, the command is restricted to the current directory."),
-		option_name, short_name,
-		option_name, short_name,
-		option_name, short_name);
-}
-
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
 	int exit_status = 0;
@@ -360,8 +366,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int add_new_files;
 	int require_pathspec;
 	char *seen = NULL;
-	const char *option_with_implicit_dot = NULL;
-	const char *short_option_with_implicit_dot = NULL;
 
 	git_config(add_config, NULL);
 
@@ -392,8 +396,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (option_with_implicit_dot && !argc) {
 		static const char *here[2] = { ".", NULL };
 		if (prefix)
-			warn_pathless_add(option_with_implicit_dot,
-					  short_option_with_implicit_dot);
+			warn_pathless_add();
 		argc = 1;
 		argv = here;
 	}
-- 
1.8.2.rc3

[PATCH 2/4] add: make warn_pathless_add() a no-op after first call

[Jonathan Nieder]

Make warn_pathless_add() print its warning the first time it is called
and do nothing if called again.  This will make it easier to show the
warning on the fly when a relevant condition is detected without
risking showing it multiple times when multiple such conditions hold.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/add.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/add.c b/builtin/add.c
index a4028eea..a424e69d 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -33,8 +33,13 @@ static const char *short_option_with_implicit_dot;
 
 static void warn_pathless_add(void)
 {
+	static int shown;
 	assert(option_with_implicit_dot && short_option_with_implicit_dot);
 
+	if (shown)
+		return;
+	shown = 1;
+
 	/*
 	 * To be consistent with "git add -p" and most Git
 	 * commands, we should default to being tree-wide, but
-- 
1.8.2.rc3

[PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Jonathan Nieder]

A common workflow in large projects is to chdir into a subdirectory of
interest and only do work there:

	cd src
	vi foo.c
	make test
	git add -u
	git commit

The upcoming change to 'git add -u' behavior would not affect such a
workflow: when the only changes present are in the current directory,
'git add -u' will add all changes, and whether that happens via an
implicit "." or implicit ":/" parameter is an unimportant
implementation detail.

The warning about use of 'git add -u' with no pathspec is annoying
because it serves no purpose in this case.  So suppress the warning
unless there are changes outside the cwd that are not being added.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/add.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index a424e69d..f05ec1c1 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -89,6 +89,22 @@ static int fix_unmerged_status(struct diff_filepair *p,
 		return DIFF_STATUS_MODIFIED;
 }
 
+static void warn_if_outside_pathspec(struct diff_queue_struct *q,
+				     struct diff_options *opt, void *cbdata)
+{
+	int i;
+	const char **pathspec = cbdata;
+
+	for (i = 0; i < q->nr; i++) {
+		const char *path = q->queue[i]->one->path;
+
+		if (!match_pathspec(pathspec, path, strlen(path), 0, NULL)) {
+			warn_pathless_add();
+			return;
+		}
+	}
+}
+
 static void update_callback(struct diff_queue_struct *q,
 			    struct diff_options *opt, void *cbdata)
 {
@@ -121,20 +137,26 @@ static void update_callback(struct diff_queue_struct *q,
 	}
 }
 
-int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
+static void diff_files_with_callback(const char *prefix, const char **pathspec,
+				     diff_format_fn_t callback, void *data)
 {
-	struct update_callback_data data;
 	struct rev_info rev;
 	init_revisions(&rev, prefix);
 	setup_revisions(0, NULL, &rev, NULL);
 	init_pathspec(&rev.prune_data, pathspec);
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
-	rev.diffopt.format_callback = update_callback;
-	data.flags = flags;
-	data.add_errors = 0;
-	rev.diffopt.format_callback_data = &data;
+	rev.diffopt.format_callback = callback;
+	rev.diffopt.format_callback_data = data;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
+}
+
+int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
+{
+	struct update_callback_data data;
+	data.flags = flags;
+	data.add_errors = 0;
+	diff_files_with_callback(prefix, pathspec, update_callback, &data);
 	return !!data.add_errors;
 }
 
@@ -371,6 +393,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int add_new_files;
 	int require_pathspec;
 	char *seen = NULL;
+	int implicit_dot = 0;
 
 	git_config(add_config, NULL);
 
@@ -400,10 +423,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	}
 	if (option_with_implicit_dot && !argc) {
 		static const char *here[2] = { ".", NULL };
-		if (prefix)
+		if (prefix && addremove)
 			warn_pathless_add();
 		argc = 1;
 		argv = here;
+		implicit_dot = 1;
 	}
 
 	add_new_files = !take_worktree_changes && !refresh_only;
@@ -450,6 +474,19 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		goto finish;
 	}
 
+	/*
+	 * Check if "git add -A" or "git add -u" was run from a
+	 * subdirectory with a modified file outside that directory,
+	 * and warn if so.
+	 *
+	 * "git add -u" will behave like "git add -u :/" instead of
+	 * "git add -u ." in the future.  This warning prepares for
+	 * that change.
+	 */
+	if (implicit_dot)
+		diff_files_with_callback(prefix, NULL,
+				warn_if_outside_pathspec, pathspec);
+
 	if (pathspec) {
 		int i;
 		struct path_exclude_check check;
-- 
1.8.2.rc3

[PATCH 4/4] add -A: only show pathless 'add -A' warning when changes exist outside cwd

[Jonathan Nieder]

In the spirit of the recent similar change for 'git add -u', avoid
pestering users that restrict their attention to a subdirectory and
will not be affected by the coming change in the behavior of pathless
'git add -A'.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/add.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index f05ec1c1..56ac4519 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -160,7 +160,9 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
 	return !!data.add_errors;
 }
 
-static char *prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
+#define WARN_IMPLICIT_DOT (1u << 0)
+static char *prune_directory(struct dir_struct *dir, const char **pathspec,
+			     int prefix, unsigned flag)
 {
 	char *seen;
 	int i, specs;
@@ -177,6 +179,16 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec, int
 		if (match_pathspec(pathspec, entry->name, entry->len,
 				   prefix, seen))
 			*dst++ = entry;
+		else if (flag & WARN_IMPLICIT_DOT)
+			/*
+			 * "git add -A" was run from a subdirectory with a
+			 * new file outside that directory.
+			 *
+			 * "git add -A" will behave like "git add -A :/"
+			 * instead of "git add -A ." in the future.
+			 * Warn about the coming behavior change.
+			 */
+			warn_pathless_add();
 	}
 	dir->nr = dst - dir->entries;
 	add_pathspec_matches_against_index(pathspec, seen, specs);
@@ -423,8 +435,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	}
 	if (option_with_implicit_dot && !argc) {
 		static const char *here[2] = { ".", NULL };
-		if (prefix && addremove)
-			warn_pathless_add();
 		argc = 1;
 		argv = here;
 		implicit_dot = 1;
@@ -464,9 +474,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		}
 
 		/* This picks up the paths that are not tracked */
-		baselen = fill_directory(&dir, pathspec);
+		baselen = fill_directory(&dir, implicit_dot ? NULL : pathspec);
 		if (pathspec)
-			seen = prune_directory(&dir, pathspec, baselen);
+			seen = prune_directory(&dir, pathspec, baselen,
+					implicit_dot ? WARN_IMPLICIT_DOT : 0);
 	}
 
 	if (refresh_only) {
-- 
1.8.2.rc3

Re: [PATCH 0/4] make pathless 'add [-u|-A]' warning less noisy

[Jeff King]

On Mon, Mar 18, 2013 at 08:44:15PM -0700, Jonathan Nieder wrote:

> >                                                          The
> > config option added by this patch gives them such an option.
> 
> I suspect the need for this config option is a sign that the warning
> is too eager.  After all, the whole idea of the change being safe is
> that it shouldn't make a difference the way people usually use git,
> no?
> 
> In other words, how about the following patches?  With them applied,
> hopefully no one would mind even if the warning becomes a fatal error.

Clever. I think it would help in my case. I sometimes follow the
workflow you describe in patch 3 (i.e., just working in a subdir), and
sometimes do something more like:

  $ vi foo.c
  $ cd t
  $ vi tXXXX-foo.sh
  $ ./tXXXX-foo.sh
  $ git add -u

With your patches, we would continue to warn about the second case, but
I think that is a good thing; git is not doing what I want. But by
reducing the false positives from the first case, I would start to
actually pay attention to the warning more.

> Jonathan Nieder (4):
>   add: make pathless 'add [-u|-A]' warning a file-global function
>   add: make warn_pathless_add() a no-op after first call
>   add -u: only show pathless 'add -u' warning when changes exist outside cwd
>   add -A: only show pathless 'add -A' warning when changes exist outside cwd

I don't see anything obviously wrong with the patches themselves. I
wonder if we would want to change the warning to be more explicit that
yes, there really were files that were impacted by this. And possibly
even list them.

I suspect I would not even mind that becoming the final behavior.  I.e.,
going to:

  $ cd subdir && git add -u
  warning: Using 'git add -u' without a pathspec operates only on the
  current subdirectory. Updates from the following files were NOT
  staged:

    file1
    file2
    other-subdir/file3

now, and then eventually converting the warning into a fatal error (and
demanding that the user use ":/" or "." as appropriate).

But in the long run, I guess defaulting to ":/" will be more convenient,
so there is no point in complaining about the ambiguity forever. And in
that case, since the warning is just a placeholder, I don't know that
it's worth much effort to make it fancier.

-Peff

Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> A common workflow in large projects is to chdir into a subdirectory of
> interest and only do work there:
>
> 	cd src
> 	vi foo.c
> 	make test
> 	git add -u
> 	git commit
>
> The upcoming change to 'git add -u' behavior would not affect such a
> workflow: when the only changes present are in the current directory,
> 'git add -u' will add all changes, and whether that happens via an
> implicit "." or implicit ":/" parameter is an unimportant
> implementation detail.
>
> The warning about use of 'git add -u' with no pathspec is annoying
> because it serves no purpose in this case.  So suppress the warning
> unless there are changes outside the cwd that are not being added.

That is a logical extension of the reason why we do not emit
warnings when run at the top level.  A user who has known about and
is very much accustomed to the "current directory only" behaviour
may run "git add -u/-A" always from the top in the current project
she happens to be working on until Git 2.0 happens, and will not get
any warnings.  We are already robbing the chance to learn about and
prepare for the upcoming change from her.  And this patch makes it
even more so.  It does not make things fundamentally worse, but it
makes it more likely to hurt such a user.

The implemenation appears to run an extra diff_files() in addition
to the one we already have to run in order to implement the "add -u"
to collect modified/deleted paths.  Is that the best we can do?  

I am wondering if we can special case the no-pathspec case to have
add_files_to_cache() call underlying run_diff_files() without any
pathspec, inspect the paths that are modified and/or deleted in the
update_callback, add ones that are under the $prefix while noticing
the ones outside as warning worthy events.

Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Jonathan Nieder]

Junio C Hamano wrote:

> The implemenation appears to run an extra diff_files() in addition
> to the one we already have to run in order to implement the "add -u"
> to collect modified/deleted paths.  Is that the best we can do?  
>
> I am wondering if we can special case the no-pathspec case to have
> add_files_to_cache() call underlying run_diff_files() without any
> pathspec, inspect the paths that are modified and/or deleted in the
> update_callback, add ones that are under the $prefix while noticing
> the ones outside as warning worthy events.

Yes, that can work, for example like this (replacing the patch you're
replying to).

-- >8 --
Subject: add -u: only show pathless 'add -u' warning when changes exist outside cwd

A common workflow in large projects is to chdir into a subdirectory of
interest and only do work there:

	cd src
	vi foo.c
	make test
	git add -u
	git commit

The upcoming change to 'git add -u' behavior would not affect such a
workflow: when the only changes present are in the current directory,
'git add -u' will add all changes, and whether that happens via an
implicit "." or implicit ":/" parameter is an unimportant
implementation detail.

The warning about use of 'git add -u' with no pathspec is annoying
because it serves no purpose in this case.  So suppress the warning
unless there are changes outside the cwd that are not being added.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/add.c | 41 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index a424e69d..e3ed5d93 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,6 +26,7 @@ static int take_worktree_changes;
 struct update_callback_data {
 	int flags;
 	int add_errors;
+	const char **implicit_dot;
 };
 
 static const char *option_with_implicit_dot;
@@ -94,10 +95,26 @@ static void update_callback(struct diff_queue_struct *q,
 {
 	int i;
 	struct update_callback_data *data = cbdata;
+	const char **implicit_dot = data->implicit_dot;
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 		const char *path = p->one->path;
+
+		/*
+		 * Check if "git add -A" or "git add -u" was run from a
+		 * subdirectory with a modified file outside that directory,
+		 * and warn if so.
+		 *
+		 * "git add -u" will behave like "git add -u :/" instead of
+		 * "git add -u ." in the future.  This warning prepares for
+		 * that change.
+		 */
+		if (implicit_dot &&
+		    !match_pathspec(implicit_dot, path, strlen(path), 0, NULL)) {
+			warn_pathless_add();
+			continue;
+		}
 		switch (fix_unmerged_status(p, data)) {
 		default:
 			die(_("unexpected diff status %c"), p->status);
@@ -121,17 +138,30 @@ static void update_callback(struct diff_queue_struct *q,
 	}
 }
 
+#define ADD_CACHE_IMPLICIT_DOT 32
 int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
 {
 	struct update_callback_data data;
 	struct rev_info rev;
+
+	data.flags = flags & ~ADD_CACHE_IMPLICIT_DOT;
+	data.add_errors = 0;
+	data.implicit_dot = NULL;
+	if (flags & ADD_CACHE_IMPLICIT_DOT) {
+		/*
+		 * Check for modified files throughout the worktree so
+		 * update_callback has a chance to warn about changes
+		 * outside the cwd.
+		 */
+		data.implicit_dot = pathspec;
+		pathspec = NULL;
+	}
+
 	init_revisions(&rev, prefix);
 	setup_revisions(0, NULL, &rev, NULL);
 	init_pathspec(&rev.prune_data, pathspec);
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = update_callback;
-	data.flags = flags;
-	data.add_errors = 0;
 	rev.diffopt.format_callback_data = &data;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
@@ -371,6 +401,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int add_new_files;
 	int require_pathspec;
 	char *seen = NULL;
+	int implicit_dot = 0;
 
 	git_config(add_config, NULL);
 
@@ -400,10 +431,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	}
 	if (option_with_implicit_dot && !argc) {
 		static const char *here[2] = { ".", NULL };
-		if (prefix)
+		if (prefix && addremove)
 			warn_pathless_add();
 		argc = 1;
 		argv = here;
+		implicit_dot = 1;
 	}
 
 	add_new_files = !take_worktree_changes && !refresh_only;
@@ -416,7 +448,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		 (intent_to_add ? ADD_CACHE_INTENT : 0) |
 		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
 		 (!(addremove || take_worktree_changes)
-		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
+		  ? ADD_CACHE_IGNORE_REMOVAL : 0)) |
+		 (implicit_dot ? ADD_CACHE_IMPLICIT_DOT : 0);
 
 	if (require_pathspec && argc == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
-- 
1.8.2.rc3

Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Jonathan Nieder]

Junio C Hamano wrote:

> The implemenation appears to run an extra diff_files() in addition
> to the one we already have to run in order to implement the "add -u"
> to collect modified/deleted paths.  Is that the best we can do?  

At least the following should be squashed in to avoid wasted effort in
the easy case (cwd at top of worktree).  Thanks for catching it.

diff --git i/builtin/add.c w/builtin/add.c
index f05ec1c1..8e4cdfae 100644
--- i/builtin/add.c
+++ w/builtin/add.c
@@ -483,7 +483,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	 * "git add -u ." in the future.  This warning prepares for
 	 * that change.
 	 */
-	if (implicit_dot)
+	if (prefix && implicit_dot)
 		diff_files_with_callback(prefix, NULL,
 				warn_if_outside_pathspec, pathspec);
 

Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Duy Nguyen]

On Tue, Mar 19, 2013 at 11:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I am wondering if we can special case the no-pathspec case to have
> add_files_to_cache() call underlying run_diff_files() without any
> pathspec, inspect the paths that are modified and/or deleted in the
> update_callback, add ones that are under the $prefix while noticing
> the ones outside as warning worthy events.

My concern is run full-tree diff can be expensive on large repos (one
of the reasons the user may choose to work from within a
subdirectory). We can exit as soon as we find a difference outside
$prefix. But in case we find nothing, we need to diff the whole
worktree.
-- 
Duy

Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Jonathan Nieder]

Duy Nguyen wrote:

> My concern is run full-tree diff can be expensive on large repos (one
> of the reasons the user may choose to work from within a
> subdirectory). We can exit as soon as we find a difference outside
> $prefix. But in case we find nothing, we need to diff the whole
> worktree.

Yes.  This is an argument for the single-diff approach that Junio
suggested, since at least it's not significantly slower than the
future default behavior ("git add -u :/").

Sysadmins and others helping to train users will need to make sure
people working on large repos understand that they can use "git add -u ."
to avoid a speed penalty.

Thanks for looking it over,
Jonathan

Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Matthieu Moy]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The warning about use of 'git add -u' with no pathspec is annoying
> because it serves no purpose in this case.  So suppress the warning
> unless there are changes outside the cwd that are not being added.

No time to review the code now. I thought about implementing something
like that, but did not do it because I didn't want the change in the
code to be too big. At some point, we'll have to remove the warning and
it's easier with my version than with yours. But the "damage" to the
code do not seem too big, so that's probably OK and will actually reduce
the pain for some users.

Discussions showed that many people were already using "git add -u"
without knowing whether it was full tree or not, so for these people the
change is somehow harmless anyway ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Yes, that can work, for example like this (replacing the patch you're
> replying to).

I think that would be a better approach if we were to do this.  I
still have the same reservation that "this is fundamentally not
worse but still hurts the users more".

> +		/*
> +		 * Check if "git add -A" or "git add -u" was run from a
> +		 * subdirectory with a modified file outside that directory,
> +		 * and warn if so.
> +		 *
> +		 * "git add -u" will behave like "git add -u :/" instead of
> +		 * "git add -u ." in the future.  This warning prepares for
> +		 * that change.
> +		 */
> +		if (implicit_dot &&
> +		    !match_pathspec(implicit_dot, path, strlen(path), 0, NULL)) {

This one really should *not* use match_pathspec(), I think.

It is a special case where we were asked to limit to our directory
but decided to grab everything instead and filtering the outcome
outselves.  We should have a "path to the starting directory" aka
"prefix" in implicit_dot and check if path is covered by the prefix
instead.

> +			warn_pathless_add();
> +			continue;
> +		}
>  		switch (fix_unmerged_status(p, data)) {
>  		default:
>  			die(_("unexpected diff status %c"), p->status);

Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> No time to review the code now. I thought about implementing something
> like that, but did not do it because I didn't want the change in the
> code to be too big. At some point, we'll have to remove the warning and
> it's easier with my version than with yours. But the "damage" to the
> code do not seem too big, so that's probably OK and will actually reduce
> the pain for some users.

Getting these warnings is a *good* thing.

You may happen to have no changed path outside the current directory
with this particular invocation of "git add -u", or you may do, or
you may not *even* remember if you touched the paths outside.

Training your fingers to type "git add -u ." without having to even
think, is primarily to help the last case.

Squelching of the warning at the top-level is much less problematic
as it is much harder to forget if you are at the top level of the
working tree than forget if you touched paths outside the current
directory.  "I know I am at the top, so I type 'git add -u' without
dot---why do you punish me with the warning?" is a much more valid
complaint.

Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Jonathan Nieder]

Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>> No time to review the code now. I thought about implementing something
>> like that, but did not do it because I didn't want the change in the
>> code to be too big. At some point, we'll have to remove the warning and
>> it's easier with my version than with yours. But the "damage" to the
>> code do not seem too big, so that's probably OK and will actually reduce
>> the pain for some users.
>
> Getting these warnings is a *good* thing.
>
> You may happen to have no changed path outside the current directory
> with this particular invocation of "git add -u", or you may do, or
> you may not *even* remember if you touched the paths outside.
>
> Training your fingers to type "git add -u ." without having to even
> think, is primarily to help the last case.

The problem is that these warnings are triggering way too often.  It
is like the story of the boy who cried "wolf": instead of training
people to type "git add -u .", we are training them to ignore
warnings.

I personally often find myself in the following situation:

	$ cd repowithdeepsubdirs/third_party/git
	$ ... hack hack hack ...
	$ git add -u

The result is a pile of warning text that I cannot convince myself not
to ignore because I already *knew* that the only changes present were
under the cwd.  The old and new "git add -u" behaviors always have the
same effect in this case, so the warning is not relevant to me.  So I
find myself being trained to ignore the warning.

Presumably habitual Java hackers that do their work in a
com/long/package/name subdirectory of the toplevel would find this
even more annoying.

One important exception is that if "git add -u :/" is slow, users need
to learn to run "git add -u ." to speed the operation up.  But I think
that is intuitive already.  Running a full-tree diff which slows down
the code that decides whether to print a warning is a good way to
train people regarding how long to expect a plain "git add -u" to
take.

Hoping that clarifies,
Jonathan

Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> ...  So I
> find myself being trained to ignore the warning.
> ...  Running a full-tree diff which slows down
> the code that decides whether to print a warning is a good way to
> train people regarding how long to expect a plain "git add -u" to
> take.

Ok, fair enough.

Incidentally, I am rebuilding the 'next' by kicking many of the
topics back to 'pu' (essentially, only the ones marked as "Safe" in
the latest issue of "What's cooking" are kept in 'next'), so perhaps
we can rebuild the jc/add-2.0-u-A-sans-pathspec topic with your
series at the bottom, or something?  I do not think I have time to
get around to it myself until later in the evening, though.

Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Jonathan Nieder]

Junio C Hamano wrote:

> Incidentally, I am rebuilding the 'next' by kicking many of the
> topics back to 'pu' (essentially, only the ones marked as "Safe" in
> the latest issue of "What's cooking" are kept in 'next'), so perhaps
> we can rebuild the jc/add-2.0-u-A-sans-pathspec topic with your
> series at the bottom, or something?  I do not think I have time to
> get around to it myself until later in the evening, though.

Sounds good.  I'll reroll with the use-prefix-not-pathspec tweak and
incorporate the current patches from jc/add-2.0-u-A-sans-pathspec.

[PATCH v2 0/6] make pathless 'add [-u|-A]' warning less noisy

[Jonathan Nieder]

As promised, here is a rerolled version of the "make pathless 'add
[-u|-A]' warning less noisy", incorporating patches from
jc/add-2.0-u-A-sans-pathspec.  Thanks for your help so far.

Just like jc/add-2.0-u-A-sans-pathspec, the only transition in this
series is the "pull the bandaid" kind.  That is, there are two steps:

 0. the current state, where the warning is a little too noisy
 1. the current state but with the warning only firing in cases
    where the user will be affected by the change to default
    'git add -u' behavior
 2. no more warning, 'git add -u' defaulting to 'git add -u :/'

Patch 1 is the same as in jc/add-2.0-u-A-sans-pathspec, included for
reference.

Patches 2-5 correspond to the original patches 1-4.  Any changes are
described after the '---' in each patch.

Patch 6 is just the patch from the tip of jc/add-2.0-u-A-sans-pathspec,
rebased.  It is meant to be applied in the 2.0 cycle.

Jeff King (1):
  t2200: check that "add -u" limits itself to subdirectory

Jonathan Nieder (4):
  add: make pathless 'add [-u|-A]' warning a file-global function
  add: make warn_pathless_add() a no-op after first call
  add -u: only show pathless 'add -u' warning when changes exist outside
    cwd
  add -A: only show pathless 'add -A' warning when changes exist outside
    cwd

Junio C Hamano (1):
  git add: -u/-A now affects the entire working tree

 Documentation/git-add.txt | 16 +++++++-------
 builtin/add.c             | 56 ++++++++---------------------------------------
 t/t2200-add-update.sh     | 11 ++++++++++
 3 files changed, 28 insertions(+), 55 deletions(-)

[PATCH 1/6] t2200: check that "add -u" limits itself to subdirectory

[Jonathan Nieder]

From: Jeff King <peff@peff.net>
Date: Thu, 14 Mar 2013 02:44:04 -0400

This behavior is due to change in the future, but let's test
it anyway. That helps make sure we do not accidentally
switch the behavior too soon while we are working in the
area, and it means that we can easily verify the change when
we do make it.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Unchanged, only included for reference.

 t/t2200-add-update.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index 4cdebda..c317254 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -80,6 +80,22 @@ test_expect_success 'change gets noticed' '
 
 '
 
+# Note that this is scheduled to change in Git 2.0, when
+# "git add -u" will become full-tree by default.
+test_expect_success 'non-limited update in subdir leaves root alone' '
+	(
+		cd dir1 &&
+		echo even more >>sub2 &&
+		git add -u
+	) &&
+	cat >expect <<-\EOF &&
+	check
+	top
+	EOF
+	git diff-files --name-only >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success SYMLINKS 'replace a file with a symlink' '
 
 	rm foo &&
-- 
1.8.2.rc3

[PATCH 2/6] add: make pathless 'add [-u|-A]' warning a file-global function

[Jonathan Nieder]

Currently warn_pathless_add() is only called directly by cmd_add(),
but that is about to change.  Move its definition higher in the file
and pass the "--update" or "--all" option name used in its message
through globals instead of function arguments to make it easier to
call without passing values that will not change through the call
chain.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Unchanged.

 builtin/add.c | 69 +++++++++++++++++++++++++++++++----------------------------
 1 file changed, 36 insertions(+), 33 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index ab1c9e8..a4028ee 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -28,6 +28,41 @@ struct update_callback_data {
 	int add_errors;
 };
 
+static const char *option_with_implicit_dot;
+static const char *short_option_with_implicit_dot;
+
+static void warn_pathless_add(void)
+{
+	assert(option_with_implicit_dot && short_option_with_implicit_dot);
+
+	/*
+	 * To be consistent with "git add -p" and most Git
+	 * commands, we should default to being tree-wide, but
+	 * this is not the original behavior and can't be
+	 * changed until users trained themselves not to type
+	 * "git add -u" or "git add -A". For now, we warn and
+	 * keep the old behavior. Later, the behavior can be changed
+	 * to tree-wide, keeping the warning for a while, and
+	 * eventually we can drop the warning.
+	 */
+	warning(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
+		  "subdirectory of the tree will change in Git 2.0 and should not be used anymore.\n"
+		  "To add content for the whole tree, run:\n"
+		  "\n"
+		  "  git add %s :/\n"
+		  "  (or git add %s :/)\n"
+		  "\n"
+		  "To restrict the command to the current directory, run:\n"
+		  "\n"
+		  "  git add %s .\n"
+		  "  (or git add %s .)\n"
+		  "\n"
+		  "With the current Git version, the command is restricted to the current directory."),
+		option_with_implicit_dot, short_option_with_implicit_dot,
+		option_with_implicit_dot, short_option_with_implicit_dot,
+		option_with_implicit_dot, short_option_with_implicit_dot);
+}
+
 static int fix_unmerged_status(struct diff_filepair *p,
 			       struct update_callback_data *data)
 {
@@ -321,35 +356,6 @@ static int add_files(struct dir_struct *dir, int flags)
 	return exit_status;
 }
 
-static void warn_pathless_add(const char *option_name, const char *short_name) {
-	/*
-	 * To be consistent with "git add -p" and most Git
-	 * commands, we should default to being tree-wide, but
-	 * this is not the original behavior and can't be
-	 * changed until users trained themselves not to type
-	 * "git add -u" or "git add -A". For now, we warn and
-	 * keep the old behavior. Later, the behavior can be changed
-	 * to tree-wide, keeping the warning for a while, and
-	 * eventually we can drop the warning.
-	 */
-	warning(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
-		  "subdirectory of the tree will change in Git 2.0 and should not be used anymore.\n"
-		  "To add content for the whole tree, run:\n"
-		  "\n"
-		  "  git add %s :/\n"
-		  "  (or git add %s :/)\n"
-		  "\n"
-		  "To restrict the command to the current directory, run:\n"
-		  "\n"
-		  "  git add %s .\n"
-		  "  (or git add %s .)\n"
-		  "\n"
-		  "With the current Git version, the command is restricted to the current directory."),
-		option_name, short_name,
-		option_name, short_name,
-		option_name, short_name);
-}
-
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
 	int exit_status = 0;
@@ -360,8 +366,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int add_new_files;
 	int require_pathspec;
 	char *seen = NULL;
-	const char *option_with_implicit_dot = NULL;
-	const char *short_option_with_implicit_dot = NULL;
 
 	git_config(add_config, NULL);
 
@@ -392,8 +396,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (option_with_implicit_dot && !argc) {
 		static const char *here[2] = { ".", NULL };
 		if (prefix)
-			warn_pathless_add(option_with_implicit_dot,
-					  short_option_with_implicit_dot);
+			warn_pathless_add();
 		argc = 1;
 		argv = here;
 	}
-- 
1.8.2.rc3

[PATCH 3/6] add: make warn_pathless_add() a no-op after first call

[Jonathan Nieder]

Make warn_pathless_add() print its warning the first time it is called
and do nothing if called again.  This will make it easier to show the
warning on the fly when a relevant condition is detected without
risking showing it multiple times when multiple such conditions hold.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
As before.

 builtin/add.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/add.c b/builtin/add.c
index a4028ee..a424e69 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -33,8 +33,13 @@ static const char *short_option_with_implicit_dot;
 
 static void warn_pathless_add(void)
 {
+	static int shown;
 	assert(option_with_implicit_dot && short_option_with_implicit_dot);
 
+	if (shown)
+		return;
+	shown = 1;
+
 	/*
 	 * To be consistent with "git add -p" and most Git
 	 * commands, we should default to being tree-wide, but
-- 
1.8.2.rc3

[PATCH 4/6] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Jonathan Nieder]

A common workflow in large projects is to chdir into a subdirectory of
interest and only do work there:

	cd src
	vi foo.c
	make test
	git add -u
	git commit

The upcoming change to 'git add -u' behavior would not affect such a
workflow: when the only changes present are in the current directory,
'git add -u' will add all changes, and whether that happens via an
implicit "." or implicit ":/" parameter is an unimportant
implementation detail.

The warning about use of 'git add -u' with no pathspec is annoying
because it seemingly serves no purpose in this case.  So suppress the
warning unless there are changes outside the cwd that are not being
added.

A previous version of this patch ran two I/O-intensive diff-files
passes: one to find changes outside the cwd, and another to find
changes to add to the index within the cwd.  This version runs one
full-tree diff and decides for each change whether to add it or warn
and suppress it in update_callback.  As a result, even on very large
repositories "git add -u" will not be significantly slower than the
future default behavior ("git add -u :/"), and the slowdown relative
to "git add -u ." should be a useful clue to users of such
repositories to get into the habit of explicitly passing '.'.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Jeff King <peff@peff.net>
Improved-by: Junio C Hamano <gitster@pobox.com>
---
This is the interesting one.

Changes since v1:
 * run only one diff-files pass, as explained in the log message
 * use strncmp_icase, not match_pathspec, to check if paths are
   under cwd
 * expand log message with performance implications
 * summarized Peff's review with an Ack.  I hope that's ok.

Thanks for your help getting this into good shape.

 builtin/add.c | 43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index a424e69..4d8d441 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,6 +26,8 @@ static int take_worktree_changes;
 struct update_callback_data {
 	int flags;
 	int add_errors;
+	const char *implicit_dot;
+	size_t implicit_dot_len;
 };
 
 static const char *option_with_implicit_dot;
@@ -94,10 +96,27 @@ static void update_callback(struct diff_queue_struct *q,
 {
 	int i;
 	struct update_callback_data *data = cbdata;
+	const char *implicit_dot = data->implicit_dot;
+	size_t implicit_dot_len = data->implicit_dot_len;
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 		const char *path = p->one->path;
+
+		/*
+		 * Check if "git add -A" or "git add -u" was run from a
+		 * subdirectory with a modified file outside that directory,
+		 * and warn if so.
+		 *
+		 * "git add -u" will behave like "git add -u :/" instead of
+		 * "git add -u ." in the future.  This warning prepares for
+		 * that change.
+		 */
+		if (implicit_dot &&
+		    strncmp_icase(path, implicit_dot, implicit_dot_len)) {
+			warn_pathless_add();
+			continue;
+		}
 		switch (fix_unmerged_status(p, data)) {
 		default:
 			die(_("unexpected diff status %c"), p->status);
@@ -121,17 +140,30 @@ static void update_callback(struct diff_queue_struct *q,
 	}
 }
 
+#define ADD_CACHE_IMPLICIT_DOT 32
 int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
 {
 	struct update_callback_data data;
 	struct rev_info rev;
+
+	memset(&data, 0, sizeof(data));
+	data.flags = flags & ~ADD_CACHE_IMPLICIT_DOT;
+	if ((flags & ADD_CACHE_IMPLICIT_DOT) && prefix) {
+		/*
+		 * Check for modified files throughout the worktree so
+		 * update_callback has a chance to warn about changes
+		 * outside the cwd.
+		 */
+		data.implicit_dot = prefix;
+		data.implicit_dot_len = strlen(prefix);
+		pathspec = NULL;
+	}
+
 	init_revisions(&rev, prefix);
 	setup_revisions(0, NULL, &rev, NULL);
 	init_pathspec(&rev.prune_data, pathspec);
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = update_callback;
-	data.flags = flags;
-	data.add_errors = 0;
 	rev.diffopt.format_callback_data = &data;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
@@ -371,6 +403,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int add_new_files;
 	int require_pathspec;
 	char *seen = NULL;
+	int implicit_dot = 0;
 
 	git_config(add_config, NULL);
 
@@ -400,10 +433,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	}
 	if (option_with_implicit_dot && !argc) {
 		static const char *here[2] = { ".", NULL };
-		if (prefix)
+		if (prefix && addremove)
 			warn_pathless_add();
 		argc = 1;
 		argv = here;
+		implicit_dot = 1;
 	}
 
 	add_new_files = !take_worktree_changes && !refresh_only;
@@ -416,7 +450,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		 (intent_to_add ? ADD_CACHE_INTENT : 0) |
 		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
 		 (!(addremove || take_worktree_changes)
-		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
+		  ? ADD_CACHE_IGNORE_REMOVAL : 0)) |
+		 (implicit_dot ? ADD_CACHE_IMPLICIT_DOT : 0);
 
 	if (require_pathspec && argc == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
-- 
1.8.2.rc3

[PATCH 5/6] add -A: only show pathless 'add -A' warning when changes exist outside cwd

[Jonathan Nieder]

In the spirit of the recent similar change for 'git add -u', avoid
pestering users that restrict their attention to a subdirectory and
will not be affected by the coming change in the behavior of pathless
'git add -A'.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
As before.

 builtin/add.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 4d8d441..2493493 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -170,7 +170,9 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
 	return !!data.add_errors;
 }
 
-static char *prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
+#define WARN_IMPLICIT_DOT (1u << 0)
+static char *prune_directory(struct dir_struct *dir, const char **pathspec,
+			     int prefix, unsigned flag)
 {
 	char *seen;
 	int i, specs;
@@ -187,6 +189,16 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec, int
 		if (match_pathspec(pathspec, entry->name, entry->len,
 				   prefix, seen))
 			*dst++ = entry;
+		else if (flag & WARN_IMPLICIT_DOT)
+			/*
+			 * "git add -A" was run from a subdirectory with a
+			 * new file outside that directory.
+			 *
+			 * "git add -A" will behave like "git add -A :/"
+			 * instead of "git add -A ." in the future.
+			 * Warn about the coming behavior change.
+			 */
+			warn_pathless_add();
 	}
 	dir->nr = dst - dir->entries;
 	add_pathspec_matches_against_index(pathspec, seen, specs);
@@ -433,8 +445,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	}
 	if (option_with_implicit_dot && !argc) {
 		static const char *here[2] = { ".", NULL };
-		if (prefix && addremove)
-			warn_pathless_add();
 		argc = 1;
 		argv = here;
 		implicit_dot = 1;
@@ -475,9 +485,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		}
 
 		/* This picks up the paths that are not tracked */
-		baselen = fill_directory(&dir, pathspec);
+		baselen = fill_directory(&dir, implicit_dot ? NULL : pathspec);
 		if (pathspec)
-			seen = prune_directory(&dir, pathspec, baselen);
+			seen = prune_directory(&dir, pathspec, baselen,
+					implicit_dot ? WARN_IMPLICIT_DOT : 0);
 	}
 
 	if (refresh_only) {
-- 
1.8.2.rc3

[PATCH 6/6] git add: -u/-A now affects the entire working tree

[Jonathan Nieder]

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

As promised in 0fa2eb530fb7 (add: warn when -u or -A is used without
pathspec, 2013-01-28), in Git 2.0, "git add -u/-A" that is run
without pathspec in a subdirectory updates all updated paths in the
entire working tree, not just the current directory and its
subdirectories.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thanks for reading.

 Documentation/git-add.txt |  16 +++----
 builtin/add.c             | 110 ++++------------------------------------------
 t/t2200-add-update.sh     |   9 +---
 3 files changed, 19 insertions(+), 116 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index b0944e5..02b99cb 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -104,10 +104,10 @@ apply to the index. See EDITING PATCHES below.
 	<pathspec>.  This removes as well as modifies index entries to
 	match the working tree, but adds no new files.
 +
-If no <pathspec> is given, the current version of Git defaults to
-"."; in other words, update all tracked files in the current directory
-and its subdirectories. This default will change in a future version
-of Git, hence the form without <pathspec> should not be used.
+If no <pathspec> is given when `-u` option is used, all
+tracked files in the entire working tree are updated (old versions
+of Git used to limit the update to the current directory and its
+subdirectories).
 
 -A::
 --all::
@@ -116,10 +116,10 @@ of Git, hence the form without <pathspec> should not be used.
 	entry.	This adds, modifies, and removes index entries to
 	match the working tree.
 +
-If no <pathspec> is given, the current version of Git defaults to
-"."; in other words, update all files in the current directory
-and its subdirectories. This default will change in a future version
-of Git, hence the form without <pathspec> should not be used.
+If no <pathspec> is given when `-A` option is used, all
+files in the entire working tree are updated (old versions
+of Git used to limit the update to the current directory and its
+subdirectories).
 
 -N::
 --intent-to-add::
diff --git a/builtin/add.c b/builtin/add.c
index 2493493..5c0a869 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,50 +26,8 @@ static int take_worktree_changes;
 struct update_callback_data {
 	int flags;
 	int add_errors;
-	const char *implicit_dot;
-	size_t implicit_dot_len;
 };
 
-static const char *option_with_implicit_dot;
-static const char *short_option_with_implicit_dot;
-
-static void warn_pathless_add(void)
-{
-	static int shown;
-	assert(option_with_implicit_dot && short_option_with_implicit_dot);
-
-	if (shown)
-		return;
-	shown = 1;
-
-	/*
-	 * To be consistent with "git add -p" and most Git
-	 * commands, we should default to being tree-wide, but
-	 * this is not the original behavior and can't be
-	 * changed until users trained themselves not to type
-	 * "git add -u" or "git add -A". For now, we warn and
-	 * keep the old behavior. Later, the behavior can be changed
-	 * to tree-wide, keeping the warning for a while, and
-	 * eventually we can drop the warning.
-	 */
-	warning(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
-		  "subdirectory of the tree will change in Git 2.0 and should not be used anymore.\n"
-		  "To add content for the whole tree, run:\n"
-		  "\n"
-		  "  git add %s :/\n"
-		  "  (or git add %s :/)\n"
-		  "\n"
-		  "To restrict the command to the current directory, run:\n"
-		  "\n"
-		  "  git add %s .\n"
-		  "  (or git add %s .)\n"
-		  "\n"
-		  "With the current Git version, the command is restricted to the current directory."),
-		option_with_implicit_dot, short_option_with_implicit_dot,
-		option_with_implicit_dot, short_option_with_implicit_dot,
-		option_with_implicit_dot, short_option_with_implicit_dot);
-}
-
 static int fix_unmerged_status(struct diff_filepair *p,
 			       struct update_callback_data *data)
 {
@@ -96,27 +54,11 @@ static void update_callback(struct diff_queue_struct *q,
 {
 	int i;
 	struct update_callback_data *data = cbdata;
-	const char *implicit_dot = data->implicit_dot;
-	size_t implicit_dot_len = data->implicit_dot_len;
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 		const char *path = p->one->path;
 
-		/*
-		 * Check if "git add -A" or "git add -u" was run from a
-		 * subdirectory with a modified file outside that directory,
-		 * and warn if so.
-		 *
-		 * "git add -u" will behave like "git add -u :/" instead of
-		 * "git add -u ." in the future.  This warning prepares for
-		 * that change.
-		 */
-		if (implicit_dot &&
-		    strncmp_icase(path, implicit_dot, implicit_dot_len)) {
-			warn_pathless_add();
-			continue;
-		}
 		switch (fix_unmerged_status(p, data)) {
 		default:
 			die(_("unexpected diff status %c"), p->status);
@@ -140,24 +82,13 @@ static void update_callback(struct diff_queue_struct *q,
 	}
 }
 
-#define ADD_CACHE_IMPLICIT_DOT 32
 int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
 {
 	struct update_callback_data data;
 	struct rev_info rev;
 
 	memset(&data, 0, sizeof(data));
-	data.flags = flags & ~ADD_CACHE_IMPLICIT_DOT;
-	if ((flags & ADD_CACHE_IMPLICIT_DOT) && prefix) {
-		/*
-		 * Check for modified files throughout the worktree so
-		 * update_callback has a chance to warn about changes
-		 * outside the cwd.
-		 */
-		data.implicit_dot = prefix;
-		data.implicit_dot_len = strlen(prefix);
-		pathspec = NULL;
-	}
+	data.flags = flags;
 
 	init_revisions(&rev, prefix);
 	setup_revisions(0, NULL, &rev, NULL);
@@ -170,9 +101,7 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
 	return !!data.add_errors;
 }
 
-#define WARN_IMPLICIT_DOT (1u << 0)
-static char *prune_directory(struct dir_struct *dir, const char **pathspec,
-			     int prefix, unsigned flag)
+static char *prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
 {
 	char *seen;
 	int i, specs;
@@ -189,16 +118,6 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec,
 		if (match_pathspec(pathspec, entry->name, entry->len,
 				   prefix, seen))
 			*dst++ = entry;
-		else if (flag & WARN_IMPLICIT_DOT)
-			/*
-			 * "git add -A" was run from a subdirectory with a
-			 * new file outside that directory.
-			 *
-			 * "git add -A" will behave like "git add -A :/"
-			 * instead of "git add -A ." in the future.
-			 * Warn about the coming behavior change.
-			 */
-			warn_pathless_add();
 	}
 	dir->nr = dst - dir->entries;
 	add_pathspec_matches_against_index(pathspec, seen, specs);
@@ -415,7 +334,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	int add_new_files;
 	int require_pathspec;
 	char *seen = NULL;
-	int implicit_dot = 0;
 
 	git_config(add_config, NULL);
 
@@ -435,19 +353,11 @@ 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) {
-		option_with_implicit_dot = "--all";
-		short_option_with_implicit_dot = "-A";
-	}
-	if (take_worktree_changes) {
-		option_with_implicit_dot = "--update";
-		short_option_with_implicit_dot = "-u";
-	}
-	if (option_with_implicit_dot && !argc) {
-		static const char *here[2] = { ".", NULL };
+
+	if ((addremove || take_worktree_changes) && !argc) {
+		static const char *whole[2] = { ":/", NULL };
 		argc = 1;
-		argv = here;
-		implicit_dot = 1;
+		argv = whole;
 	}
 
 	add_new_files = !take_worktree_changes && !refresh_only;
@@ -460,8 +370,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		 (intent_to_add ? ADD_CACHE_INTENT : 0) |
 		 (ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
 		 (!(addremove || take_worktree_changes)
-		  ? ADD_CACHE_IGNORE_REMOVAL : 0)) |
-		 (implicit_dot ? ADD_CACHE_IMPLICIT_DOT : 0);
+		  ? ADD_CACHE_IGNORE_REMOVAL : 0));
 
 	if (require_pathspec && argc == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
@@ -485,10 +394,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		}
 
 		/* This picks up the paths that are not tracked */
-		baselen = fill_directory(&dir, implicit_dot ? NULL : pathspec);
+		baselen = fill_directory(&dir, pathspec);
 		if (pathspec)
-			seen = prune_directory(&dir, pathspec, baselen,
-					implicit_dot ? WARN_IMPLICIT_DOT : 0);
+			seen = prune_directory(&dir, pathspec, baselen);
 	}
 
 	if (refresh_only) {
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index c317254..4281e18 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -80,18 +80,13 @@ test_expect_success 'change gets noticed' '
 
 '
 
-# Note that this is scheduled to change in Git 2.0, when
-# "git add -u" will become full-tree by default.
-test_expect_success 'non-limited update in subdir leaves root alone' '
+test_expect_success 'non-qualified update in subdir updates from the root' '
 	(
 		cd dir1 &&
 		echo even more >>sub2 &&
 		git add -u
 	) &&
-	cat >expect <<-\EOF &&
-	check
-	top
-	EOF
+	: >expect &&
 	git diff-files --name-only >actual &&
 	test_cmp expect actual
 '
-- 
1.8.2.rc3

Re: [PATCH 4/6] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Jeff King]

On Tue, Mar 19, 2013 at 03:50:50PM -0700, Jonathan Nieder wrote:

> This is the interesting one.
> [...]
>  * summarized Peff's review with an Ack.  I hope that's ok.

Yeah, OK with me. I certainly agree with the intent, and I think your
reasoning on the performance change is valid.

I don't see anything wrong in the patch itself, except for one minor
nit:

> @@ -121,17 +140,30 @@ static void update_callback(struct diff_queue_struct *q,
>  	}
>  }
>  
> +#define ADD_CACHE_IMPLICIT_DOT 32
>  int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
>  {
>  	struct update_callback_data data;
>  	struct rev_info rev;

Should this be defined in cache.h with the other flags? I realize it's
mostly private to builtin/add.c, but without even a comment in cache.h
saying "/* 32 is reserved */", we run the risk of another commit
adding a new flag with the same number.

-Peff

Re: [PATCH 4/6] add -u: only show pathless 'add -u' warning when changes exist outside cwd

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> A common workflow in large projects is to chdir into a subdirectory of
> interest and only do work there:
>
> 	cd src
> 	vi foo.c
> 	make test
> 	git add -u
> 	git commit
>
> The upcoming change to 'git add -u' behavior would not affect such a
> workflow: when the only changes present are in the current directory,
> 'git add -u' will add all changes, and whether that happens via an
> implicit "." or implicit ":/" parameter is an unimportant
> implementation detail.
>
> The warning about use of 'git add -u' with no pathspec is annoying
> because it seemingly serves no purpose in this case.  So suppress the
> warning unless there are changes outside the cwd that are not being
> added.
>
> A previous version of this patch ran two I/O-intensive diff-files
> passes: one to find changes outside the cwd, and another to find
> changes to add to the index within the cwd.  This version runs one
> full-tree diff and decides for each change whether to add it or warn
> and suppress it in update_callback.  As a result, even on very large
> repositories "git add -u" will not be significantly slower than the
> future default behavior ("git add -u :/"), and the slowdown relative
> to "git add -u ." should be a useful clue to users of such
> repositories to get into the habit of explicitly passing '.'.

I'll queue this as-is for now, but the point Peff raised on
IMPLICIT_DOT needs to be addressed.

This is a tangent, but I would also suggest at least considering to
measure how big the working tree is relative to the area covered by
the implicit dot [*1*].  If you are running "add -u" in a project
with 30k paths but are working with only a few dozen files, you may
want more encouragement to use an explicit "."  than the command
mysteriously and silently getting slower at Git version boundary.

IOW, an advice message issued only when (1) you are indeed working
in a narrow directory, (2) you did not touch anything outside, and
(3) you did not give an explicit ".".  We would want to keep the
advice in place even after Git 2.0 switches the default to ":/".  In
fact, it would become even more valuable after the transition. And
make it protected under advice.addUAuseexplicitdot or something.

By the way, I am not a big fan of the approach taken in [3/6].  It
may be a lot cleaner to separate the "do we need to warn?" logic
that flips a global (or a field in a callback, if we make some of
these codepaths callable from other places) and the code to issue
the warning, no?


[Footnote]

*1* The former you can get an approximation from active_nr, the
latter you can count after first finding where prefix would insert
to the active_cache with index_name_pos().  I suspect that you also
could add a new read-only API to cache-tree to see if it already
knows how many index entries a given subtree for the prefix covers.

Re: [PATCH 5/6] add -A: only show pathless 'add -A' warning when changes exist outside cwd

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> In the spirit of the recent similar change for 'git add -u', avoid
> pestering users that restrict their attention to a subdirectory and
> will not be affected by the coming change in the behavior of pathless
> 'git add -A'.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> As before.

Have you considered implementing this by adding a separate check
immediately after fill_directory() to inspect the paths in dir with
the same strncmp_icase() against the prefix, without touching
prune_directory() at all?  I suspect that would be much cleaner and
easier to change in the version boundary.

Same comment about measuring the size of the working tree and the
area the user is working on applies to this.  After Git 2.0, we
would still want to advise users who say "git add -A" without
pathspecs to see if the user would have been better off with an
explicit ".", so unless advice.addUAuseexplicitdot is set to false,
we would still want to inspect the result from fill_directory (and
in that case we won't be calling into prune_directory).

>  builtin/add.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 4d8d441..2493493 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -170,7 +170,9 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
>  	return !!data.add_errors;
>  }
>  
> -static char *prune_directory(struct dir_struct *dir, const char **pathspec, int prefix)
> +#define WARN_IMPLICIT_DOT (1u << 0)
> +static char *prune_directory(struct dir_struct *dir, const char **pathspec,
> +			     int prefix, unsigned flag)
>  {
>  	char *seen;
>  	int i, specs;
> @@ -187,6 +189,16 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec, int
>  		if (match_pathspec(pathspec, entry->name, entry->len,
>  				   prefix, seen))
>  			*dst++ = entry;
> +		else if (flag & WARN_IMPLICIT_DOT)
> +			/*
> +			 * "git add -A" was run from a subdirectory with a
> +			 * new file outside that directory.
> +			 *
> +			 * "git add -A" will behave like "git add -A :/"
> +			 * instead of "git add -A ." in the future.
> +			 * Warn about the coming behavior change.
> +			 */
> +			warn_pathless_add();
>  	}
>  	dir->nr = dst - dir->entries;
>  	add_pathspec_matches_against_index(pathspec, seen, specs);
> @@ -433,8 +445,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  	}
>  	if (option_with_implicit_dot && !argc) {
>  		static const char *here[2] = { ".", NULL };
> -		if (prefix && addremove)
> -			warn_pathless_add();
>  		argc = 1;
>  		argv = here;
>  		implicit_dot = 1;
> @@ -475,9 +485,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  		}
>  
>  		/* This picks up the paths that are not tracked */
> -		baselen = fill_directory(&dir, pathspec);
> +		baselen = fill_directory(&dir, implicit_dot ? NULL : pathspec);
>  		if (pathspec)
> -			seen = prune_directory(&dir, pathspec, baselen);
> +			seen = prune_directory(&dir, pathspec, baselen,
> +					implicit_dot ? WARN_IMPLICIT_DOT : 0);
>  	}
>  
>  	if (refresh_only) {

Re: [PATCH 2/2] add: add a newline at the end of pathless 'add [-u|-A]' warning

[Matthieu Moy]

Sorry for the late reply,

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

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> When the commands give an actual output (e.g. when ran with -v), the
>> output is visually mixed with the warning. The newline makes the actual
>> output more visible.
>>
>> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>> ---
>
> It would have been easier to immediately understand what is going on
> if you said "blank line" instead of "newline" ;-)

Indeed.

> An obvious issues is what if user does not run with "-v" or if "-v"
> produces no results.  We will be left with an extra blank line at
> the end.

Right, but displaying the blank line only when there's an actual output
does not seem easy, and I'd rather avoid too much damage in the code for
a warning which is only temporary.

> I suspect that the true reason why the warning does not stand out
> and other output looks mixed in it may be because we only prefix the
> first line with the "warning: " label.  In the longer term, I have a
> feeling that we should be showing something like this instead:
>
>     $ cd t && echo >>t0000*.sh && git add -u -v
>     warning: The behavior of 'git add --update (or -u)' with no path ar...
>     warning: subdirectory of the tree will change in Git 2.0 and should...
>     warning: To add content for the whole tree, run:

I personnally do not like this kind of output, the "warning:" on the 2nd
and 3rd lines break the flow reading the message. But that's probably a
matter of taste.

> using a logic similar to what strbuf_add_commented_lines() and
> strbuf_add_lines() use.

This would mean changing the warning() function, which would change all
warnings.

I'm fine with either dropping my patch or applying it as-is (with
s/newline/blank line/ in the commit message); a bit reluctant to
changing the output of warning(...), but that's an option if other
people like it.

>>  builtin/add.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/add.c b/builtin/add.c
>> index ab1c9e8..620bf00 100644
>> --- a/builtin/add.c
>> +++ b/builtin/add.c
>> @@ -344,7 +344,7 @@ static void warn_pathless_add(const char *option_name, const char *short_name) {
>>  		  "  git add %s .\n"
>>  		  "  (or git add %s .)\n"
>>  		  "\n"
>> -		  "With the current Git version, the command is restricted to the current directory."),
>> +		  "With the current Git version, the command is restricted to the current directory.\n"),
>>  		option_name, short_name,
>>  		option_name, short_name,
>>  		option_name, short_name);
>
>

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 2/2] add: add a newline at the end of pathless 'add [-u|-A]' warning

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> I'm fine with either dropping my patch or applying it as-is (with
> s/newline/blank line/ in the commit message).

OK; let's insert it immediately after e24afab09137 (add: make
pathless 'add [-u|-A]' warning a file-global function, 2013-03-19),
like the attached.

I'd prefer to spell this

	die("...\n"
	    "");

over

	die("...\n");

as the latter _looks_ as if the author didn't know die() gives us
line termination.  The former hopefully is more explicit that we do
want an empty line at the end.

commit a8ed21a59219e8fe81b76ed0961641555f25cdad
Author: Matthieu Moy <Matthieu.Moy@imag.fr>
Date:   Mon Mar 11 09:01:33 2013 +0100

    add: add a blank line at the end of pathless 'add [-u|-A]' warning
    
    When the commands give an actual output (e.g. when ran with -v), the
    output is visually mixed with the warning.
    
    An additional blank line makes the actual output more visible.
    
    Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/add.c b/builtin/add.c
index a4028ee..db02233 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -57,7 +57,9 @@ static void warn_pathless_add(void)
 		  "  git add %s .\n"
 		  "  (or git add %s .)\n"
 		  "\n"
-		  "With the current Git version, the command is restricted to the current directory."),
+		  "With the current Git version, the command is restricted to "
+		  "the current directory.\n",
+		  ""),
 		option_with_implicit_dot, short_option_with_implicit_dot,
 		option_with_implicit_dot, short_option_with_implicit_dot,
 		option_with_implicit_dot, short_option_with_implicit_dot);

Re: [PATCH 2/2] add: add a newline at the end of pathless 'add [-u|-A]' warning

[Matthieu Moy]

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

> diff --git a/builtin/add.c b/builtin/add.c
> index a4028ee..db02233 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -57,7 +57,9 @@ static void warn_pathless_add(void)
>  		  "  git add %s .\n"
>  		  "  (or git add %s .)\n"
>  		  "\n"
> -		  "With the current Git version, the command is restricted to the current directory."),
> +		  "With the current Git version, the command is restricted to "
> +		  "the current directory.\n",
> +		  ""),

Looks good. Thanks,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/