Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

Subsystems: the rest

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

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:53 

Jeff King [off-list ref] writes:

Yeah, I had the same thought, as this warning has been bugging me for
the last day or two. The worst part about it is that I finally trained
myself to type "git add ." to silence the _other_ warning, and now it
triggers this one. :)
So here is the "reworked" one on top of what is in 'next'.

It introduces a bit of conflict with the "add -u/-A" topic, so I am
not ready to push out the integration result yet.

-- >8 --
Subject: [PATCH] git add: rework the logic to warn "git add <pathspec>..." default change

The earlier logic to warn against "git add subdir" that is run
without "-A" or "--no-all" was only to check any <pathspec> given
exactly spells a directory name that (still) exists on the
filesystem.  This had number of problems:

 * "git add '*dir'" (note that the wildcard is hidden from the
   shell) would not trigger the warning.

 * "git add '*.py'" would behave differently between the current
   version of Git and Git 2.0 for the same reason as "subdir", but
   would not trigger the warning.

 * "git add dir" for a submodule "dir" would just update the index
   entry for the submodule "dir" without ever recursing into it, and
   use of "-A" or "--no-all" would matter.  But the logic only
   checks the directory-ness of "dir" and gives an unnecessary
   warning.

Rework the logic to detect the case where the behaviour will be
different in Git 2.0, and issue a warning only when it matters.
Even with the code before this warning, "git add subdir" will have
to traverse the directory in order to find _new_ files the index
does not know about _anyway_, so we can do this check without adding
an extra pass to find if <pathspec> matches any removed file.

This essentially updates the "add_files_to_cache()" public API to
"update_files_in_cache()" API that is internal to "git add", because
with the "--all" option, the function is no longer about "adding"
paths to the cache, but is also used to remove them.

There are other callers of the former from "checkout" (used when
"checkout -m" prepares the temporary tree that represents the local
modifications to be merged) and "commit" ("commit --include" that
picks up local changes in addition to what is in the index).  Since
ADD_CACHE_IGNORE_ERRORS (aka "--no-all") is not used by either of
them, once dust settles after Git 2.0 and the warning becomes
unnecessary, we may want to unify these two functions again.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin/add.c | 64 +++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 26 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index f8f6c9e..4242bce 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,6 +26,9 @@ static int take_worktree_changes;
 struct update_callback_data {
 	int flags;
 	int add_errors;
+
+	/* only needed for 2.0 transition preparation */
+	int warn_add_would_remove;
 };
 
 static int fix_unmerged_status(struct diff_filepair *p,
@@ -49,6 +52,17 @@ static int fix_unmerged_status(struct diff_filepair *p,
 		return DIFF_STATUS_MODIFIED;
 }
 
+static void warn_add_would_remove(const char *path)
+{
+	warning(_("In Git 2.0, 'git add <pathspec>...' will also update the\n"
+		  "index for paths removed from the working tree that match\n"
+		  "the given pathspec. If you want to 'add' only changed\n"
+		  "or newly created paths, say 'git add --no-all <pathspec>...'"
+		  " instead.\n\n"
+		  "'%s' would be removed from the index without --no-all."),
+		path);
+}
+
 static void update_callback(struct diff_queue_struct *q,
 			    struct diff_options *opt, void *cbdata)
 {
@@ -70,6 +84,10 @@ static void update_callback(struct diff_queue_struct *q,
 			}
 			break;
 		case DIFF_STATUS_DELETED:
+			if (data->warn_add_would_remove) {
+				warn_add_would_remove(path);
+				data->warn_add_would_remove = 0;
+			}
 			if (data->flags & ADD_CACHE_IGNORE_REMOVAL)
 				break;
 			if (!(data->flags & ADD_CACHE_PRETEND))
@@ -81,20 +99,27 @@ static void update_callback(struct diff_queue_struct *q,
 	}
 }
 
-int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
+static void update_files_in_cache(const char *prefix, const char **pathspec,
+				  struct update_callback_data *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_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;
+
+	memset(&data, 0, sizeof(data));
+	data.flags = flags;
+	update_files_in_cache(prefix, pathspec, &data);
 	return !!data.add_errors;
 }
 
@@ -354,18 +379,6 @@ static void warn_pathless_add(const char *option_name, const char *short_name) {
 		option_name, short_name);
 }
 
-static int directory_given(int argc, const char **argv)
-{
-	struct stat st;
-
-	while (argc--) {
-		if (!lstat(*argv, &st) && S_ISDIR(st.st_mode))
-			return 1;
-		argv++;
-	}
-	return 0;
-}
-
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
 	int exit_status = 0;
@@ -378,6 +391,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	char *seen = NULL;
 	const char *option_with_implicit_dot = NULL;
 	const char *short_option_with_implicit_dot = NULL;
+	struct update_callback_data update_data;
 
 	git_config(add_config, NULL);
 
@@ -403,15 +417,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	/*
 	 * Warn when "git add pathspec..." was given without "-u" or "-A"
-	 * and pathspec... contains a directory name.
+	 * and pathspec... covers a removed path.
 	 */
-	if (!take_worktree_changes && addremove_explicit < 0 &&
-	    directory_given(argc, argv))
-		warning(_("In Git 2.0, 'git add <pathspec>...' will also update the\n"
-			  "index for paths removed from the working tree that match\n"
-			  "the given pathspec. If you want to 'add' only changed\n"
-			  "or newly created paths, say 'git add --no-all <pathspec>...'"
-			  " instead."));
+	memset(&update_data, 0, sizeof(update_data));
+	if (!take_worktree_changes && addremove_explicit < 0)
+		update_data.warn_add_would_remove = 1;
 
 	if (!take_worktree_changes && addremove_explicit < 0 && argc)
 		/*
@@ -508,8 +518,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	plug_bulk_checkin();
 
-	exit_status |= add_files_to_cache(prefix, pathspec, flags);
+	update_data.flags = flags;
+	update_files_in_cache(prefix, pathspec, &update_data);
 
+	exit_status |= !!update_data.add_errors;
 	if (add_new_files)
 		exit_status |= add_files(&dir, flags);
 
-- 
1.8.2.1-552-g964983e

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

From: Jeff King <hidden>
Date: 2016-06-15 22:56:54 

On Wed, Apr 17, 2013 at 06:39:06PM -0700, Junio C Hamano wrote:

Subject: [PATCH] git add: rework the logic to warn "git add <pathspec>..." default change

[...]

Rework the logic to detect the case where the behaviour will be
different in Git 2.0, and issue a warning only when it matters.
Even with the code before this warning, "git add subdir" will have
to traverse the directory in order to find _new_ files the index
does not know about _anyway_, so we can do this check without adding
an extra pass to find if <pathspec> matches any removed file.
Thanks, I think this is looking much better.

A few minor nits on the message itself:

+static void warn_add_would_remove(const char *path)
+{
+	warning(_("In Git 2.0, 'git add <pathspec>...' will also update the\n"
+		  "index for paths removed from the working tree that match\n"
+		  "the given pathspec. If you want to 'add' only changed\n"
+		  "or newly created paths, say 'git add --no-all <pathspec>...'"
+		  " instead.\n\n"
This wrapping looks funny in the actual output, due to the extra
"warning:" and the lack of newline before "instead":

  warning: In Git 2.0, 'git add <pathspec>...' will also update the
  index for paths removed from the working tree that match
  the given pathspec. If you want to 'add' only changed
  or newly created paths, say 'git add --no-all <pathspec>...' instead.

Wrapping it like this ends up with a much more natural-looking
paragraph:

  warning: In Git 2.0, 'git add <pathspec>...' will also update the
  index for paths removed from the working tree that match the given
  pathspec. If you want to 'add' only changed or newly created paths,
  say 'git add --no-all <pathspec>...' instead.

+		  "'%s' would be removed from the index without --no-all."),
+		path);
I think mentioning the filename is a good thing; the original message
left me scratching my head and wondering "so, did you add it or not?".
I still think your "would be" is unnecessarily confusing, though. It is
"would be in Git 2.0 without --no-all, but we did not now". Which makes
sense when you think about it, but it took me a moment to parse.

Perhaps we can be more direct with something like:

  warning: did not stage removal of 'foo'

or perhaps the present tense "not staging removal of..." would be
better.

I also think it makes sense to show every path that is affected, not
just the first one, to be clear about what was done (and what _would_
have been done in Git 2.0).

A patch with all of the suggestions together is below. I still think the
multi-line warning block looks ugly. I kind of like the way advise()
puts "hint:" on each line. I wonder if we should do the same here.

diff --git a/builtin/add.c b/builtin/add.c
index 4242bce..aae550a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -52,15 +52,19 @@ static void warn_add_would_remove(const char *path)
 		return DIFF_STATUS_MODIFIED;
 }
 
+static const char *add_would_remove_warning = N_(
+/* indent for "warning: " */
+         "In Git 2.0, 'git add <pathspec>...' will also update the\n"
+"index for paths removed from the working tree that match the given\n"
+"pathspec. If you want to 'add' only changed or newly created paths,\n"
+"say 'git add --no-all <pathspec>...' instead.\n");
+
 static void warn_add_would_remove(const char *path)
 {
-	warning(_("In Git 2.0, 'git add <pathspec>...' will also update the\n"
-		  "index for paths removed from the working tree that match\n"
-		  "the given pathspec. If you want to 'add' only changed\n"
-		  "or newly created paths, say 'git add --no-all <pathspec>...'"
-		  " instead.\n\n"
-		  "'%s' would be removed from the index without --no-all."),
-		path);
+	static int warned_once;
+	if (!warned_once++)
+		warning(_(add_would_remove_warning));
+	warning("did not stage removal of '%s'", path);
 }
 
 static void update_callback(struct diff_queue_struct *q,
@@ -84,10 +88,8 @@ static void update_callback(struct diff_queue_struct *q,
 			}
 			break;
 		case DIFF_STATUS_DELETED:
-			if (data->warn_add_would_remove) {
+			if (data->warn_add_would_remove)
 				warn_add_would_remove(path);
-				data->warn_add_would_remove = 0;
-			}
 			if (data->flags & ADD_CACHE_IGNORE_REMOVAL)
 				break;
 			if (!(data->flags & ADD_CACHE_PRETEND))
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help