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

From: David Kastrup <dak@gnu.org>
Date: Jul 21, 2007, 05:28 UTC
Message-ID: <85odi69vgt.fsf@lola.goethe.zz>
In-Reply-To: <alpine.LFD.0.999.0707202154220.27249@woody.linux-foundation.org>

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 11 quoted lines

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

Hm, that's bad. The thing is that the directory names I am tracking are called "." (that's what I was currently trying to reconcile your code with).

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

Hm, I'll have to check what git-fsck does.

Show 10 quoted lines

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

You completely lost me here. I guess I'll be able to pick this up only after investing considerable more time into the data structures. And I have to goto bed right now.

Show 19 quoted lines

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

The _flattened_ directory name would end in /. in my scheme. I would not want to use "xxx/" for a directory name, and "xxx" for a tree: that would be completely backwards. And I also don't like the duplication of xxx when listing objects.

Sure, that's an implementation detail, but I don't like implementations hurting my eyes...

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum