[2/8] Revert "dir.c: make 'git-status --ignored' work within leading directories"
diff mbox series

Message ID bfaf7592ee611f7e9f6fbcf7b1e4ae32f8307548.1575924465.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • Directory traversal bugs
Related show

Commit Message

Johannes Schindelin via GitGitGadget Dec. 9, 2019, 8:47 p.m. UTC
From: Elijah Newren <newren@gmail.com>

Commit be8a84c52669 ("dir.c: make 'git-status --ignored' work within
leading directories", 2013-04-15) noted that
   git status --ignored <SOMEPATH>
would not list ignored files and directories within <SOMEPATH> if
<SOMEPATH> was untracked, and modified the behavior to make it show
them.  However, it did so via a hack that broke consistency; it would
show paths under <SOMEPATH> differently than a simple
   git status --ignored | grep <SOMEPATH>
would show them.  A correct fix is slightly more involved, and
complicated slightly by this hack, so we revert this commit (but keep
corrected versions of the testcases) and will later fix the original
bug with a subsequent patch.

Some history may be helpful:

A very, very similar case to the commit we are reverting was raised in
commit 48ffef966c76 ("ls-files: fix overeager pathspec optimization",
2010-01-08); but it actually went in somewhat the opposite direction.  In
that commit, it mentioned how
   git ls-files -o --exclude-standard t/
used to show untracked files under t/ even when t/ was ignored, and then
changed the behavior to stop showing untracked files under an ignored
directory.  More importantly, this commit considered keeping this
behavior but noted that it would be inconsistent with the behavior when
multiple pathspecs were specified and thus rejected it.

The reason for this whole inconsistency when one pathspec is specified
versus zero or two is because common prefixes of pathspecs are sent
through a different set of checks (in treat_leading_path()) than normal
file/directory traversal (those go through read_directory_recursive()
and treat_path()).  As such, for consistency, one needs to check that
both codepaths produce the same result.

Revert commit be8a84c526691667fc04a8241d93a3de1de298ab, except instead
of removing the testcase it added, modify it to check for correct and
consistent behavior.  A subsequent patch in this series will fix the
testcase.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 dir.c                      | 3 ---
 t/t7061-wtstatus-ignore.sh | 7 +++++--
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Denton Liu Dec. 9, 2019, 9:32 p.m. UTC | #1
Hi Elijah,

On Mon, Dec 09, 2019 at 08:47:39PM +0000, Elijah Newren via GitGitGadget wrote:
> diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
> index 0c394cf995..ded7f97181 100755
> --- a/t/t7061-wtstatus-ignore.sh
> +++ b/t/t7061-wtstatus-ignore.sh
> @@ -43,11 +43,14 @@ test_expect_success 'status untracked directory with --ignored -u' '
>  	test_cmp expected actual
>  '
>  cat >expected <<\EOF
> -?? untracked/uncommitted
> +?? untracked/
>  !! untracked/ignored
>  EOF
>  
> -test_expect_success 'status prefixed untracked directory with --ignored' '
> +test_expect_failure 'status of untracked directory with --ignored works with or without prefix' '
> +	git status --porcelain --ignored | grep untracked/ >actual &&

Can we break this pipe up into two invocations so that we don't have a
git command in the upstream of a pipe?

Thanks,

Denton

P.S. Perhaps in the future, we (I) could try to extend chainlint so that
it catches this and git commands in non-assignment command
substitutions... I think that would be pretty nice.

> +	test_cmp expected actual &&
> +
>  	git status --porcelain --ignored untracked/ >actual &&
>  	test_cmp expected actual
>  '
> -- 
> gitgitgadget
>
Elijah Newren Dec. 9, 2019, 9:51 p.m. UTC | #2
On Mon, Dec 9, 2019 at 1:32 PM Denton Liu <liu.denton@gmail.com> wrote:
>
> Hi Elijah,
>
> On Mon, Dec 09, 2019 at 08:47:39PM +0000, Elijah Newren via GitGitGadget wrote:
> > diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
> > index 0c394cf995..ded7f97181 100755
> > --- a/t/t7061-wtstatus-ignore.sh
> > +++ b/t/t7061-wtstatus-ignore.sh
> > @@ -43,11 +43,14 @@ test_expect_success 'status untracked directory with --ignored -u' '
> >       test_cmp expected actual
> >  '
> >  cat >expected <<\EOF
> > -?? untracked/uncommitted
> > +?? untracked/
> >  !! untracked/ignored
> >  EOF
> >
> > -test_expect_success 'status prefixed untracked directory with --ignored' '
> > +test_expect_failure 'status of untracked directory with --ignored works with or without prefix' '
> > +     git status --porcelain --ignored | grep untracked/ >actual &&
>
> Can we break this pipe up into two invocations so that we don't have a
> git command in the upstream of a pipe?

Sigh...yeah, I keep doing this.  And I'll probably keep doing it if
someone can't chainlint (or pipefail) it.  I'll fix it up.
Eric Sunshine Dec. 9, 2019, 10:09 p.m. UTC | #3
On Mon, Dec 9, 2019 at 4:32 PM Denton Liu <liu.denton@gmail.com> wrote:
> On Mon, Dec 09, 2019 at 08:47:39PM +0000, Elijah Newren via GitGitGadget wrote:
> > +test_expect_failure 'status of untracked directory with --ignored works with or without prefix' '
> > +     git status --porcelain --ignored | grep untracked/ >actual &&
>
> Can we break this pipe up into two invocations so that we don't have a
> git command in the upstream of a pipe?
>
> P.S. Perhaps in the future, we (I) could try to extend chainlint so that
> it catches this and git commands in non-assignment command
> substitutions... I think that would be pretty nice.

Rather than getting mired down in chainlint (which could make your
eyeballs melt), an easier way to catch this sort of thing would be to
introduce a new script which checks test scripts for Git
best-practices non-conformity, similar to how
t/check-non-portable-shell.pl checks for non-portable shell
constructs. (You could even extend check-non-portable-shell.pl with
the functionality, but then the script would no longer be specific to
"non-portable shell", so either renaming it or making a new new script
is warranted.)

By the way, I have considered adding a best-practices linting script
like this, but it (at least at first) would need to have some sort of
opt-in or opt-out feature since there (likely) are still so many
instances of tests which don't follow best-practices, and it could
take a while to "fix" them all (and eat up a lot of reviewer time, so
it should be done in small batches).

Patch
diff mbox series

diff --git a/dir.c b/dir.c
index 61f559f980..0dd5266629 100644
--- a/dir.c
+++ b/dir.c
@@ -2083,14 +2083,12 @@  static int treat_leading_path(struct dir_struct *dir,
 	struct strbuf sb = STRBUF_INIT;
 	int baselen, rc = 0;
 	const char *cp;
-	int old_flags = dir->flags;
 
 	while (len && path[len - 1] == '/')
 		len--;
 	if (!len)
 		return 1;
 	baselen = 0;
-	dir->flags &= ~DIR_SHOW_OTHER_DIRECTORIES;
 	while (1) {
 		cp = path + baselen + !!baselen;
 		cp = memchr(cp, '/', path + len - cp);
@@ -2113,7 +2111,6 @@  static int treat_leading_path(struct dir_struct *dir,
 		}
 	}
 	strbuf_release(&sb);
-	dir->flags = old_flags;
 	return rc;
 }
 
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 0c394cf995..ded7f97181 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -43,11 +43,14 @@  test_expect_success 'status untracked directory with --ignored -u' '
 	test_cmp expected actual
 '
 cat >expected <<\EOF
-?? untracked/uncommitted
+?? untracked/
 !! untracked/ignored
 EOF
 
-test_expect_success 'status prefixed untracked directory with --ignored' '
+test_expect_failure 'status of untracked directory with --ignored works with or without prefix' '
+	git status --porcelain --ignored | grep untracked/ >actual &&
+	test_cmp expected actual &&
+
 	git status --porcelain --ignored untracked/ >actual &&
 	test_cmp expected actual
 '