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

[Karsten Blees <karsten.blees@gmail.com>]

Am 16.04.2013 19:33, schrieb Ramkumar Ramachandra:
> 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

IIRC i mentioned untracked subdirectory explicitly because the problem doesn't occur if the subdirectory is tracked (i.e. contains a tracked file).

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

With your example, you get this:

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

Without the untracked directory 'baz' between 'quux' and 'foo':

	quux/
		bar
		foo

you get this:

3) git status --ignored -uall: quux/foo
4) git status --ignored -unormal: quux/

The bug is in case (2), this should be:

2) git-status --ignored -unormal: quux/

I.e. ignored directories with untracked files in untracked sub directories should be listed as ignored, because they contain ignored files. Currently these directories are not listed. I realize this sounds pretty much like the original commit message, but I don't know how to describe it any better.

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

Both notations (as well as just 'status') seem to be widely used. Does this justify a reroll of the entire series with changed commit messages (I believe I've used git-status / git-ls-files / git-clean consistently in all patches)?

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

This simply does a recursive excluded check via is_path_excluded, because is_excluded is not good enough. The rest is just boilerplate code (i.e. initialize and clear the check structure and passing a dtype == DT_DIR because dirname obviously is a directory here). We already have similar code in treat_file (and all other places that use is_path_excluded).

> Sorry that I'm not able to do a more thorough review.
> 
>> 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.
> 

$ grep -e '^cat .*EOF$' t*.sh | wc -l
    743
$ grep -e '^    cat .*EOF$' t*.sh | wc -l
     22

Setting up expected results outside test_expect_* (i.e. unindented) seems to be the norm in git tests.

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

This test expands on the previous test 'status ignored tracked directory and uncommitted file with --ignore', i.e. the test just adds the 'in untracked subdir' part (and thus moves the 'uncommitted' file into a sub directory). Expanding on previous state is the whole point of putting several tests in a single t*.sh file, right? If I wanted to set up the test fixture from scratch I would have started a new t7063-whatever.sh.

> Yeah, changes to tracked ignored directories are not shown, but the
> commit message didn't tell me this.
> 
>> +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.
> 

Note that all tests in t7061 come in pairs '--ignored' (i.e. --untracked-files=normal) and '--ignored -u' (i.e. --untracked-files=all). If '-uall' lists individual ignored files, then '-unormal' should list the ignored directory.