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