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

Jonathan Nieder [off-list ref] 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.

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

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

On Tue, Mar 19, 2013 at 11:25 AM, Junio C Hamano [off-list ref] 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

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