Re: [PATCH] Add `git diff2`, a GNU diff workalike

Johannes Schindelin [off-list ref] writes:

Git does have a wonderful diff engine. For example, colored diffs
really shine, and there are other useful options like --check,
--patch-with-stat, etc. I always dreamt of using this diff engine
also outside of a git repository.

With this commit, you can say

	git diff2 file1 file2

Why diff2?  I would have expected this to be a new low-level
sibling of diff-files and diff-index, which in turn hook into
the overall "diff" driver.

+static int queue_diff(struct diff_options *o,
+		const char *name1, const char *name2)
+{
+	struct stat st;
+	int mode1 = 0, mode2 = 0;
+
+	if (name1) {
+		if (stat(name1, &st))
+			return error("Could not access '%s'", name1);
+		mode1 = st.st_mode;
+	}
+	if (name2) {
+		if (stat(name2, &st))
+			return error("Could not access '%s'", name1);
+		mode2 = st.st_mode;
+	}

I am still debating myself if these should be lstat(2) instead
of stat(2).  The former is more consistent with what git does.

+	if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))
+		return error("file/directory conflict: %s, %s", name1, name2);

If a/frotz is a file and b/frotz/nitfol is there, I do not think
we show an error; we say "a/frotz" was removed (see notes below,
though).

+	if (S_ISDIR(mode1) || S_ISDIR(mode2)) {
+		char buffer1[PATH_MAX], buffer2[PATH_MAX];
+		struct path_list p1 = {NULL, 0, 0, 1}, p2 = {NULL, 0, 0, 1};
+		int len1 = 0, len2 = 0, i1, i2, ret = 0;
+
+		if (name1 && read_directory(name1, &p1))
+			return -1;
+		if (name2 && read_directory(name2, &p2)) {
+			path_list_clear(&p1, 0);
+			return -1;
+		}

I suspect your favorite path-list might not be optimal for this
kind of codeflow; wouldn't reading everything in an expanding
array and sorting them with a single qsort() after reading one
directory more efficient?

+		for (i1 = i2 = 0; !ret && (i1 < p1.nr || i2 < p2.nr); ) {
+			const char *n1, *n2;
+			int comp;
+
+			if (i1 == p1.nr)
+				comp = 1;
+			else if (i2 == p2.nr)
+				comp = -1;
+			else
+				comp = strcmp(p1.items[i1].path,
+					p2.items[i2].path);

I think you would want to be consistent with how git sorts paths
here.  In particular, you can always pretend that a path that is
a directory has '/' at the end.  Which (as a side effect) means
that you do not have to worry about a/frotz and b/frotz/nitfol,
because element from p1 will be "frotz" and the one from p2 will
be "frotz/" in this case.  You will never feed queue_diff() with
the same name with d/f conflicts that way.

+int cmd_diff2(int argc, char **argv, char **envp)
+{

The rest looks quite straightforward use of diffcore API, done
very cleanly.

I didn't look, but you might also need to teach diffcore-rename
that two objects both with null object names are not equal.