Re: [PATCH] status: report ignored yet tracked directories

2013-01-07 Thread Jeff King 
On Sun, Jan 06, 2013 at 05:40:46PM +0100, Antoine Pelisse wrote:

  Looking at your fix and remembering how the index hashing works, I think
  the answer is that:
 
1. This bug only affects directories, because they are the only thing
   that can be simultaneously ignored and untracked and tracked
   (i.e., they have entries of both, and we are using
   DIR_SHOW_OTHER_DIRECTORIES).
 
2. When core.ignorecase is false, the index name hash contains only
   the file entries, and cache_name_exists returns an exact match. So
   it doesn't matter if we make an extra check when adding the
   directory via dir_add_name; we know that it will not be there, and
   the final check is a no-op.
 
3. When core.ignorecase is true, we also store directory entries in
   the index name hash, and this extra check is harmful; the entry
   does not really exist in the index, and we still need to add it.
 
 Yes, because of this couple of lines I guess (name-hash.c, 
 hash_index_entry()):
 
   if (ignore_case)
 hash_index_entry_directories(istate, ce);

Exactly. I couldn't remember at first why this was the case, but after
reading 5102c61 (Add case insensitivity support for directories when
using git status, 2010-10-03) again, I think it is because we cannot do
a partial-name lookup via the hash (i.e., the hash for foo/ and
foo/bar have no relation to each other). Not related to your patch,
obviously, but it was the missing piece for me to understand why the
code was doing what it does.

  I think in the normal file case, we'd expect treat_path to just tell us
  that it is handled, and we would not ever call dir_add_name in the first
  place. But what if we have an index entry for a file, but the working
  tree now contains a directory?
 
 The directory is treated as any other untracked directory (it never
 matches indexed file because of the trailing /).

Ah, right. That makes sense.

  I _think_ we still do not hit this code path in that instance, because
  we will end up in treat_directory, and we will end up checking
  directory_exists_in_index. And I cannot get it to misbehave in practice.
  So I think your fix is correct, but the exact how and why is a bit
  subtle.
 
 Thanks a lot for the help, I will try to come up with a better commit
 message now.

Thanks. I think the patch is right, but the reasoning is just a bit
subtle.

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

[PATCH] status: report ignored yet tracked directories

2013-01-05 Thread Antoine Pelisse 
Tracked directories (i.e. directories containing tracked files) that
are ignored must be reported as ignored if they contain untracked files.

Currently, tracked files or directories can't be reported untracked or ignored.
Remove that constraint when searching ignored files.

Signed-off-by: Antoine Pelisse apeli...@gmail.com
---
Torsten, Jeff,

Can you please test this patch and tell me if this is better ? (t7061 is now
successful with core.ignorecase=true)

This patch applies on top of ap/status-ignored-in-ignored-directory (but
 should also apply cleanly on top of next for testing purpose).

Thanks,

 dir.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index 9b80348..eefa8ab 100644
--- a/dir.c
+++ b/dir.c
@@ -672,7 +672,8 @@ static struct dir_entry *dir_entry_new(const char 
*pathname, int len)

 static struct dir_entry *dir_add_name(struct dir_struct *dir, const char 
*pathname, int len)
 {
-   if (cache_name_exists(pathname, len, ignore_case))
+   if (!(dir-flags  DIR_SHOW_IGNORED) 
+   cache_name_exists(pathname, len, ignore_case))
return NULL;

ALLOC_GROW(dir-entries, dir-nr+1, dir-alloc);
--
1.7.12.4.2.geb8c5b8.dirty

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

Re: [PATCH] status: report ignored yet tracked directories

2013-01-05 Thread Torsten Bögershausen 
On 05.01.13 21:42, Antoine Pelisse wrote:
 Tracked directories (i.e. directories containing tracked files) that
 are ignored must be reported as ignored if they contain untracked files.

 Currently, tracked files or directories can't be reported untracked or 
 ignored.
 Remove that constraint when searching ignored files.

 Signed-off-by: Antoine Pelisse apeli...@gmail.com
 ---
 Torsten, Jeff,

 Can you please test this patch and tell me if this is better ? (t7061 is now
 successful with core.ignorecase=true)

 This patch applies on top of ap/status-ignored-in-ignored-directory (but
  should also apply cleanly on top of next for testing purpose).

 Thanks,

  dir.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

 diff --git a/dir.c b/dir.c
 index 9b80348..eefa8ab 100644
 --- a/dir.c
 +++ b/dir.c
 @@ -672,7 +672,8 @@ static struct dir_entry *dir_entry_new(const char 
 *pathname, int len)

  static struct dir_entry *dir_add_name(struct dir_struct *dir, const char 
 *pathname, int len)
  {
 - if (cache_name_exists(pathname, len, ignore_case))
 + if (!(dir-flags  DIR_SHOW_IGNORED) 
 + cache_name_exists(pathname, len, ignore_case))
   return NULL;

   ALLOC_GROW(dir-entries, dir-nr+1, dir-alloc);
 --
 1.7.12.4.2.geb8c5b8.dirty

(BTW: thanks for contributing to git)
Antoine, the test is OK:
# passed all 10 test(s)

I'm not sure if I am happy with the commit message, so I try to have an improved
version below, which may be a starting point for a discussion:

git status: report ignored directories correctly

A directory containing tracked files where the directory
is ignored must be reported as ignored even if it contains untracked files.

/Torsten


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

Re: [PATCH] status: report ignored yet tracked directories

2013-01-05 Thread Jeff King 
On Sat, Jan 05, 2013 at 09:42:43PM +0100, Antoine Pelisse wrote:

 Tracked directories (i.e. directories containing tracked files) that
 are ignored must be reported as ignored if they contain untracked files.
 
 Currently, tracked files or directories can't be reported untracked or 
 ignored.
 Remove that constraint when searching ignored files.
 
 Signed-off-by: Antoine Pelisse apeli...@gmail.com
 ---

I was expecting to see some explanation of the user-visible bug here. In
other words, what does this fix, and why does the bug only happen when
core.ignorecase is set.

Looking at your fix and remembering how the index hashing works, I think
the answer is that:

  1. This bug only affects directories, because they are the only thing
 that can be simultaneously ignored and untracked and tracked
 (i.e., they have entries of both, and we are using
 DIR_SHOW_OTHER_DIRECTORIES).

  2. When core.ignorecase is false, the index name hash contains only
 the file entries, and cache_name_exists returns an exact match. So
 it doesn't matter if we make an extra check when adding the
 directory via dir_add_name; we know that it will not be there, and
 the final check is a no-op.

  3. When core.ignorecase is true, we also store directory entries in
 the index name hash, and this extra check is harmful; the entry
 does not really exist in the index, and we still need to add it.

But that makes me wonder. In the ignorecase=false case, I claimed that
the check in dir_add_name is a no-op for mixed tracked/ignored
directories. But it is presumably not a no-op for other cases. Your
patch only turns it off when DIR_SHOW_IGNORED is set. But is it possible
for us to have DIR_SHOW_IGNORED set, _and_ to pass in a path that exists
in the index as a regular file?

I think in the normal file case, we'd expect treat_path to just tell us
that it is handled, and we would not ever call dir_add_name in the first
place. But what if we have an index entry for a file, but the working
tree now contains a directory?

I _think_ we still do not hit this code path in that instance, because
we will end up in treat_directory, and we will end up checking
directory_exists_in_index. And I cannot get it to misbehave in practice.
So I think your fix is correct, but the exact how and why is a bit
subtle.

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