Re: Broken adding of cache entries

[Junio C Hamano <junkio@cox.net>]

>>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:

PB> What about
PB> ... suggested changes removed.
PB> then? (Completely untested and everything.)

It is clear that it is untested.  In the sequence Kay had
trouble with:

    $ rm -fr path; date >path
    $ git-update-cache --add path
    $ rm -f path; mkdir path; date >path/file
    $ git-update-cache --add path/file

The cache_name_compare function you are looking at is called
with "path" and "path/file", and compares the name strings (up
to the length of the shorter one), and when they are the same,
then compares the lengths of them.  At that point, the if()
statements have already answered that "path" comes before
"path/file" and your changes will _not_ change the behavior of
that function.

That said, I understand the behaviour you would want to see, and
I tend to agree that this should be prevented from happening in
the first place when git-update-cache happens.  Let me think a
bit more about it.

PB> I'd still prefer add_cache_entry() to just replace the original entry
PB> (as it does otherwise).

I do not think it should just silently replace; instead it
should make the user take notice, because this is an unusual
situation.  In other words, it should refuse until the user
makes a conscious decision to tell it that the user is removing
it and then placing something completely different.

This implies (I am thinking aloud here):

    $ find fs -type f -print | xargs git-update-cache --add
    $ rm -fr fs
    $ date >fs
    $ git-update-cache --add fs

would refuse to add "fs" because it notices it has some entry
whose "name" field starts with "fs/".  Likewise, for the
opposite sequence:

    $ date >fs
    $ git-update-cache --add fs
    $ rm -f fs; mkdir fs; date >fs/file    
    $ find fs -type f -print | xargs git-update-cache --add

Telling git-update-cache about anything that contains a '/' in
its name would first check if an entry whose name is exactly one
of the leading path prefix, and refuses the operation if it
finds one.

The above logic should work and it would also work if we decide
to just replace without refusing as well.  I'll code it up and
see which one I like; maybe it will end up as an option.