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

[David Kastrup <dak@gnu.org>]

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

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

Personally, I am not much in favor of using different names in index
and repository.

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

dir.c and readcache.c

> but it's used by "index_name_pos()/cache_name_pos()", which *is*
> used in many places.

cache_name_pos:
builtin-apply.c
builtin-blame.c
builtin-checkout-index.c
builtin-ls-files.c
builtin-mv.c
builtin-read-tree.c
builtin-rm.c
builtin-update-index.c
diff.c
diff-lib.c
dir.c
merge-index.c
sha1_name.c
unpack-trees.c
wt-status.c

index_name_pos:
read-cache.c

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

Actually, pretty much all of the above files are likely to get touched
by directory support one way or another anyway.  One really should aim
for the cleanest solution in the long run, and this for me more or
less means that it makes no sense to have different names in index and
repository.  Putting that slash in always would probably simplify some
logic in the repository as well, but I don't really like something as
marker-like as "/" in the data structures.  Putting a slash there
would involve a three-phase plan:

a) make fsck and the other code deal gracefully with either slash or
   no slash.
Wait until everybody uses this code.

b) make the code actually _put_ slashes there.
Wait until everybody has used this code.

c) deal with it for all eternity, oops: since rewriting the
   cryptographic history of existing repositories is pretty much out
   as far as I understand (which might be insufficient), one has to
   navigate around slash/noslash all the time when accessing
   repositories, including the sorting.  The index, however, can at
   one point of time phase out the slash-specific sorting.  There is
   no such thing as prehistoric indexes we would need to mind.

I guess that looks like not being worth the pain.  Double the code or
no money back.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum