Re: [RFC PATCH] Re: Empty directories...

On Fri, 20 Jul 2007, Linus Torvalds wrote:

As far as I can tell, it would have been exactly the same thing as the 
S_IFDIR, just instead of the S_IFDIR check, you'd have had to check the 
end of the filename for being '/'.

BTW, there is actually one big difference, and the '/' at the end actually 
has one huge advantage.

Why? Because my preliminary patches sort the index entries wrong. A 
directory should always sort *as*if* it had the '/' at the end.

See base_name_compare() for details.

And we've never done that for the index, because the index has never had 
this issue (since it never contained directories). So sit down and compare 
base_name_compare (for tree entries) with cache_name_compare() (for index 
entries), and see how the latter doesn't care about the type of names.

This was actually something that I hit already with subproject support, 
and one of my very first patches even had some (aborted) code to start 
sorting subprojects in the index the way we sort directories.

And I *should* have done it that way, but I never did. It now makes the 
S_ISDIR handling harder, because directories really do have to be sorted 
as if they had the '/' at the end, or "git-fsck" will complain about bad 
sorting.

Sad, sad, sad. It effectively means that S_IFGITLINK is *not* quite the 
same as S_IFDIR, because they sort differently. Duh.

Of course, it seldom matters, but basically, you should test a directory 
structure that has the files

	dir.c
	dir/test

in it, and the "dir" directory should always sort _after_ "dir.c".

And yes, having the index entry with a '/' at the end would handle that 
automatically.

As it is, with the "mode" difference, it instead needs to fix up 
"cache_name_compare()". Admittedly, that would actually be a cleanup 
(since it would now match base_name_compare() in logic, and could actually 
use that to do the name comparison!), but it's a damn painful cleanup 
because we don't even pass in the mode to "cache_name_compare()", since we 
never needed it.

Gaah.

cache_name_compare itself isn't used in that many places, but it's used 
by "index_name_pos()/cache_name_pos()", which *is* used in many places. 
And again, that one doesn't even have the mode, so it cannot pass it down.

So it probably *is* easier to add the '/' at the end of the name instead, 
to make directories sort the right way in the index. I'd still suggest you 
*also* make the mode be S_IFDIR, though (and preferably make git-fsck 
actually verify that the mode and the last character of the name 
matches!).

		Linus