[PATCH] dir: check pathspecs before returning `path_excluded`

["Martin Ågren" <martin.agren@gmail.com>]

On Mon, 20 Jul 2020 at 17:25, Elijah Newren <newren@gmail.com> wrote:
>
> Awesome, thanks Andreas for the bisected report and Martin for finding
> and fixing the bug.  As for the reason that the old patch injected the
> pathspec check between the path_excluded and the path_untracked cases,
> that appears to me to just be "I'm good at making boneheaded
> mistakes".  Your changes here are the right fix.

Ok, here it is as a proper patch.

> Reviewed-by: Elijah Newren <newren@gmail.com>

Thanks. I've included your reviewed-by below. The log message is
obviously new, but the diff is identical to what I posted earlier.

BTW, this bug first appeared in v2.27.0, so this is not a regression
during the v2.28.0 cycle.

Martin

-- >8 --
In 95c11ecc73 ("Fix error-prone fill_directory() API; make it only
return matches", 2020-04-01), we taught `fill_directory()`, or more
specifically `treat_path()`, to check against any pathspecs so that we
could simplify the callers.

But in doing so, we added a slightly-to-early return for the "excluded"
case. We end up not checking the pathspecs, meaning we return
`path_excluded` when maybe we should return `path_none`. As a result,
`git status --ignored -- pathspec` might show paths that don't actually
match "pathspec".

Move the "excluded" check down to after we've checked any pathspecs.

Reported-by: Andreas Schwab <schwab@linux-m68k.org>
Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 dir.c                      |  4 ++--
 t/t7061-wtstatus-ignore.sh | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index 1045cc9c6f..fe64be30ed 100644
--- a/dir.c
+++ b/dir.c
@@ -2209,13 +2209,13 @@ static enum path_treatment treat_path(struct dir_struct *dir,
 				       baselen, excluded, pathspec);
 	case DT_REG:
 	case DT_LNK:
-		if (excluded)
-			return path_excluded;
 		if (pathspec &&
 		    !match_pathspec(istate, pathspec, path->buf, path->len,
 				    0 /* prefix */, NULL /* seen */,
 				    0 /* is_dir */))
 			return path_none;
+		if (excluded)
+			return path_excluded;
 		return path_untracked;
 	}
 }
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index e4cf5484f9..2f9bea9793 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -30,6 +30,31 @@ test_expect_success 'same with gitignore starting with BOM' '
 	test_cmp expected actual
 '
 
+test_expect_success 'status untracked files --ignored with pathspec (no match)' '
+	git status --porcelain --ignored -- untracked/i >actual &&
+	test_must_be_empty actual &&
+	git status --porcelain --ignored -- untracked/u >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'status untracked files --ignored with pathspec (literal match)' '
+	git status --porcelain --ignored -- untracked/ignored >actual &&
+	echo "!! untracked/ignored" >expected &&
+	test_cmp expected actual &&
+	git status --porcelain --ignored -- untracked/uncommitted >actual &&
+	echo "?? untracked/uncommitted" >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'status untracked files --ignored with pathspec (glob match)' '
+	git status --porcelain --ignored -- untracked/i\* >actual &&
+	echo "!! untracked/ignored" >expected &&
+	test_cmp expected actual &&
+	git status --porcelain --ignored -- untracked/u\* >actual &&
+	echo "?? untracked/uncommitted" >expected &&
+	test_cmp expected actual
+'
+
 cat >expected <<\EOF
 ?? .gitignore
 ?? actual
-- 
2.28.0.rc1