bug: git-add --patch any negative pathspec fails

bug: git-add --patch any negative pathspec fails

[Erik Cervin Edin]

Steps to reproduce:
git-add -p -- ':!foo'

Expected:
Add everything that doesn't match the pathspec.

Actual:
fatal: empty string is not a valid pathspec. please use . instead if
you meant to match all paths
Cannot close git diff-index --cached --numstat --summary HEAD --
:(exclude,prefix:0)bar  () at C:/Program
Files/Git/mingw64/libexec/git-core\git-add--interactive line 242.

Relevant trace:
19:44:00.359141 run-command.c:667       trace: run_command: git
add--interactive --patch -- ':(exclude,prefix:0)bar' ''
Note the added empty string pathspec

Affects:
git version 2.31.1.windows.1

Re: bug: git-add --patch any negative pathspec fails

[Jeff King]

On Thu, Dec 16, 2021 at 08:09:24PM +0100, Erik Cervin Edin wrote:

> Steps to reproduce:
> git-add -p -- ':!foo'
> 
> Expected:
> Add everything that doesn't match the pathspec.
> 
> Actual:
> fatal: empty string is not a valid pathspec. please use . instead if
> you meant to match all paths

Hmm. This is a regression from 728c573803 (Merge branch
'ex/deprecate-empty-pathspec-as-match-all', 2017-11-06). But I think the
fault is really in the earlier 859b7f1d0e (pathspec: don't error out on
all-exclusionary pathspec patterns, 2017-02-07).

It works by adding an extra "match everything" pathspec internally, but
it just passes the empty string as the "original" string to
init_pathspec_item(). And "git add -p" works by iterating over the
parsed pathspec list and adding the result to the helper script's argv.

So either:

  - it should instead make sure to pass the true original set of
    pathspecs we got (so just the exclusion, not the "match everything"
    one). This would work because those pathspecs are then interpreted
    by further Git programs anyway (so they'd have their own "match
    everything" logic internally).

  - we should pass something better for the "match everything" pathspec.
    The patch below does that, though I didn't think too hard on whether
    the choice of "something better" would always be correct (e.g., if
    we change directory internally, do we need to adjust "." to match
    original cwd)?

    It does fix your case, and it passes the test suite, but I have a
    feeling this area is not very well tested (I doubt we even look at
    the "original" string all that often).

diff --git a/pathspec.c b/pathspec.c
index ddeeba7911..5a0e083229 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -628,8 +628,16 @@ void parse_pathspec(struct pathspec *pathspec,
 	 * that matches everything. We allocated an extra one for this.
 	 */
 	if (nr_exclude == n) {
-		int plen = (!(flags & PATHSPEC_PREFER_CWD)) ? 0 : prefixlen;
-		init_pathspec_item(item + n, 0, prefix, plen, "");
+		int plen;
+		const char *orig;
+		if (flags & PATHSPEC_PREFER_CWD) {
+			plen = prefixlen;
+			orig = ".";
+		} else {
+			plen = 0;
+			orig = ":/";
+		}
+		init_pathspec_item(item + n, 0, prefix, plen, orig);
 		pathspec->nr++;
 	}