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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Jul 21, 2007, 05:08 UTC
Message-ID: <alpine.LFD.0.999.0707202154220.27249@woody.linux-foundation.org>
In-Reply-To: <alpine.LFD.0.999.0707202135450.27249@woody.linux-foundation.org>

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