[dev] [sbase][PATCH] diff: 1 minor bug fix + code improvements

From: Mattias Andrée <maandree_AT_kth.se>
Date: Sun, 31 Jan 2016 20:55:42 +0100

The fix bug was in main() in the code block
for `else if (old && !new)`.

Signed-off-by: Mattias Andrée <maandree_AT_kth.se>
---
 diff.c | 172 +++++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 92 insertions(+), 80 deletions(-)
diff --git a/diff.c b/diff.c
index 3c99ae8..558f834 100644
--- a/diff.c
+++ b/diff.c
_AT_@ -32,7 +32,6 @@
 #define eprintf(...)   enprintf(EXIT_FAILURE, __VA_ARGS__)
 #define eperror(...)   (perror(__VA_ARGS__), exit(EXIT_FAILURE))
 
-#define CLASSIFY(f)  (!(f) ? "directory" : (f)->is_empty ? "regular empty file" : "regular file")
 #define BOLD(...)    use_colour ? "\033[1m" : "", __VA_ARGS__, use_colour ? "\033[m" : ""
 
 struct file_data {
_AT_@ -99,7 +98,8 @@ load_lines(const char *pathname)
 		eperror(pathname);
 	}
 
-	fstat(fd, &attr);
+	if (fstat(fd, &attr))
+		eperror(pathname);
 	if (S_ISDIR(attr.st_mode))
 		return 0;
 
_AT_@ -126,6 +126,8 @@ load_lines(const char *pathname)
 	}
 	bin = (strchr(p, '\0') != buffer + ptr);
 
+	/* This is a bit ugly, if not done this way, it would require unnecessarily many
+	 * malloc:s to create rc and unnecessarily many free:s to destroy it. */
 	rc = erealloc(buffer, sizeof(*rc) + (n + 1) * sizeof(char *) + (ptr + 1 + sizeof(NO_LF_MARK)));
 	buffer = ((char *)rc) + sizeof(*rc) + (n + 1) * sizeof(char *);
 	memmove(buffer, rc, ptr);
_AT_@ -156,6 +158,32 @@ load_lines(const char *pathname)
 }
 
 static char *
+join_paths(const char *a, const char *b)
+{
+	char *rc = emalloc(strlen(a) + strlen(b) + sizeof("/"));
+	sprintf(rc, "%s/%s", a, b);
+	return rc;
+}
+
+static const char *
+classify(struct file_data *f)
+{
+	return !f ? "directory" : f->is_empty ? "regular empty file" : "regular file";
+}
+
+static int
+is_incommensurable(mode_t mode)
+{
+	/* POSIX specifies that if a and b refer to the same special device,
+	 * there should be no comparision. This seems unnecessary since it
+	 * also specifies that special devices and FIFO:s shall not be compared.
+	 * We extend this to not compare sockets either. POSIX says that it
+	 * is implementation-specified for other types than special files,
+	 * FIFO:s, regular files and directories. */
+	return S_ISCHR(mode) || S_ISBLK(mode) || S_ISFIFO(mode) || S_ISSOCK(mode);
+}
+
+static char *
 rstrip(char *text, char *removed)
 {
 	char *end = strchr(text, '\0');
_AT_@ -183,45 +211,41 @@ strcmp_rstrip_a(char *a, char *b)
 
 /* TODO use <20160128154757.GA20170_AT_debian> when `an` is too large. */
 static char *
-diff2_(char **a, char **b, size_t an, size_t bn, int (*cmp)(char *, char *))
+diff2_minimal(char **a, char **b, size_t an, size_t bn, int (*cmp)(char *, char *))
 {
-#define matrix (*matrix)
-#define map (*map)
-	char map[an + 1][bn + 1] = emalloc(sizeof(char[an + 1][bn + 1]));
-	size_t matrix[2][bn + 1] = ecalloc(1, sizeof(size_t[2][bn + 1]));
+	char (*map)[an + 1][bn + 1] = emalloc(sizeof(char[an + 1][bn + 1]));
+	size_t (*matrix)[2][bn + 1] = ecalloc(1, sizeof(size_t[2][bn + 1]));
 	char *rc;
 	size_t ai, bi, ri = 0, mi = 0;
 
-	memset(map[0], 2, bn + 1);
+	memset((*map)[0], 2, bn + 1);
 
 	a--, b--;
 	for (ai = 1; ai <= an; ai++) {
-		size_t *last = matrix[mi];
-		size_t *this = matrix[mi ^= 1];
-		map[ai][0] = 1;
+		size_t *last = (*matrix)[mi];
+		size_t *this = (*matrix)[mi ^= 1];
+		(*map)[ai][0] = 1;
 		for (bi = 1; bi <= bn; bi++) {
 			if (!cmp(a[ai], b[bi])) {
 				this[bi] = last[bi - 1] + 1;
-				map[ai][bi] = 0;
+				(*map)[ai][bi] = 0;
 			} else {
 				size_t u = last[bi];
 				size_t l = this[bi - 1];
 				this[bi] = l >= u ? l : u;
-				map[ai][bi] = 1 + (l >= u);
+				(*map)[ai][bi] = 1 + (l >= u);
 			}
 		}
 	}
-#undef matrix
 	free(matrix);
 
 	rc = emalloc(an + bn + 1);
 	rc[ri++] = END_OF_PATH;
 	for (ai = an, bi = bn; ai + bi; ri++) {
-		rc[ri] = map[ai][bi];
+		rc[ri] = (*map)[ai][bi];
 		ai -= rc[ri] != 2;
 		bi -= rc[ri] != 1;
 	}
-#undef map
 	free(map);
 
 	return rc + ri;
_AT_@ -296,14 +320,14 @@ enhance_trace(char *path)
 }
 
 static struct trace *
-diff2(char **a, char **b, size_t an, size_t bn, int do_rstrip)
+diff2(char **a, char **b, size_t an, size_t bn)
 {
 	size_t skip_start = 0, skip_end = 0;
 	char *rc;
 	int (*cmp)(char *, char *) = (int (*)(char *, char *))strcmp;
 	int transpose = bn < an; 
 
-	if (do_rstrip) {
+	if (bflag) {
 		char **lines;
 		char _c;
 		for (lines = !transpose ? b : a; *lines; lines++)
_AT_@ -325,7 +349,7 @@ diff2(char **a, char **b, size_t an, size_t bn, int do_rstrip)
 		if (cmp(a[an - 1], b[bn - 1]))
 			break;
 
-	rc = !transpose ? diff2_(a, b, an, bn, cmp) : diff2_(b, a, bn, an, cmp);
+	rc = !transpose ? diff2_minimal(a, b, an, bn, cmp) : diff2_minimal(b, a, bn, an, cmp);
 	if (transpose) {
 		char *path;
 		char trace;
_AT_@ -373,16 +397,15 @@ get_time_string(const struct stat *attr)
 }
 
 static int
-get_diff_chunks(struct trace *path, size_t an, size_t bn, struct chunk **head, struct chunk **tail)
+get_diff_chunks(struct trace *path, size_t an, size_t bn, struct chunk **headp, struct chunk **tailp)
 {
-#define head  (*head)
-#define tail  (*tail)
 	struct trace trace;
 	size_t ai, bi;
 	int ret = 0, suppressed = 1, have_a = 0, have_b = 0;
+	struct chunk *head = *headp;
 
 	head = ecalloc(an + bn + 1, sizeof(*head));
-	tail = head++;
+	*tailp = head++;
 
 	for (ai = bi = 0; (trace = *path++).f != END_OF_PATH;) {
 		if (trace.d > n_context) {
_AT_@ -414,9 +437,8 @@ get_diff_chunks(struct trace *path, size_t an, size_t bn, struct chunk **head, s
 		head++;
 	}
 
+	*headp = head;
 	return ret;
-#undef head
-#undef tail
 }
 
 #define OUTPUT_BEGIN\
_AT_@ -435,7 +457,7 @@ get_diff_chunks(struct trace *path, size_t an, size_t bn, struct chunk **head, s
 	printf("%s"B" %s\t%s%s\n", BOLD(new->path, get_time_string(&(new->attr))))
 
 #define OUTPUT_QUEUE\
-	path = diff2(a, b, old->line_count, new->line_count, bflag);\
+	path = diff2(a, b, old->line_count, new->line_count);\
 	ret = get_diff_chunks(path, old->line_count, new->line_count, &head, &tail);\
 	(void) chunk_old;\
 	for (head = tail;;) {\
_AT_@ -449,7 +471,7 @@ get_diff_chunks(struct trace *path, size_t an, size_t bn, struct chunk **head, s
 			break
 
 #define OUTPUT_STACK\
-	path = diff2(a, b, old->line_count, new->line_count, bflag);\
+	path = diff2(a, b, old->line_count, new->line_count);\
 	ret = get_diff_chunks(path, old->line_count, new->line_count, &head, &tail);\
 	(void) chunk_old;\
 	for (;;) {\
_AT_@ -480,7 +502,7 @@ output_unified(struct file_data *old, struct file_data *new)
 	int ret = 0;
 	int suppressed = 1;
 
-	path = diff2(old->lines, new->lines, old->line_count, new->line_count, bflag);
+	path = diff2(old->lines, new->lines, old->line_count, new->line_count);
 	path_ = path;
 
 	OUTPUT_HEAD("---", "+++");
_AT_@ -522,9 +544,6 @@ output_unified(struct file_data *old, struct file_data *new)
 static int
 output_copied(struct file_data *old, struct file_data *new)
 {
-	OUTPUT_BEGIN;
-	OUTPUT_HEAD("***", "---");
-	OUTPUT_QUEUE;
 #define PRINT_PART(L, C, S, A)\
 	printf("%s"A" %zu", use_colour ? "\033[1;3"#C"m" : "", L##i + 1 - (!have_##L));\
 	if (chunk->L##_len > 1)\
_AT_@ -541,20 +560,23 @@ output_copied(struct file_data *old, struct file_data *new)
 		L##i += chunk->f != (3 - C);\
 	}
 
+	OUTPUT_BEGIN;
+	OUTPUT_HEAD("***", "---");
+	OUTPUT_QUEUE;
+
 	printf("%s\n", use_colour ? "\033[36m***************\033[m" : "***************");
 	chunk_old = chunk;
 	PRINT_PART(a, 1, "-", "***");
 	chunk = chunk_old;
 	PRINT_PART(b, 2, "+", "---");
-#undef PRINT_PART
+
 	OUTPUT_END;
+#undef PRINT_PART
 }
 
 static int
 output_default(struct file_data *old, struct file_data *new)
 {
-	OUTPUT_BEGIN;
-	OUTPUT_QUEUE;
 #define PRINT_PART(L, C, S)\
 	for (; have_##L && chunk->f != END_OF_PATH && chunk->d <= n_context; chunk++) {\
 		if (chunk->f == 0)\
_AT_@ -567,6 +589,9 @@ output_default(struct file_data *old, struct file_data *new)
 		L##i += chunk->f != (3 - C);\
 	}
 
+	OUTPUT_BEGIN;
+	OUTPUT_QUEUE;
+
 	printf("%s%zu", use_colour ? "\033[36m" : "", ai + 1 - (!have_a));
 	if (chunk->a_len > 1)
 		printf(",%zu", ai + chunk->a_len);
_AT_@ -582,8 +607,9 @@ output_default(struct file_data *old, struct file_data *new)
 		printf("%s\n", use_colour ? "\033[36m---\033[m" : "---");
 	chunk = chunk_old;
 	PRINT_PART(b, 2, ">");
-#undef PRINT_PART
+
 	OUTPUT_END;
+#undef PRINT_PART
 }
 
 static int
_AT_@ -646,19 +672,18 @@ output_ed_alternative(struct file_data *old, struct file_data *new)
 static int
 do_binaries_differ(struct file_data *old, struct file_data *new)
 {
-#define TURN_INTO_BINARY(f)\
-	if (!f->is_binary) {\
-		char **lines = f->lines;\
-		size_t len = 0, part_len;\
-		for (; *lines; lines++) {\
-			len += 1 + (part_len = strlen(*lines));\
-			(*lines)[part_len] = '\n';\
-		}\
-		f->line_count = len - !f->lf_terminated;\
-	}
-
-	TURN_INTO_BINARY(old);
-	TURN_INTO_BINARY(new);
+	struct file_data *f = old;
+	do {
+		if (!f->is_binary) {
+			char **lines = f->lines;
+			size_t len = 0, part_len;
+			for (; *lines; lines++) {
+				len += 1 + (part_len = strlen(*lines));
+				(*lines)[part_len] = '\n';
+			}
+			f->line_count = len - !f->lf_terminated;
+		}
+	} while (f == old ? (f = new) : (void*)0);
 
 	if (old->line_count != new->line_count)
 		return 1;
_AT_@ -674,16 +699,16 @@ compare_files(struct file_data *old, struct file_data *new)
 	if (old->is_binary || new->is_binary) {
 		if (do_binaries_differ(old, new)) {
 			printf("Binary files %s and %s differ\n", old->path, new->path);
-			ret = 2;
+			return 2;
 		}
-		return ret;
+		return 0;
 	}
 
 	if (!(eflag || fflag)) {
 		if (!old->lf_terminated)
-			strcpy(strchr(old->lines[old->line_count - 1], '\0'), NO_LF_MARK);
+			strcat(old->lines[old->line_count - 1], NO_LF_MARK);
 		if (!new->lf_terminated)
-			strcpy(strchr(new->lines[new->line_count - 1], '\0'), NO_LF_MARK);
+			strcat(new->lines[new->line_count - 1], NO_LF_MARK);
 	}
 
 	ret = (uflag ? output_unified :
_AT_@ -706,10 +731,6 @@ compare_files(struct file_data *old, struct file_data *new)
 static int
 compare_directories(const char *old, const char *new, const char *diff_line)
 {
-#define GET_FILENAME(buf, i)\
-	(buf = emalloc(strlen(paths[i]) + strlen(file->d_name) + 2),\
-	 stpcpy(stpcpy(stpcpy(buf, paths[i]), "/"), file->d_name))
-
 	int ret = 0, r, i = 0, j = 1;
 	DIR *dir;
 	const char *paths[2] = { old, new };
_AT_@ -728,7 +749,7 @@ again:
 	while ((errno = 0, file = readdir(dir))) {
 		if (!strcmp(file->d_name, ".") || !strcmp(file->d_name, ".."))
 			continue;
-		GET_FILENAME(b_path, j);
+		b_path = join_paths(paths[j], file->d_name);
 		if (access(b_path, F_OK)) {
 			printf("%sOnly i %s: %s%s\n", BOLD(paths[i], file->d_name));
 			ret = ret > 1 ? ret : 1;
_AT_@ -736,7 +757,7 @@ again:
 		} else if (i == 1) {
 			goto next;
 		}
-		GET_FILENAME(a_path, i);
+		a_path = join_paths(paths[i], file->d_name);
 
 		if (stat(a_path, &a_attr))
 			eperror(a_path);
_AT_@ -745,14 +766,7 @@ again:
 
 		if (a_attr.st_dev == b_attr.st_dev && a_attr.st_ino == b_attr.st_ino)
 			goto skip;
-		/* POSIX specifies that if a and b refer to the same special device,
-		 * there should be no comparision. This seems unnecessary since it
-		 * also specifies that special devices and FIFO:s shall not be compared.
-		 * We extend this to not compare sockets either. POSIX says that it
-		 * is implementation-specified for other types than special files,
-		 * FIFO:s, regular files and directories. */
-#define IS_INCOMMENSURABLE(mode)  (S_ISCHR(mode) || S_ISBLK(mode) || S_ISFIFO(mode) || S_ISSOCK(mode))
-		if (IS_INCOMMENSURABLE(a_attr.st_mode) || IS_INCOMMENSURABLE(b_attr.st_mode))
+		if (is_incommensurable(a_attr.st_mode) || is_incommensurable(b_attr.st_mode))
 			goto skip;
 
 		a = load_lines(a_path);
_AT_@ -760,19 +774,18 @@ again:
 
 		if (!a ^ !b) {
 			printf("%sFile %s is a %s while file %s is a %s%s\n",
-			       BOLD(a_path, CLASSIFY(a), b_path, CLASSIFY(b)));
-		ret = ret > 1 ? ret : 1;
+			       BOLD(a_path, classify(a), b_path, classify(b)));
+			r = 1;
 		} else if (!a && !b && !rflag) {
 			printf("%sCommon subdirectories: %s and %s%s\n", BOLD(a_path, b_path));
-			ret = ret > 1 ? ret : 1;
+			r = 1;
 		} else if (!a && !b) {
 			r = compare_directories(a_path, b_path, diff_line);
-			ret = ret > r ? ret : r;
 		} else {
 			printf("%s%s %s %s%s\n", BOLD(diff_line, a_path, b_path));
 			r = compare_files(a, b);
-			ret = ret > r ? ret : r;
 		}
+		ret = ret > r ? ret : r;
 
 		free(a);
 		free(b);
_AT_@ -814,7 +827,7 @@ main(int argc, char *argv[])
 			len += strlen(argv[i]) + 1;
 		p = diff_line = emalloc(len + 1);
 		for (i = 0; i < argc - 2; i++)
-			p = stpcpy(stpcpy(p, argv[i]), " ");
+			p += sprintf(p, "%s ", argv[i]);
 		p[-1] = 0;
 	}
 
_AT_@ -830,7 +843,9 @@ main(int argc, char *argv[])
 	default:
 		usage();
 	} ARGEND;
-	/* Use of `atol` is intentional, '-U -1' and '-C -1' shall display the entire file. */
+	/* Use of `atol` is intentional, '-U -1' and '-C -1' shall display the entire file.
+	 * This is a not specified in POSIX, but appears in other implementations and is
+	 * useful whilst removing complexity. */
 
 	if (argc != 2 || (bflag | rflag) > 1 || cflag + eflag + fflag + uflag > 1)
 		usage();
_AT_@ -843,16 +858,14 @@ redo:
 
 	if ((old_proper || new_proper) && (!old || !new)) {
 		printf("%sFile %s is a %s while file %s is a %s%s\n",
-		       BOLD(old_proper ? old_proper : argv[0], CLASSIFY(old),
-		            new_proper ? new_proper : argv[1], CLASSIFY(new)));
+		       BOLD(old_proper ? old_proper : argv[0], classify(old),
+		            new_proper ? new_proper : argv[1], classify(new)));
 		ret = 1;
 	} else if (!old && new) {
-		old_proper = emalloc(strlen(argv[0]) + strlen(argv[1]) + 2);
-		stpcpy(stpcpy(stpcpy(old_proper, argv[0]), "/"), basename(argv[1]));
+		old_proper = join_paths(argv[0], basename(argv[1]));
 		goto redo;
 	} else if (old && !new) {
-		old_proper = emalloc(strlen(argv[0]) + strlen(argv[1]) + 2);
-		stpcpy(stpcpy(stpcpy(old_proper, argv[0]), "/"), basename(argv[1]));
+		new_proper = join_paths(argv[1], basename(argv[0]));
 		goto redo;
 	} else if (!old && !new) {
 		ret = compare_directories(argv[0], argv[1], diff_line);
_AT_@ -860,7 +873,6 @@ redo:
 		ret = compare_files(old, new);
 	}
 
-done:
 	if (fshut(stdout, "<stdout>"))
 		ret = EXIT_FAILURE;
 
-- 
2.7.0
Received on Sun Jan 31 2016 - 20:55:42 CET

This archive was generated by hypermail 2.3.0 : Sun Jan 31 2016 - 21:00:14 CET