Re: [PATCH] pathspec: die on empty strings as pathspec

2 messages, 2 authors, 2017-06-10 · open the first message on its own page

Re: [PATCH] pathspec: die on empty strings as pathspec

From: Junio C Hamano <hidden>
Date: 2017-06-09 16:29:01

Emily Xie [off-list ref] writes:

An empty string as a pathspec element matches all paths.  A buggy
script, however, could accidentally assign an empty string to a
variable that then gets passed to a Git command invocation, e.g.:

  path=... compute a path to be removed in $path ...
        git rm -r "$path"

which would unintentionally remove all paths in the current
directory.

The fix for this issue comprises of two steps. Step 1, which warns
that empty strings as pathspecs will become invalid, has already
been implemented in commit d426430 ("pathspec: warn on empty strings
as pathspec", 2016-06-22).

This patch is step 2. It removes the warning and throws an error
instead.

Signed-off-by: Emily Xie <redacted>
Reported-by: David Turner <redacted>
---

We started this at v2.11.0 at the end of November 2016, and this
cycle is expected to complete at around the end of July 2017, so
this patch makes it a 8-month deprecation cycle.  I think that it
should be long enough.

Thanks.

quoted hunk

diff --git a/pathspec.c b/pathspec.c
index 50f76ff..65e18b1 100644
--- a/pathspec.c
+++ b/pathspec.c

@@ -638,12 +638,10 @@ void parse_pathspec(struct pathspec *pathspec,
 	}
 
 	n = 0;
-	warn_empty_string = 1;
 	while (argv[n]) {
-		if (*argv[n] == '\0' && warn_empty_string) {
-			warning(_("empty strings as pathspecs will be made invalid in upcoming releases. "
-				  "please use . instead if you meant to match all paths"));
-			warn_empty_string = 0;
+		if (*argv[n] == '\0') {
+			die("empty string is not a valid pathspec. "
+				  "please use . instead if you meant to match all paths");
 		}
 		n++;
 	}

The {} around a single statement becomes unnecessary, so I'll remove
them while queuing.

Re: [PATCH] pathspec: die on empty strings as pathspec

From: Jeff King <hidden>
Date: 2017-06-10 06:21:57

On Sat, Jun 10, 2017 at 01:28:54AM +0900, Junio C Hamano wrote:

Emily Xie [off-list ref] writes:

quoted

An empty string as a pathspec element matches all paths.  A buggy
script, however, could accidentally assign an empty string to a
variable that then gets passed to a Git command invocation, e.g.:

  path=... compute a path to be removed in $path ...
        git rm -r "$path"

which would unintentionally remove all paths in the current
directory.

The fix for this issue comprises of two steps. Step 1, which warns
that empty strings as pathspecs will become invalid, has already
been implemented in commit d426430 ("pathspec: warn on empty strings
as pathspec", 2016-06-22).

This patch is step 2. It removes the warning and throws an error
instead.

Signed-off-by: Emily Xie <redacted>
Reported-by: David Turner <redacted>
---

We started this at v2.11.0 at the end of November 2016, and this
cycle is expected to complete at around the end of July 2017, so
this patch makes it a 8-month deprecation cycle.  I think that it
should be long enough.

That's long enough for people who actually ran the intermediate
versions.  But what about people on distros who jump from v2.10 or lower
straight to v2.14?

I think to catch them we'd literally need years on our deprecation
schedules. Maybe it's not worth caring about. That's an awful long time
for people who _are_ upgrading each version to see the warning. Of
course the end game in this case is that it becomes an error, so they'll
be notified either way. :)

I'm OK with this case being a relatively quick deprecation, but your
"long enough" made me wonder how we would decide that.

-Peff

[1] Debian stable is set to release in a week or so. People doing that
    stable upgrade will go from v2.1.4 to v2.11.0. So they won't
    actually skip the deprecation notice, but they'll probably live with
    the warning for 3 years or so.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help