Re: [PATCH v2 03/14] dir.c: git-status --ignored: don't list empty ignored directories

2013-04-16 Thread Karsten Blees
Am 16.04.2013 19:48, schrieb Ramkumar Ramachandra:
> Karsten Blees wrote:
>> 'git-status --ignored' lists ignored tracked directories without any
>> ignored files if a tracked file happens to match an exclude pattern.
> 
> Here, I have:
> 
> quux/
> bar
> baz/
> foo
> 
> So, quux is an ignored tracked directory.  bar is tracked, but matches
> an ignore pattern.  Currently, git status --ignored lists quux/.  I'm
> confused.
> 

The crucial part is 'without any ignored files'. So if you 'rm quux/baz/foo' or 
'git-add -f quux/baz/foo', you get this:

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

In case 2), quux/ is listed as ignored even though there are no ignored files 
and the DIR_HIDE_EMPTY_DIRECTORIES flag should kick in.

>> Always exclude tracked files.
> 
> "exclude" it from the 'git status --ignored' output, I presume?
> There's already an _exclude_ pattern in your previous sentence, so you
> can see why the reader might be confused about what you're talking
> about.
> 

I see, but "Always *ignore* tracked files" is also ambiguous. Suggestions?

>> diff --git a/dir.c b/dir.c
>> index 7c9bc9c..fd1f088 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1109,16 +1109,13 @@ static int treat_file(struct dir_struct *dir, struct 
>> strbuf *path, int exclude,
>> struct path_exclude_check check;
>> int exclude_file = 0;
>>
>> +   /* Always exclude indexed files */
>> +   if (index_name_exists(&the_index, path->buf, path->len, ignore_case))
>> +   return 1;
>> +
>> if (exclude)
>> exclude_file = !(dir->flags & DIR_SHOW_IGNORED);
>> else if (dir->flags & DIR_SHOW_IGNORED) {
>> -   /* Always exclude indexed files */
>> -   struct cache_entry *ce = index_name_exists(&the_index,
>> -   path->buf, path->len, ignore_case);
>> -
>> -   if (ce)
>> -   return 1;
>> -
> 
> Okay, so you just moved this segment outside the else if()
> conditional.  Can you explain what the old logic was doing, and what
> the rationale for it was?
> 

Quoting Antoine's patch from January:
+   /*
+* Optimization:
+* Don't spend time on indexed files, they won't be
+* added to the list anyway
+*/

In other words: don't do the (expensive) recursive is_path_excluded check if 
the file will be dropped later anyway (see the additional 'cache_name_is_other' 
checks in wt_status.c::wt_status_collect_untracked and 
builtin/ls-files.c::show_other_files).

In the DIR_SHOW_OTHER_DIRECTORIES case, we use read_directory_recursive to just 
*check* for ignored / untracked files, all within dir.c. So the results must be 
correct *here*, we cannot rely on some other modules to fix up incorrect files 
later.

>> diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
>> index 4ece129..28b7d95 100755
>> --- a/t/t7061-wtstatus-ignore.sh
>> +++ b/t/t7061-wtstatus-ignore.sh
>> @@ -122,10 +122,34 @@ cat >expected <<\EOF
>>  ?? .gitignore
>>  ?? actual
>>  ?? expected
>> +EOF
>> +
>> +test_expect_success 'status ignored tracked directory and ignored file with 
>> --ignore' '
>> +   echo "committed" >>.gitignore &&
>> +   git status --porcelain --ignored >actual &&
>> +   test_cmp expected actual
>> +'
> 
> Um, didn't really get this one.  You have three untracked files, and
> git status seems to be showing them fine.  What am I missing?
> 

We have this:

tracked/
committed

.gitignore:
tracked
committed

Both the directory and tracked file match an exclude pattern. There are no 
untracked ignored files, yet before this patch, git-status --ignored would list 
tracked/ as ignored directory.

>> +cat >expected <<\EOF
>> +?? .gitignore
>> +?? actual
>> +?? expected
>> +EOF
>> +
>> +test_expect_success 'status ignored tracked directory and ignored file with 
>> --ignore -u' '
>> +   git status --porcelain --ignored -u >actual &&
>> +   test_cmp expected actual
>> +'
> 
> I didn't understand why you're invoking -u here (doesn't it imply
> "all", as opposed to "normal" when unspecified?).  There are really no
> directories, so I don't know what I'm expected to see.
> 
>> +cat >expected <<\EOF
>> +?? .gitignore
>> +?? actual
>> +?? expected
>>  !! tracked/
>>  EOF
>>
>>  test_expect_success 'status ignored tracked directory and uncommitted file 
>> with --ignore' '
>> +   echo "tracked" >.gitignore &&
>> : >tracked/uncommitted &&
>> git status --porcelain --ignored >actual &&
>> test_cmp expected actual
> 
> Didn't we test this in the last patch?  Okay, I'm completely confused now.
> 

'echo "tracked" >.gitignore' reverts the 'echo "committed" >>.gitignore' from 
above.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vge

Re: [PATCH v2 03/14] dir.c: git-status --ignored: don't list empty ignored directories

2013-04-16 Thread Ramkumar Ramachandra
Karsten Blees wrote:
> 'git-status --ignored' lists ignored tracked directories without any
> ignored files if a tracked file happens to match an exclude pattern.

Here, I have:

quux/
bar
baz/
foo

So, quux is an ignored tracked directory.  bar is tracked, but matches
an ignore pattern.  Currently, git status --ignored lists quux/.  I'm
confused.

> Always exclude tracked files.

"exclude" it from the 'git status --ignored' output, I presume?
There's already an _exclude_ pattern in your previous sentence, so you
can see why the reader might be confused about what you're talking
about.

> diff --git a/dir.c b/dir.c
> index 7c9bc9c..fd1f088 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1109,16 +1109,13 @@ static int treat_file(struct dir_struct *dir, struct 
> strbuf *path, int exclude,
> struct path_exclude_check check;
> int exclude_file = 0;
>
> +   /* Always exclude indexed files */
> +   if (index_name_exists(&the_index, path->buf, path->len, ignore_case))
> +   return 1;
> +
> if (exclude)
> exclude_file = !(dir->flags & DIR_SHOW_IGNORED);
> else if (dir->flags & DIR_SHOW_IGNORED) {
> -   /* Always exclude indexed files */
> -   struct cache_entry *ce = index_name_exists(&the_index,
> -   path->buf, path->len, ignore_case);
> -
> -   if (ce)
> -   return 1;
> -

Okay, so you just moved this segment outside the else if()
conditional.  Can you explain what the old logic was doing, and what
the rationale for it was?

> diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
> index 4ece129..28b7d95 100755
> --- a/t/t7061-wtstatus-ignore.sh
> +++ b/t/t7061-wtstatus-ignore.sh
> @@ -122,10 +122,34 @@ cat >expected <<\EOF
>  ?? .gitignore
>  ?? actual
>  ?? expected
> +EOF
> +
> +test_expect_success 'status ignored tracked directory and ignored file with 
> --ignore' '
> +   echo "committed" >>.gitignore &&
> +   git status --porcelain --ignored >actual &&
> +   test_cmp expected actual
> +'

Um, didn't really get this one.  You have three untracked files, and
git status seems to be showing them fine.  What am I missing?

> +cat >expected <<\EOF
> +?? .gitignore
> +?? actual
> +?? expected
> +EOF
> +
> +test_expect_success 'status ignored tracked directory and ignored file with 
> --ignore -u' '
> +   git status --porcelain --ignored -u >actual &&
> +   test_cmp expected actual
> +'

I didn't understand why you're invoking -u here (doesn't it imply
"all", as opposed to "normal" when unspecified?).  There are really no
directories, so I don't know what I'm expected to see.

> +cat >expected <<\EOF
> +?? .gitignore
> +?? actual
> +?? expected
>  !! tracked/
>  EOF
>
>  test_expect_success 'status ignored tracked directory and uncommitted file 
> with --ignore' '
> +   echo "tracked" >.gitignore &&
> : >tracked/uncommitted &&
> git status --porcelain --ignored >actual &&
> test_cmp expected actual

Didn't we test this in the last patch?  Okay, I'm completely confused now.

Once again, apologies for my inexperienced comments:  I'm contributing
whatever little I can to the review process.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html