Re: [PATCH v2 01/14] dir.c: git-status --ignored: don't drop ignored directories
From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:56:52
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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.
+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.
+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.