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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html