From: Junio C Hamano <junkio@cox.net>
Date: Wed, 04 May 2005 23:16:22 GMT
Subject: Re: git and symlinks as tracked content
Message-ID: <7vwtqemvjt.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20050504223532.GA22967@vrfy.org>

It seems to follow the original suggestion by Linus and looks
good.  Some comments:

 * It continues to assume that S_IFREG, S_IFDIR and S_IFLNK have
   the same bit pattern everywhere.  In the same spirit as we
   store mode bits in network byte order, it may be a good time
   to introduce something like this:

   -#define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644)
   -#define create_ce_mode(mode) htonl(S_IFREG | ce_permissions(mode))
   +#define CE_IFREG  0100000
   +#define CE_IFDIR  0040000
   +#define CE_IFLNK  0120000
   +#define CE_IFMASK 0770000
   +
   +#define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644) /* REG only */ 
   +#define create_ce_mode(mode) htonl(S_ISREG(mode) ?
   +				   (CE_IFREG | ce_permissions(mode)) :
   +				   S_ISLNK(mode) ?
   +				   CE_IFLNK :
   +				   0) /* what would we do for unknowns? */

 * read-cache.c:cache_match_stat() needs to know about the
   object type.  It was allowed to assume that anything thrown
   at it was a file, but not anymore.  How about something like
   this:

     int cache_match_stat(struct cache_entry 
     {
            unsigned int changed = 0;

    +       switch (ntohl(ce->ce_mode) & CE_IFMASK) {
    +       case CE_IFREG:
    +               changed |= !S_ISREG(st->st_mode) ? TYPE_CHANGED : 0;
    +               break;
    +       case CE_IFLNK:
    +               changed |= !S_ISLNK(st->st_mode) ? TYPE_CHANGED : 0;
    +               break;
    +       default:
    +               die("internal error: ce_mode is %o", ntohl(ce->ce_mode));
    +       }

    (in cache.h) 
     #define INODE_CHANGED   0x0010
     #define DATA_CHANGED    0x0020
    +#define TYPE_CHANGED    0x0040

  * update-cache.c:refresh_entry() needs to know that if the
    type of the path changed, it would never match:

            /*
    -        * If the mode has changed, there's no point in trying
    +        * If the mode or type has changed, there's no point in trying
             * to refresh the entry - it's not going to match
             */
    -       if (changed & MODE_CHANGED)
    +       if (changed & (MODE_CHANGED | TYPE_CHANGED))
                    return ERR_PTR(-EINVAL);

            if (compare_data(ce, st.st_size))

  * (this is just a minor nit).  Since you have st here,
    st.st_size can be used to see how big a buffer you need to
    prepare for readlink() here:

    +               unsigned int len;
    +               char target[1024];
    +               ce->ce_mode = htonl(S_IFLNK);
    +               len = readlink(path, target, sizeof(target));
    +               if (len == -1 || len+1 > sizeof(target))
    +                       return -1;

  * Probably diff.c needs to be made aware of this change.