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


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/ b/t/
>> index 0da1214..0f1034e 100755
>> --- a/t/
>> +++ b/t/
>> @@ -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
$ grep -e '^    cat .*EOF$' t*.sh | wc -l

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

> 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 

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to
More majordomo info at

Reply via email to