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

From: Jeff King <peff@peff.net>
Date: Apr 18, 2013, 17:27 UTC
Message-ID: <20130418172714.GA24690@sigill.intra.peff.net>
In-Reply-To: <7va9owd3d1.fsf@alter.siamese.dyndns.org>

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

Show 10 quoted lines

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

Show 7 quoted lines

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