Re: [PATCH v2 01/14] dir.c: git-status --ignored: don't drop ignored directories

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Apr 16, 2013, 17:33 UTC
Message-ID: <CALkWK0=0MbKnEWc+uLkbynJZXYYiRyfJ539HjtPTajHotB4q9A@mail.gmail.com>
In-Reply-To: <516C4F6F.5020708@gmail.com>

Firstly, great work on the series! I've just started looking into it, so please don't take my comments too seriously: some of them may be queries, and others may be minor suggestions, but I can't say I understand the area you're patching. I know Junio doesn't like me mixing queries in reviews, but I don't fully agree with his policy.

Karsten Blees wrote:

> 'git-status --ignored' drops ignored directories if they contain untracked
> files in an untracked sub directory.

Wait, ignored directories will always contain untracked subdirectories, unless you add -f them, right? Why are you saying untracked files in an _untracked_ subdirectory? We don't track directories anyway, and I would call a directory "tracked" if there's atleast one file inside it is tracked. So, my understanding of this is:

    quux/
       bar
        baz/
             foo

In this example, if quux is ignored and untracked, git status --ignored currently shows quux/. If quux/bar is tracked (say with add -f), but baz/ is untracked, git status --ignored doesn't show me anything. What exactly is the bug you're fixing? I'll try to look at the tests to infer this, but your commit message could probably be clearer.

Nit: please s/git-status/git status/

> Fix it by getting exact (recursive) excluded status in treat_directory.

Okay, so you're patching treat_directory() in dir.c to do some recursive exclude handling. Let's see what this is.

Show 17 quoted lines

> diff --git a/dir.c b/dir.c
> index 57394e4..ec4eebf 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1060,6 +1060,15 @@ static enum directory_treatment treat_directory(struct dir_struct *dir,
>
>         /* This is the "show_other_directories" case */
>
> +       /* might be a sub directory in an excluded directory */
> +       if (!exclude) {
> +               struct path_exclude_check check;
> +               int dt = DT_DIR;
> +               path_exclude_check_init(&check, dir);
> +               exclude = is_path_excluded(&check, dirname, len, &dt);
> +               path_exclude_check_clear(&check);
> +       }
> +

So, I'm guessing that DT_DIR refers to a value that a field in struct dirent can take; that value could be one of DIR (directory), REG (regular file?), LNK (symbolic link?). I don't get much of this, but what I do get is that you're setting exclude for the rest of the code in this function.

Sorry that I'm not able to do a more thorough review.

Show 14 quoted lines

> diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
> index 0da1214..0f1034e 100755
> --- a/t/t7061-wtstatus-ignore.sh
> +++ b/t/t7061-wtstatus-ignore.sh
> @@ -143,4 +143,31 @@ test_expect_success 'status ignored tracked directory and uncommitted file with
>         test_cmp expected actual
>  '
>
> +cat >expected <<\EOF
> +?? .gitignore
> +?? actual
> +?? expected
> +!! tracked/
> +EOF

Please put these segments inside the test_expect_success block, so it's easy to think about those blocks in isolation. I know you're just following the existing conventions existing in this test, but those are not necessarily good conventions.

Show 7 quoted lines

> +test_expect_success 'status ignored tracked directory with uncommitted file in untracked subdir with --ignore' '
> +       rm -rf tracked/uncommitted &&
> +       mkdir tracked/ignored &&
> +       : >tracked/ignored/uncommitted &&
> +       git status --porcelain --ignored >actual &&
> +       test_cmp expected actual
> +'

This is very confusing. How is tracked a tracked directory? Oh, right: some previous test git add'ed tracked/committed. How do I know about that in this test?

Yeah, changes to tracked ignored directories are not shown, but the commit message didn't tell me this.

Show 13 quoted lines

> +cat >expected <<\EOF
> +?? .gitignore
> +?? actual
> +?? expected
> +!! tracked/ignored/uncommitted
> +EOF
> +
> +test_expect_success 'status ignored tracked directory with uncommitted file in untracked subdir with --ignore -u' '
> +       git status --porcelain --ignored -u >actual &&
> +       test_cmp expected actual
> +'
> +
>  test_done

I suppose the commit message told me about this one vaguely, but I think it could be much clearer overall.

Thanks.