[PATCH] fix compare symlink against readlink not data

[PATCH] fix compare symlink against readlink not data

[Kay Sievers]

Fix update-cache to compare the blob of a symlink against the link-target
and not the file it points to. Also ignore all permissions applied to
links.
Thanks to Greg for recognizing this while he added our list of symlinks
back to the udev repository.

Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
---

--- a/diff-files.c
+++ b/diff-files.c
@@ -111,7 +111,7 @@ int main(int argc, char **argv)
 			continue;
 		}
  
-		if (stat(ce->name, &st) < 0) {
+		if (lstat(ce->name, &st) < 0) {
 			if (errno != ENOENT) {
 				perror(ce->name);
 				continue;
--- a/read-cache.c
+++ b/read-cache.c
@@ -16,6 +16,9 @@ int cache_match_stat(struct cache_entry 
 	switch (ntohl(ce->ce_mode) & S_IFMT) {
 	case S_IFREG:
 		changed |= !S_ISREG(st->st_mode) ? TYPE_CHANGED : 0;
+		/* We consider only the owner x bit to be relevant for "mode changes" */
+		if (0100 & (ntohl(ce->ce_mode) ^ st->st_mode))
+			changed |= MODE_CHANGED;
 		break;
 	case S_IFLNK:
 		changed |= !S_ISLNK(st->st_mode) ? TYPE_CHANGED : 0;
@@ -43,9 +46,6 @@ int cache_match_stat(struct cache_entry 
 	if (ce->ce_uid != htonl(st->st_uid) ||
 	    ce->ce_gid != htonl(st->st_gid))
 		changed |= OWNER_CHANGED;
-	/* We consider only the owner x bit to be relevant for "mode changes" */
-	if (0100 & (ntohl(ce->ce_mode) ^ st->st_mode))
-		changed |= MODE_CHANGED;
 	if (ce->ce_dev != htonl(st->st_dev) ||
 	    ce->ce_ino != htonl(st->st_ino))
 		changed |= INODE_CHANGED;
--- a/update-cache.c
+++ b/update-cache.c
@@ -64,7 +64,7 @@ static int add_file_to_cache_1(char *pat
 	struct stat st;
 	int fd;
 	unsigned int len;
-	char target[1024];
+	char *target;
 
 	if (lstat(path, &st) < 0) {
 		if (errno == ENOENT || errno == ENOTDIR) {
@@ -90,11 +90,14 @@ static int add_file_to_cache_1(char *pat
 			return -1;
 		break;
 	case S_IFLNK:
-		len = readlink(path, target, sizeof(target));
-		if (len == -1 || len+1 > sizeof(target))
+		target = xmalloc(st.st_size+1);
+		if (readlink(path, target, st.st_size+1) != st.st_size) {
+			free(target);
 			return -1;
-		if (write_sha1_file(target, len, "blob", ce->sha1))
+		}
+		if (write_sha1_file(target, st.st_size, "blob", ce->sha1))
 			return -1;
+		free(target);
 		break;
 	default:
 		return -1;
@@ -163,6 +166,32 @@ static int compare_data(struct cache_ent
 	return match;
 }
 
+static int compare_link(struct cache_entry *ce, unsigned long expected_size)
+{
+	int match = -1;
+	char *target;
+	void *buffer;
+	unsigned long size;
+	char type[10];
+	int len;
+	target = xmalloc(expected_size);
+	len = readlink(ce->name, target, expected_size);
+	if (len != expected_size ) {
+		free(target);
+		return -1;
+	}
+	buffer = read_sha1_file(ce->sha1, type, &size);
+	if (!buffer) {
+		free(target);
+		return -1;
+	}
+	if (size == expected_size)
+		match = memcmp(buffer, target, size);
+	free(buffer);
+	free(target);
+	return match;
+}
+
 /*
  * "refresh" does not calculate a new sha1 file or bring the
  * cache up-to-date for mode/content changes. But what it
@@ -194,8 +223,18 @@ static struct cache_entry *refresh_entry
 	if (changed & (MODE_CHANGED | TYPE_CHANGED))
 		return ERR_PTR(-EINVAL);
 
-	if (compare_data(ce, st.st_size))
+	switch (st.st_mode & S_IFMT) {
+	case S_IFREG:
+		if (compare_data(ce, st.st_size))
+			return ERR_PTR(-EINVAL);
+		break;
+	case S_IFLNK:
+		if (compare_link(ce, st.st_size))
+			return ERR_PTR(-EINVAL);
+		break;
+	default:
 		return ERR_PTR(-EINVAL);
+	}
 
 	cache_changed = 1;
 	size = ce_size(ce);

Re: [PATCH] fix compare symlink against readlink not data

[Greg KH]

On Fri, May 06, 2005 at 03:45:01PM +0200, Kay Sievers wrote:
> Fix update-cache to compare the blob of a symlink against the link-target
> and not the file it points to. Also ignore all permissions applied to
> links.
> Thanks to Greg for recognizing this while he added our list of symlinks
> back to the udev repository.

Hm, even with this patch applied (it's in Linus's tree right now), I
still get the following with a clean checked out udev tree:
 $ cg-diff
 Index: test/sys/block/cciss!c0d0/device
 ===================================================================
 Index: test/sys/block/rd!c0d0/device
 ===================================================================
 Index: test/sys/block/sda/device
 ===================================================================
 Index: test/sys/bus/pci/devices/0000:00:09.0
 ===================================================================
 Index: test/sys/bus/pci/devices/0000:00:1e.0
 ===================================================================
 Index: test/sys/bus/pci/devices/0000:02:05.0
 ===================================================================
 Index: test/sys/bus/pci/drivers/aic7xxx/0000:02:05.0
 ===================================================================
 Index: test/sys/bus/scsi/devices/0:0:0:0
 ===================================================================
 Index: test/sys/bus/scsi/drivers/sd/0:0:0:0
 ===================================================================
 Index: test/sys/bus/usb-serial/devices/ttyUSB0
 ===================================================================
 Index: test/sys/bus/usb-serial/drivers/PL-2303/ttyUSB0
 ===================================================================
 Index: test/sys/bus/usb/devices/3-0:1.0
 ===================================================================
 Index: test/sys/bus/usb/devices/3-1
 ===================================================================
 Index: test/sys/bus/usb/devices/3-1:1.0
 ===================================================================
 Index: test/sys/bus/usb/devices/usb3
 ===================================================================
 Index: test/sys/bus/usb/drivers/hub/3-0:1.0
 ===================================================================
 Index: test/sys/bus/usb/drivers/pl2303/3-1:1.0
 ===================================================================
 Index: test/sys/bus/usb/drivers/usb/3-1
 ===================================================================
 Index: test/sys/bus/usb/drivers/usb/usb3
 ===================================================================
 Index: test/sys/class/tty/ttyUSB0/device
 ===================================================================
 Index: test/sys/devices/pci0000:00/0000:00:09.0/usb3/3-1/ttyUSB0/driver
 ===================================================================

Any idea?

thanks,

greg k-h

Re: [PATCH] fix compare symlink against readlink not data

[Kay Sievers]

On Fri, 2005-05-06 at 09:03 -0700, Greg KH wrote:
> On Fri, May 06, 2005 at 03:45:01PM +0200, Kay Sievers wrote:
> > Fix update-cache to compare the blob of a symlink against the link-target
> > and not the file it points to. Also ignore all permissions applied to
> > links.
> > Thanks to Greg for recognizing this while he added our list of symlinks
> > back to the udev repository.
> 
> Hm, even with this patch applied (it's in Linus's tree right now), I
> still get the following with a clean checked out udev tree:
>  $ cg-diff
>  Index: test/sys/block/cciss!c0d0/device
>  ===================================================================

I can't reproduce this. Are you sure, that the git-core binaries are
called and not the cogito ones?

  git-update-cache --refresh
  git-diff-cache -r HEAD
from the core-git should print nothing.

Thanks,
Kay

Re: [PATCH] fix compare symlink against readlink not data

[Junio C Hamano]

>>>>> "GKH" == Greg KH <greg@kroah.com> writes:

GKH> On Fri, May 06, 2005 at 03:45:01PM +0200, Kay Sievers wrote:
>> Thanks to Greg for recognizing this while he added our list of symlinks
>> back to the udev repository.

GKH> Hm, even with this patch applied (it's in Linus's tree right now), I
GKH> still get the following with a clean checked out udev tree:
GKH>  $ cg-diff
GKH>  Index: test/sys/block/cciss!c0d0/device
GKH>  ===================================================================
GKH>  Index: test/sys/block/rd!c0d0/device
GKH>  ===================================================================

GKH> Any idea?

I do not use Cogito but probably it is this piece of code in
cg-Xdiffdo.  It is assuming that a valid SHA1 means it can diff
against the filesystem object that resides there.  It used to be
the case before symlinks but not anymore.

    mkbanner () {
            loc=$1; treeid=$2; fname=$3; mode=$4; sha1=$5;

            if [ "$sha1" != "0000000000000000000000000000000000000000" ]; then
                    git-cat-file blob $sha1 >$loc
            else
                    ln -s "$(pwd)/$fname" "$loc"
                    sha1="!"
            fi

Maybe changing the if to (I'm writing this in e-mail editor so
completely untested) something like:

    if expr "$sha1" : '0*$' >/dev/null ||
       expr "$mode" : '.*120000$' >/dev/null
    then
        git-cat-file blob "$sha1" >$loc

would help?

Also could you try the low-level git command, git-diff-cache -p,
against the tree you are comparing?  The built-in diff stuff
might get this wrong too.

Re: [PATCH] fix compare symlink against readlink not data

[Greg KH]

On Fri, May 06, 2005 at 06:23:34PM +0200, Kay Sievers wrote:
> On Fri, 2005-05-06 at 09:03 -0700, Greg KH wrote:
> > On Fri, May 06, 2005 at 03:45:01PM +0200, Kay Sievers wrote:
> > > Fix update-cache to compare the blob of a symlink against the link-target
> > > and not the file it points to. Also ignore all permissions applied to
> > > links.
> > > Thanks to Greg for recognizing this while he added our list of symlinks
> > > back to the udev repository.
> > 
> > Hm, even with this patch applied (it's in Linus's tree right now), I
> > still get the following with a clean checked out udev tree:
> >  $ cg-diff
> >  Index: test/sys/block/cciss!c0d0/device
> >  ===================================================================
> 
> I can't reproduce this. Are you sure, that the git-core binaries are
> called and not the cogito ones?
> 
>   git-update-cache --refresh
>   git-diff-cache -r HEAD
> from the core-git should print nothing.

Odd.  If I reclone the whole tree from the udev kernel.org tree, then it
works just fine.  If I create a new copy of my local tree, I still have
the same problem.  Diffing the trees shows no difference in the objects
at all...

Can you add a symlink to a local tree and see if you can duplicate this?

thanks,

greg k-h

Re: [PATCH] fix compare symlink against readlink not data

[Greg KH]

On Fri, May 06, 2005 at 09:26:15AM -0700, Junio C Hamano wrote:
> 
> Also could you try the low-level git command, git-diff-cache -p,
> against the tree you are comparing?  The built-in diff stuff
> might get this wrong too.

No, 'git-diff-cache -r HEAD' shows me:

$ git-diff-cache -r HEAD
*120000->100644 blob    2d78258b1a0fe49afabc8c16a352117df5dc338a->2d78258b1a0fe49afabc8c16a352117df5dc338a      test/sys/block/cciss!c0d0/device
*120000->100644 blob    2d78258b1a0fe49afabc8c16a352117df5dc338a->2d78258b1a0fe49afabc8c16a352117df5dc338a      test/sys/block/rd!c0d0/device
*120000->100644 blob    2d78258b1a0fe49afabc8c16a352117df5dc338a->2d78258b1a0fe49afabc8c16a352117df5dc338a      test/sys/block/sda/device
*120000->100644 blob    1c776568bdc9dc750addd0885dded6b008a44460->1c776568bdc9dc750addd0885dded6b008a44460      test/sys/bus/pci/devices/0000:00:09.0
*120000->100644 blob    e000c77614a23ad57fed284bd007ed7c1cb7872e->e000c77614a23ad57fed284bd007ed7c1cb7872e      test/sys/bus/pci/devices/0000:00:1e.0
*120000->100644 blob    630d35bf617944a4ba6afc90ca5176cb342a2662->630d35bf617944a4ba6afc90ca5176cb342a2662      test/sys/bus/pci/devices/0000:02:05.0
*120000->100644 blob    bd644e0e9d0c2f289bc4a3e3a034d528d5d671cc->bd644e0e9d0c2f289bc4a3e3a034d528d5d671cc      test/sys/bus/pci/drivers/aic7xxx/0000:02:05.0
*120000->100644 blob    ebb65b3bac36ef935a55a7f1010e4d3a242188eb->ebb65b3bac36ef935a55a7f1010e4d3a242188eb      test/sys/bus/scsi/devices/0:0:0:0
*120000->100644 blob    239003f712dd9112171e635a44160da1898f5996->239003f712dd9112171e635a44160da1898f5996      test/sys/bus/scsi/drivers/sd/0:0:0:0
*120000->100644 blob    b7733a68e08e564300212a22c9f81888c12bb55a->b7733a68e08e564300212a22c9f81888c12bb55a      test/sys/bus/usb-serial/devices/ttyUSB0
*120000->100644 blob    177f109e4899cf4008b9413933392d4f07832fdc->177f109e4899cf4008b9413933392d4f07832fdc      test/sys/bus/usb-serial/drivers/PL-2303/ttyUSB0
*120000->100644 blob    9137978832942ecce572d376f14244c1588748a2->9137978832942ecce572d376f14244c1588748a2      test/sys/bus/usb/devices/3-0:1.0
*120000->100644 blob    e47b4d58c4e5406bdba3ea1384c0c3efe007b8f6->e47b4d58c4e5406bdba3ea1384c0c3efe007b8f6      test/sys/bus/usb/devices/3-1
*120000->100644 blob    f519185eb36af29f79ca89d4b3d51011756b6837->f519185eb36af29f79ca89d4b3d51011756b6837      test/sys/bus/usb/devices/3-1:1.0
*120000->100644 blob    fb1919e7c9794ce31a257b50621f71f6f4f8bdef->fb1919e7c9794ce31a257b50621f71f6f4f8bdef      test/sys/bus/usb/devices/usb3
*120000->100644 blob    2bc160c20cd950c52e34d4bab30e1e25d6f4df34->2bc160c20cd950c52e34d4bab30e1e25d6f4df34      test/sys/bus/usb/drivers/hub/3-0:1.0
*120000->100644 blob    49d32d5abd7e26766f4c905f1d4edf1e28f8b322->49d32d5abd7e26766f4c905f1d4edf1e28f8b322      test/sys/bus/usb/drivers/pl2303/3-1:1.0
*120000->100644 blob    03c76193e99a93c7ff45c9ac986d2bc8e0706b0b->03c76193e99a93c7ff45c9ac986d2bc8e0706b0b      test/sys/bus/usb/drivers/usb/3-1
*120000->100644 blob    61dc52a61345178c8c171ecfe96df9646af2f16c->61dc52a61345178c8c171ecfe96df9646af2f16c      test/sys/bus/usb/drivers/usb/usb3
*120000->100644 blob    b7733a68e08e564300212a22c9f81888c12bb55a->b7733a68e08e564300212a22c9f81888c12bb55a      test/sys/class/tty/ttyUSB0/device
*120000->100644 blob    9ff2c81f529a95bd93ddaf66de6a72c74166c268->9ff2c81f529a95bd93ddaf66de6a72c74166c268      test/sys/devices/pci0000:00/0000:00:09.0/usb3/3-1/ttyUSB0/driver

thanks,

greg k-h

Re: [PATCH] fix compare symlink against readlink not data

[Kay Sievers]

On Fri, 2005-05-06 at 09:36 -0700, Greg KH wrote:
> On Fri, May 06, 2005 at 06:23:34PM +0200, Kay Sievers wrote:
> > On Fri, 2005-05-06 at 09:03 -0700, Greg KH wrote:
> > > On Fri, May 06, 2005 at 03:45:01PM +0200, Kay Sievers wrote:
> > > > Fix update-cache to compare the blob of a symlink against the link-target
> > > > and not the file it points to. Also ignore all permissions applied to
> > > > links.
> > > > Thanks to Greg for recognizing this while he added our list of symlinks
> > > > back to the udev repository.
> > > 
> > > Hm, even with this patch applied (it's in Linus's tree right now), I
> > > still get the following with a clean checked out udev tree:
> > >  $ cg-diff
> > >  Index: test/sys/block/cciss!c0d0/device
> > >  ===================================================================
> > 
> > I can't reproduce this. Are you sure, that the git-core binaries are
> > called and not the cogito ones?
> > 
> >   git-update-cache --refresh
> >   git-diff-cache -r HEAD
> > from the core-git should print nothing.
> 
> Odd.  If I reclone the whole tree from the udev kernel.org tree, then it
> works just fine.  If I create a new copy of my local tree, I still have
> the same problem.  Diffing the trees shows no difference in the objects
> at all...
> 
> Can you add a symlink to a local tree and see if you can duplicate this?

Works as it should be.
What happens when you throw away your old .git/index with?
  git-read-tree HEAD

Kay

Re: [PATCH] fix compare symlink against readlink not data

[Linus Torvalds]

On Fri, 6 May 2005, Greg KH wrote:
> 
> Odd.  If I reclone the whole tree from the udev kernel.org tree, then it
> works just fine.  If I create a new copy of my local tree, I still have
> the same problem.  Diffing the trees shows no difference in the objects
> at all...

You've not updated your cache.

Guys, remember this command:

	git-diff-files

Just like that, with no arguments. It shows you what is different in your 
cache. If you get a lot of output, it means that your index file isn't 
up-to-date.

The other magic command is

	git-update-cache --refresh

and you need to do that after you merge.

If you use cogito, and cogito doesn't refresh after pulls etc, that would
be a cogito bug. But if you do things like "cp -a" of a git tree, and 
you forget to refresh the cache in the new tree, than that is _your_ bug..

		Linus

Re: [PATCH] fix compare symlink against readlink not data

[Greg KH]

On Fri, May 06, 2005 at 10:11:49AM -0700, Linus Torvalds wrote:
> 
> 
> On Fri, 6 May 2005, Greg KH wrote:
> > 
> > Odd.  If I reclone the whole tree from the udev kernel.org tree, then it
> > works just fine.  If I create a new copy of my local tree, I still have
> > the same problem.  Diffing the trees shows no difference in the objects
> > at all...
> 
> You've not updated your cache.
> 
> Guys, remember this command:
> 
> 	git-diff-files
> 
> Just like that, with no arguments. It shows you what is different in your 
> cache. If you get a lot of output, it means that your index file isn't 
> up-to-date.
> 
> The other magic command is
> 
> 	git-update-cache --refresh

Damm, I still was using update-cache and checkout-cache from an old git
version.  That was my problem.

Sorry for the noise, it works just fine.

thanks,

greg k-h