Re: [ANNOUNCE] git-pasky-0.7

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

I was reviewing the differences in C-part between what is in
git-pasky and Linus tree.  By the way, I finally decided to look
at the upper layer again, now you switched to dircache based
implementation.  It looks like a good start.  Anyway, this
message is about purely C part.

First some nitpicks and possible bugs.

 - The remove_file_from_cache() function in read-cache.c was
   fixed for quite some time ago in the Linus tree to let users
   resolve unmerged path by "update-cache --remove".  You do not
   seem to have this part (the diff is reversed) [*R1*].  If this is
   not a merge oversight I'd like to know why?

 - show-diff exiting non-zero when unmatched paths are specified
   does not make much sense.  diff traditionally exits non-zero
   when differences are found.  Are you using this exit status
   to see if entries on the command line are in the dircache?
   If so that is not show-diff's job but probably belongs to
   show-files [*R2*].

Comments, commends and requests.

 - The -t option you have in show-files sounds generally
   useful [*R3*].  Could you push this upstream?

 - There are many small changes that adds free() and close(fd)
   and from my cursory looking they all look reasonable fixes.
   If you are confident with these, could please also push them
   upstream?

Thanks.

[References]

*R1*

This hunk should be reverted from Pasky if there is no good reason.

--- git.linus/read-cache.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/read-cache.c	2005-04-24 00:49:00.000000000 -0700
@@ -99,9 +99,7 @@
 int remove_file_from_cache(char *path)
 {
 	int pos = cache_name_pos(path, strlen(path));
-	if (pos < 0)
-		pos = -pos-1;
-	while (pos < active_nr && !strcmp(active_cache[pos]->name, path))
+	if (pos >= 0)
 		remove_entry_at(pos);
 	return 0;
 }

*R2*

This hunk should be reverted from Pasky if there is no good reason.

--- git.linus/show-diff.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/show-diff.c	2005-04-24 00:49:00.000000000 -0700
@@ -126,6 +127,7 @@
 	int machine_readable = 0;
 	int reverse = 0;
 	int entries = read_cache();
+	int matched = 0;
 	int i;
 
 	while (1 < argc && argv[1][0] == '-') {
@@ -161,6 +163,7 @@
 		if (1 < argc &&
 		    ! matches_pathspec(ce, argv+1, argc-1))
 			continue;
+		matched++;
 
 		if (ce_stage(ce)) {
 			if (machine_readable)
@@ -192,10 +195,8 @@
 			continue;
 		if (!machine_readable)
 			printf("%s: %s\n", ce->name, sha1_to_hex(ce->sha1));
-		else {
-			printf("%s %s%c", sha1_to_hex(ce->sha1), ce->name, 0);
-			continue;
-		}
+		else
+			printf("M %s %s%c", sha1_to_hex(ce->sha1), ce->name, 0);
 		if (silent)
 			continue;
 
@@ -208,5 +209,8 @@
 					 reverse);
 		free(old);
 	}
+
+	if (1 < argc && !matched)
+		return 1;
 	return 0;
 }

*R3*

This should be pushed upstream.

--- git.linus/show-files.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/show-files.c	2005-04-24 00:49:00.000000000 -0700
@@ -17,6 +17,11 @@
 static int show_unmerged = 0;
 static int line_terminator = '\n';
 
+static const char *tag_cached = "";
+static const char *tag_unmerged = "";
+static const char *tag_removed = "";
+static const char *tag_other = "";
+
 static const char **dir;
 static int nr_dir;
 static int dir_alloc;
@@ -107,7 +112,7 @@
 	}
 	if (show_others) {
 		for (i = 0; i < nr_dir; i++)
-			printf("%s%c", dir[i], line_terminator);
+			printf("%s%s%c", tag_other, dir[i], line_terminator);
 	}
 	if (show_cached | show_stage) {
 		for (i = 0; i < active_nr; i++) {
@@ -115,10 +120,13 @@
 			if (show_unmerged && !ce_stage(ce))
 				continue;
 			if (!show_stage)
-				printf("%s%c", ce->name, line_terminator);
+				printf("%s%s%c",
+				       ce_stage(ce) ? tag_unmerged : tag_cached,
+				       ce->name, line_terminator);
 			else
 				printf(/* "%06o %s %d %10d %s%c", */
-				       "%06o %s %d %s%c",
+				       "%s %06o %s %d %s%c",
+				       ce_stage(ce) ? tag_unmerged : tag_cached,
 				       ntohl(ce->ce_mode),
 				       sha1_to_hex(ce->sha1),
 				       ce_stage(ce),
@@ -132,7 +140,7 @@
 			struct stat st;
 			if (!stat(ce->name, &st))
 				continue;
-			printf("%s%c", ce->name, line_terminator);
+			printf("%s%s%c", tag_removed, ce->name, line_terminator);
 		}
 	}
 	if (show_ignored) {
@@ -151,6 +159,13 @@
 			line_terminator = 0;
 			continue;
 		}
+		if (!strcmp(arg, "-t")) {
+			tag_cached = "H ";
+			tag_unmerged = "M ";
+			tag_removed = "R ";
+			tag_other = "? ";
+			continue;
+		}
 
 		if (!strcmp(arg, "--cached")) {
 			show_cached = 1;
@@ -179,7 +194,7 @@
 			continue;
 		}
 
-		usage("show-files [-z] (--[cached|deleted|others|ignored|stage])*");
+		usage("show-files [-z] [-t] (--[cached|deleted|others|ignored|stage])*");
 	}
 
 	/* With no flags, we default to showing the cached files */


*R4*

These leak fixes look reasonable from cursory looking.  If you
are confident, please push them upstream.

--- git.linus/checkout-cache.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/checkout-cache.c	2005-04-24 00:49:00.000000000 -0700
@@ -48,6 +48,7 @@
 		buf[len] = 0;
 		mkdir(buf, 0755);
 	}
+	free(buf);
 }
 
 static int create_file(const char *path, unsigned int mode)
@@ -75,6 +76,8 @@
 
 	new = read_sha1_file(ce->sha1, type, &size);
 	if (!new || strcmp(type, "blob")) {
+		if (new)
+			free(new);
 		return error("checkout-cache: unable to read sha1 file of %s (%s)",
 			path, sha1_to_hex(ce->sha1));
 	}


--- git.linus/ls-tree.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/ls-tree.c	2005-04-24 00:49:00.000000000 -0700
@@ -77,6 +77,7 @@
 	if (!buffer)
 		die("unable to read sha1 file");
 	list_recursive(buffer, "tree", size, NULL);
+	free(buffer);
 	return 0;
 }
 

--- git.linus/read-tree.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/read-tree.c	2005-04-24 00:49:00.000000000 -0700
@@ -11,11 +11,14 @@
 {
 	void *buffer;
 	unsigned long size;
+	int ret;
 
 	buffer = read_tree_with_tree_or_commit_sha1(sha1, &size, 0);
 	if (!buffer)
 		return -1;
-	return read_tree(buffer, size, stage);
+	ret = read_tree(buffer, size, stage);
+	free(buffer);
+	return ret;
 }
 
 static char *lockfile_name;


--- git.linus/rev-tree.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/rev-tree.c	2005-04-24 00:49:00.000000000 -0700
@@ -56,10 +56,10 @@
 	struct commit_list *parents;
 	struct commit *obj = lookup_commit(sha1);
 
-	if (obj->object.parsed)
+	if (obj && obj->object.parsed)
 		return;
-
-	parse_commit(obj);
+	if (!obj || parse_commit(obj))
+		die("unable to parse commit (%s)", sha1_to_hex(sha1));
 	
 	parents = obj->parents;
 	while (parents) {


--- git.linus/show-diff.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/show-diff.c	2005-04-24 00:49:00.000000000 -0700
@@ -100,6 +100,7 @@
 		return;
 	}
 	show_differences("/dev/null", ce->name, old, size, reverse);
+	free(old);
 }
 
 static const char *show_diff_usage = "show-diff [-q] [-s] [-z] [paths...]";


* These all look genuine leak fixes.  If applicable to Linus tree please
  push them upstream.

--- git.linus/checkout-cache.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/checkout-cache.c	2005-04-24 00:49:00.000000000 -0700
@@ -48,6 +48,7 @@
 		buf[len] = 0;
 		mkdir(buf, 0755);
 	}
+	free(buf);
 }
 
 static int create_file(const char *path, unsigned int mode)
@@ -75,6 +76,8 @@
 
 	new = read_sha1_file(ce->sha1, type, &size);
 	if (!new || strcmp(type, "blob")) {
+		if (new)
+			free(new);
 		return error("checkout-cache: unable to read sha1 file of %s (%s)",
 			path, sha1_to_hex(ce->sha1));
 	}


--- git.linus/ls-tree.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/ls-tree.c	2005-04-24 00:49:00.000000000 -0700
@@ -77,6 +77,7 @@
 	if (!buffer)
 		die("unable to read sha1 file");
 	list_recursive(buffer, "tree", size, NULL);
+	free(buffer);
 	return 0;
 }
 
--- git.linus/read-tree.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/read-tree.c	2005-04-24 00:49:00.000000000 -0700
@@ -11,11 +11,14 @@
 {
 	void *buffer;
 	unsigned long size;
+	int ret;
 
 	buffer = read_tree_with_tree_or_commit_sha1(sha1, &size, 0);
 	if (!buffer)
 		return -1;
-	return read_tree(buffer, size, stage);
+	ret = read_tree(buffer, size, stage);
+	free(buffer);
+	return ret;
 }
 
 static char *lockfile_name;

--- git.linus/rev-tree.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/rev-tree.c	2005-04-24 00:49:00.000000000 -0700
@@ -56,10 +56,10 @@
 	struct commit_list *parents;
 	struct commit *obj = lookup_commit(sha1);
 
-	if (obj->object.parsed)
+	if (obj && obj->object.parsed)
 		return;
-
-	parse_commit(obj);
+	if (!obj || parse_commit(obj))
+		die("unable to parse commit (%s)", sha1_to_hex(sha1));
 	
 	parents = obj->parents;
 	while (parents) {

--- git.linus/sha1_file.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/sha1_file.c	2005-04-24 00:49:00.000000000 -0700
@@ -278,6 +278,7 @@
 
 	if (write(fd, compressed, size) != size)
 		die("unable to write file");
+	free(compressed);
 	close(fd);
 		
 	return 0;
@@ -295,8 +296,10 @@
 	if (fd < 0)
 		return -1;
 
-	if (fstat(fd, &st) < 0 || size != st.st_size)
+	if (fstat(fd, &st) < 0 || size != st.st_size) {
+		close(fd);
 		return -1;
+	}
 
 	map = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);


--- git.linus/show-diff.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/show-diff.c	2005-04-24 00:49:00.000000000 -0700
@@ -100,6 +100,7 @@
 		return;
 	}
 	show_differences("/dev/null", ce->name, old, size, reverse);
+	free(old);
 }
 
 static const char *show_diff_usage = "show-diff [-q] [-s] [-z] [paths...]";


--- git.linus/update-cache.c	2005-04-24 00:05:02.000000000 -0700
+++ git.pasky/update-cache.c	2005-04-24 00:49:00.000000000 -0700
@@ -104,6 +104,11 @@
 		close(fd);
 		return -1;
 	}
+	if (S_ISDIR(st.st_mode)) {
+		fprintf(stderr, "'%s' is a directory, ignoring\n", path);
+		close(fd);
+		return 0;
+	}
 	namelen = strlen(path);
 	size = cache_entry_size(namelen);
 	ce = malloc(size);
@@ -113,10 +118,15 @@
 	ce->ce_mode = create_ce_mode(st.st_mode);
 	ce->ce_flags = htons(namelen);
 
-	if (index_fd(ce->sha1, fd, &st) < 0)
+	if (index_fd(ce->sha1, fd, &st) < 0) {
+		free(ce);
 		return -1;
-
-	return add_cache_entry(ce, allow_add);
+	}
+	if (add_cache_entry(ce, allow_add)) {
+		free(ce);
+		return -1;
+	}
+	return 0;
 }
 
 static int match_data(int fd, void *buffer, unsigned long size)
@@ -141,7 +151,7 @@
 	if (fd >= 0) {
 		void *buffer;
 		unsigned long size;
-		char type[10];
+		char type[20];
 
 		buffer = read_sha1_file(ce->sha1, type, &size);
 		if (buffer) {
@@ -216,6 +226,9 @@
 			printf("%s: needs update\n", ce->name);
 			continue;
 		}
+		/* You can NOT just free active_cache[i] here, since it
+		 * might not be necessarily malloc()ed but can also come
+		 * from mmap(). */
 		active_cache[i] = new;
 	}
 }