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

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Apr 16, 2013, 17:48 UTC
Message-ID: <CALkWK0mG6bzWu7dqdP1mBe+AZfUDK=Mx4+ZkDOc2saXr3gBsUQ@mail.gmail.com>
In-Reply-To: <516C4FE4.9090806@gmail.com>

Karsten Blees wrote:

> '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.

> 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.

Show 22 quoted lines

> 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?

Show 15 quoted lines

> 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?

Show 10 quoted lines

> +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.

Show 12 quoted lines

> +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.

Once again, apologies for my inexperienced comments: I'm contributing whatever little I can to the review process.