Re: [PATCH v2 03/14] dir.c: git-status --ignored: don't list empty ignored directories

Am 16.04.2013 19:48, schrieb Ramkumar Ramachandra:

Karsten Blees wrote:

quoted

'git-status --ignored' lists ignored tracked directories without any
ignored files if a tracked file happens to match an exclude pattern.

Here, I have:

    quux/
        bar
        baz/
            foo

So, quux is an ignored tracked directory.  bar is tracked, but matches
an ignore pattern.  Currently, git status --ignored lists quux/.  I'm
confused.

The crucial part is 'without any ignored files'. So if you 'rm quux/baz/foo' or 'git-add -f quux/baz/foo', you get this:

1) git-status --ignored -uall: <empty>
2) git-status --ignored -unormal: quux/

In case 2), quux/ is listed as ignored even though there are no ignored files and the DIR_HIDE_EMPTY_DIRECTORIES flag should kick in.

quoted

Always exclude tracked files.

"exclude" it from the 'git status --ignored' output, I presume?
There's already an _exclude_ pattern in your previous sentence, so you
can see why the reader might be confused about what you're talking
about.

I see, but "Always *ignore* tracked files" is also ambiguous. Suggestions?

quoted

diff --git a/dir.c b/dir.c
index 7c9bc9c..fd1f088 100644
--- a/dir.c
+++ b/dir.c

@@ -1109,16 +1109,13 @@ static int treat_file(struct dir_struct *dir, struct strbuf *path, int exclude,
        struct path_exclude_check check;
        int exclude_file = 0;

+       /* Always exclude indexed files */
+       if (index_name_exists(&the_index, path->buf, path->len, ignore_case))
+               return 1;
+
        if (exclude)
                exclude_file = !(dir->flags & DIR_SHOW_IGNORED);
        else if (dir->flags & DIR_SHOW_IGNORED) {
-               /* Always exclude indexed files */
-               struct cache_entry *ce = index_name_exists(&the_index,
-                   path->buf, path->len, ignore_case);
-
-               if (ce)
-                       return 1;
-

Okay, so you just moved this segment outside the else if()
conditional.  Can you explain what the old logic was doing, and what
the rationale for it was?

Quoting Antoine's patch from January:
+		/*
+		 * Optimization:
+		 * Don't spend time on indexed files, they won't be
+		 * added to the list anyway
+		 */

In other words: don't do the (expensive) recursive is_path_excluded check if the file will be dropped later anyway (see the additional 'cache_name_is_other' checks in wt_status.c::wt_status_collect_untracked and builtin/ls-files.c::show_other_files).

In the DIR_SHOW_OTHER_DIRECTORIES case, we use read_directory_recursive to just *check* for ignored / untracked files, all within dir.c. So the results must be correct *here*, we cannot rely on some other modules to fix up incorrect files later.

quoted

diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 4ece129..28b7d95 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh

@@ -122,10 +122,34 @@ cat >expected <<\EOF
 ?? .gitignore
 ?? actual
 ?? expected
+EOF
+
+test_expect_success 'status ignored tracked directory and ignored file with --ignore' '
+       echo "committed" >>.gitignore &&
+       git status --porcelain --ignored >actual &&
+       test_cmp expected actual
+'

Um, didn't really get this one.  You have three untracked files, and
git status seems to be showing them fine.  What am I missing?

We have this:

	tracked/
		committed

.gitignore:
tracked
committed

Both the directory and tracked file match an exclude pattern. There are no untracked ignored files, yet before this patch, git-status --ignored would list tracked/ as ignored directory.

quoted

+cat >expected <<\EOF
+?? .gitignore
+?? actual
+?? expected
+EOF
+
+test_expect_success 'status ignored tracked directory and ignored file with --ignore -u' '
+       git status --porcelain --ignored -u >actual &&
+       test_cmp expected actual
+'

I didn't understand why you're invoking -u here (doesn't it imply
"all", as opposed to "normal" when unspecified?).  There are really no
directories, so I don't know what I'm expected to see.

quoted

+cat >expected <<\EOF
+?? .gitignore
+?? actual
+?? expected
 !! tracked/
 EOF

 test_expect_success 'status ignored tracked directory and uncommitted file with --ignore' '
+       echo "tracked" >.gitignore &&
        : >tracked/uncommitted &&
        git status --porcelain --ignored >actual &&
        test_cmp expected actual

Didn't we test this in the last patch?  Okay, I'm completely confused now.

'echo "tracked" >.gitignore' reverts the 'echo "committed" >>.gitignore' from above.