Clean up diff implementation for review

This fixes several bugs, updates tests and docs, eliminates the
FILE* assumption in favor of printing callbacks for the diff patch
formatter helpers, and adds a "diff" example function that can
perform a diff from the command line.

examples/Makefile

@@ -1,13 +1,14 @@
 .PHONY: all
 
 CC = gcc
-CFLAGS = -g -I../include
+CFLAGS = -g -I../include -I../src
 LFLAGS = -L../build -lgit2 -lz
 
-all: general showindex
+all: general showindex diff
 
 % : %.c
 	$(CC) -o $@ $(CFLAGS) $< $(LFLAGS)
 
 clean:
-	$(RM) general showindex
+	$(RM) general showindex diff
+	$(RM) -r *.dSYM

examples/diff.c

@@ -0,0 +1,134 @@
+#include <stdio.h>
+#include <git2.h>
+#include <stdlib.h>
+#include <string.h>
+
+void check(int error, const char *message)
+{
+	if (error) {
+		fprintf(stderr, "%s (%d)\n", message, error);
+		exit(1);
+	}
+}
+
+int resolve_to_tree(git_repository *repo, const char *identifier, git_tree **tree)
+{
+	int err = 0;
+	size_t len = strlen(identifier);
+	git_oid oid;
+	git_object *obj = NULL;
+
+	/* try to resolve as OID */
+	if (git_oid_fromstrn(&oid, identifier, len) == 0)
+		git_object_lookup_prefix(&obj, repo, &oid, len, GIT_OBJ_ANY);
+
+	/* try to resolve as reference */
+	if (obj == NULL) {
+		git_reference *ref, *resolved;
+		if (git_reference_lookup(&ref, repo, identifier) == 0) {
+			git_reference_resolve(&resolved, ref);
+			git_reference_free(ref);
+			if (resolved) {
+				git_object_lookup(&obj, repo, git_reference_oid(resolved), GIT_OBJ_ANY);
+				git_reference_free(resolved);
+			}
+		}
+	}
+
+	if (obj == NULL)
+		return GIT_ENOTFOUND;
+
+	switch (git_object_type(obj)) {
+	case GIT_OBJ_TREE:
+		*tree = (git_tree *)obj;
+		break;
+	case GIT_OBJ_COMMIT:
+		err = git_commit_tree(tree, (git_commit *)obj);
+		git_object_free(obj);
+		break;
+	default:
+		err = GIT_ENOTFOUND;
+	}
+
+	return err;
+}
+
+char *colors[] = {
+	"\033[m", /* reset */
+	"\033[1m", /* bold */
+	"\033[31m", /* red */
+	"\033[32m", /* green */
+	"\033[36m" /* cyan */
+};
+
+int printer(void *data, char usage, const char *line)
+{
+	int *last_color = data, color = 0;
+
+	if (*last_color >= 0) {
+		switch (usage) {
+		case GIT_DIFF_LINE_ADDITION: color = 3; break;
+		case GIT_DIFF_LINE_DELETION: color = 2; break;
+		case GIT_DIFF_LINE_ADD_EOFNL: color = 3; break;
+		case GIT_DIFF_LINE_DEL_EOFNL: color = 2; break;
+		case GIT_DIFF_LINE_FILE_HDR: color = 1; break;
+		case GIT_DIFF_LINE_HUNK_HDR: color = 4; break;
+		default: color = 0;
+		}
+		if (color != *last_color) {
+			if (*last_color == 1 || color == 1)
+				fputs(colors[0], stdout);
+			fputs(colors[color], stdout);
+			*last_color = color;
+		}
+	}
+
+	fputs(line, stdout);
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	char path[GIT_PATH_MAX];
+	git_repository *repo = NULL;
+	git_tree *a, *b;
+	git_diff_options opts = {0};
+	git_diff_list *diff;
+	char *dir = ".";
+	int color = -1;
+
+	if (argc != 3) {
+		fprintf(stderr, "usage: diff <tree-oid> <tree-oid>\n");
+		exit(1);
+	}
+
+	check(git_repository_discover(path, sizeof(path), dir, 0, "/"),
+		"Could not discover repository");
+	check(git_repository_open(&repo, path),
+		"Could not open repository");
+
+	check(resolve_to_tree(repo, argv[1], &a), "Looking up first tree");
+	check(resolve_to_tree(repo, argv[2], &b), "Looking up second tree");
+
+	check(git_diff_tree_to_tree(repo, &opts, a, b, &diff), "Generating diff");
+
+	fputs(colors[0], stdout);
+
+	check(git_diff_print_compact(diff, &color, printer), "Displaying diff summary");
+
+	fprintf(stdout, "--\n");
+
+	color = 0;
+
+	check(git_diff_print_patch(diff, &color, printer), "Displaying diff");
+
+	fputs(colors[0], stdout);
+
+	git_diff_list_free(diff);
+	git_tree_free(a);
+	git_tree_free(b);
+	git_repository_free(repo);
+
+	return 0;
+}
+

include/git2/diff.h

@@ -16,21 +16,52 @@
 /**
  * @file git2/diff.h
  * @brief Git tree and file differencing routines.
+ *
+ * Calculating diffs is generally done in two phases: building a diff list
+ * then traversing the diff list.  This makes is easier to share logic
+ * across the various types of diffs (tree vs tree, workdir vs index, etc.),
+ * and also allows you to insert optional diff list post-processing phases,
+ * such as rename detected, in between the steps.  When you are done with a
+ * diff list object, it must be freed.
+ *
  * @ingroup Git
  * @{
  */
 GIT_BEGIN_DECL
 
+/**
+ * Structure describing options about how the diff should be executed.
+ *
+ * @todo Most of the parameters here are not actually supported at this time.
+ */
 typedef struct {
 	int context_lines;
 	int interhunk_lines;
 	int ignore_whitespace;
-	int force_text;
+	int force_text;			/**< generate text diffs even for binaries */
 	git_strarray pathspec;
 } git_diff_options;
 
+/**
+ * The diff list object that contains all individual file deltas.
+ */
+typedef struct git_diff_list git_diff_list;
+
+/**
+ * Description of changes to one file.
+ *
+ * When iterating over a diff list object, this will generally be passed to
+ * most callback functions and you can use the contents to understand
+ * exactly what has changed.
+ *
+ * Under some circumstances, not all fields will be filled in, but the code
+ * generally tries to fill in as much as possible.  One example is that the
+ * "binary" field will not actually look at file contents if you do not
+ * pass in hunk and/or line callbacks to the diff foreach iteration function.
+ * It will just use the git attributes for those files.
+ */
 typedef struct {
-	git_status_t status;     /* value from tree.h */
+	git_status_t status;     /**< value from tree.h */
 	unsigned int old_attr;
 	unsigned int new_attr;
 	git_oid      old_oid;
@@ -38,16 +69,22 @@ typedef struct {
 	git_blob     *old_blob;
 	git_blob     *new_blob;
 	const char   *path;
-	const char   *new_path;  /* NULL unless status is RENAMED or COPIED */
+	const char   *new_path;  /**< NULL unless status is RENAMED or COPIED */
-	int          similarity; /* value from 0 to 100 */
+	int          similarity; /**< for RENAMED and COPIED, value from 0 to 100 */
-	int          binary;     /* diff as binary? */
+	int          binary;     /**< files in diff are binary? */
 } git_diff_delta;
 
+/**
+ * When iterating over a diff, callback that will be made per file.
+ */
 typedef int (*git_diff_file_fn)(
 	void *cb_data,
 	git_diff_delta *delta,
 	float progress);
 
+/**
+ * Structure describing a hunk of a diff.
+ */
 typedef struct {
 	int old_start;
 	int old_lines;
@@ -55,6 +92,9 @@ typedef struct {
 	int new_lines;
 } git_diff_range;
 
+/**
+ * When iterating over a diff, callback that will be made per hunk.
+ */
 typedef int (*git_diff_hunk_fn)(
 	void *cb_data,
 	git_diff_delta *delta,
@@ -62,25 +102,60 @@ typedef int (*git_diff_hunk_fn)(
 	const char *header,
 	size_t header_len);
 
-#define GIT_DIFF_LINE_CONTEXT	' '
+/**
-#define GIT_DIFF_LINE_ADDITION	'+'
+ * Line origin constants.
-#define GIT_DIFF_LINE_DELETION	'-'
+ *
-#define GIT_DIFF_LINE_ADD_EOFNL '\n'
+ * These values describe where a line came from and will be passed to
-#define GIT_DIFF_LINE_DEL_EOFNL '\0'
+ * the git_diff_line_fn when iterating over a diff.  There are some
+ * special origin contants at the end that are used for the text
+ * output callbacks to demarcate lines that are actually part of
+ * the file or hunk headers.
+ */
+enum {
+	/* these values will be sent to `git_diff_line_fn` along with the line */
+	GIT_DIFF_LINE_CONTEXT   = ' ',
+	GIT_DIFF_LINE_ADDITION  = '+',
+	GIT_DIFF_LINE_DELETION  = '-',
+	GIT_DIFF_LINE_ADD_EOFNL = '\n', /**< LF was added at end of file */
+	GIT_DIFF_LINE_DEL_EOFNL = '\0', /**< LF was removed at end of file */
+	/* these values will only be sent to a `git_diff_output_fn` */
+	GIT_DIFF_LINE_FILE_HDR  = 'F',
+	GIT_DIFF_LINE_HUNK_HDR  = 'H',
+	GIT_DIFF_LINE_BINARY    = 'B'
+};
 
+/**
+ * When iterating over a diff, callback that will be made per text diff
+ * line.
+ */
 typedef int (*git_diff_line_fn)(
 	void *cb_data,
 	git_diff_delta *delta,
-	char line_origin, /* GIT_DIFF_LINE value from above */
+	char line_origin, /**< GIT_DIFF_LINE_... value from above */
 	const char *content,
 	size_t content_len);
 
-typedef struct git_diff_list git_diff_list;
+/**
-
+ * When printing a diff, callback that will be made to output each line
-/*
+ * of text.  This uses some extra GIT_DIFF_LINE_... constants for output
- * Generate diff lists
+ * of lines of file and hunk headers.
  */
+typedef int (*git_diff_output_fn)(
+	void *cb_data,
+	char line_origin, /**< GIT_DIFF_LINE_... value from above */
+	const char *formatted_output);
 
+
+/** @name Diff List Generator Functions
+ *
+ * These are the functions you would use to create (or destroy) a
+ * git_diff_list from various objects in a repository.
+ */
+/**@{*/
+
+/**
+ * Compute a difference between two tree objects.
+ */
 GIT_EXTERN(int) git_diff_tree_to_tree(
 	git_repository *repo,
 	const git_diff_options *opts,
@@ -88,29 +163,58 @@ GIT_EXTERN(int) git_diff_tree_to_tree(
 	git_tree *new,
 	git_diff_list **diff);
 
+/**
+ * Compute a difference between a tree and the index.
+ * @todo NOT IMPLEMENTED
+ */
 GIT_EXTERN(int) git_diff_index_to_tree(
 	git_repository *repo,
 	const git_diff_options *opts,
 	git_tree *old,
 	git_diff_list **diff);
 
+/**
+ * Compute a difference between the working directory and a tree.
+ * @todo NOT IMPLEMENTED
+ */
 GIT_EXTERN(int) git_diff_workdir_to_tree(
 	git_repository *repo,
 	const git_diff_options *opts,
 	git_tree *old,
 	git_diff_list **diff);
 
+/**
+ * Compute a difference between the working directory and the index.
+ * @todo NOT IMPLEMENTED
+ */
 GIT_EXTERN(int) git_diff_workdir_to_index(
 	git_repository *repo,
 	const git_diff_options *opts,
 	git_diff_list **diff);
 
+/**
+ * Deallocate a diff list.
+ */
 GIT_EXTERN(void) git_diff_list_free(git_diff_list *diff);
 
-/*
+/**@}*/
- * Process diff lists
- */
 
+
+/** @name Diff List Processor Functions
+ *
+ * These are the functions you apply to a diff list to process it
+ * or read it in some way.
+ */
+/**@{*/
+
+/**
+ * Iterate over a diff list issuing callbacks.
+ *
+ * If the hunk and/or line callbacks are not NULL, then this will calculate
+ * text diffs for all files it thinks are not binary.  If those are both
+ * NULL, then this will not bother with the text diffs, so it can be
+ * efficient.
+ */
 GIT_EXTERN(int) git_diff_foreach(
 	git_diff_list *diff,
 	void *cb_data,
@@ -118,20 +222,34 @@ GIT_EXTERN(int) git_diff_foreach(
 	git_diff_hunk_fn hunk_cb,
 	git_diff_line_fn line_cb);
 
-#ifndef _STDIO_H_
+/**
-#include <stdio.h>
+ * Iterate over a diff generating text output like "git diff --name-status".
-#endif
+ */
-
 GIT_EXTERN(int) git_diff_print_compact(
-	FILE *fp, git_diff_list *diff);
+	git_diff_list *diff,
+	void *cb_data,
+	git_diff_output_fn print_cb);
 
+/**
+ * Iterate over a diff generating text output like "git diff".
+ *
+ * This is a super easy way to generate a patch from a diff.
+ */
 GIT_EXTERN(int) git_diff_print_patch(
-	FILE *fp, git_diff_list *diff);
+	git_diff_list *diff,
+	void *cb_data,
+	git_diff_output_fn print_cb);
+
+/**@}*/
+
 
 /*
  * Misc
  */
 
+/**
+ * Directly run a text diff on two blobs.
+ */
 GIT_EXTERN(int) git_diff_blobs(
 	git_repository *repo,
 	git_blob *old,

include/git2/tree.h

@@ -313,15 +313,14 @@ enum git_treewalk_mode {
  */
 GIT_EXTERN(int) git_tree_walk(git_tree *tree, git_treewalk_cb callback, int mode, void *payload);
 
-/** @} */
-
 typedef enum {
 	GIT_STATUS_UNMODIFIED = 0,
 	GIT_STATUS_ADDED = 1,
 	GIT_STATUS_DELETED = 2,
 	GIT_STATUS_MODIFIED = 3,
-    GIT_STATUS_RENAMED = 4,
+	/* the following will only be generated by git_diff functions */
-    GIT_STATUS_COPIED = 5,
+	GIT_STATUS_RENAMED = 4,
+	GIT_STATUS_COPIED = 5,
 	GIT_STATUS_IGNORED = 6,
 	GIT_STATUS_UNTRACKED = 7
 } git_status_t;
@@ -354,5 +353,7 @@ int git_tree_diff(git_tree *old, git_tree *newer, git_tree_diff_cb cb, void *dat
 
 int git_tree_diff_index_recursive(git_tree *tree, git_index *index, git_tree_diff_cb cb, void *data);
 
+/** @} */
+
 GIT_END_DECL
 #endif

src/diff.c

@@ -12,11 +12,10 @@
 #include "blob.h"
 #include <ctype.h>
 
-static git_diff_delta *new_file_delta(
+static git_diff_delta *file_delta_new(
 	git_diff_list *diff,
 	const git_tree_diff_data *tdiff)
 {
-	git_buf path = GIT_BUF_INIT;
 	git_diff_delta *delta = git__calloc(1, sizeof(git_diff_delta));
 
 	if (!delta) {
@@ -30,10 +29,8 @@ static git_diff_delta *new_file_delta(
 	delta->new_attr = tdiff->new_attr;
 	delta->old_oid  = tdiff->old_oid;
 	delta->new_oid  = tdiff->new_oid;
-
+	delta->path     = git__strdup(diff->pfx.ptr);
-	if (git_buf_joinpath(&path, diff->pfx.ptr, tdiff->path) < GIT_SUCCESS ||
+	if (delta->path == NULL) {
-		(delta->path = git_buf_detach(&path)) == NULL)
-	{
 		git__free(delta);
 		git__rethrow(GIT_ENOMEM, "Could not allocate diff record path");
 		return NULL;
@@ -42,35 +39,140 @@ static git_diff_delta *new_file_delta(
 	return delta;
 }
 
+static void file_delta_free(git_diff_delta *delta)
+{
+	if (!delta)
+		return;
+
+	if (delta->new_path != delta->path) {
+		git__free((char *)delta->new_path);
+		delta->new_path = NULL;
+	}
+
+	git__free((char *)delta->path);
+	delta->path = NULL;
+
+	git__free(delta);
+}
+
+static int tree_add_cb(const char *root, git_tree_entry *entry, void *data)
+{
+	int error;
+	git_diff_list *diff = data;
+	ssize_t pfx_len = diff->pfx.size;
+	git_tree_diff_data tdiff;
+	git_diff_delta *delta;
+
+	memset(&tdiff, 0, sizeof(tdiff));
+	tdiff.new_attr = git_tree_entry_attributes(entry);
+	if (S_ISDIR(tdiff.new_attr))
+		return GIT_SUCCESS;
+
+	git_oid_cpy(&tdiff.new_oid, git_tree_entry_id(entry));
+	tdiff.status = GIT_STATUS_ADDED;
+	tdiff.path = git_tree_entry_name(entry);
+
+	if ((error = git_buf_joinpath(&diff->pfx, diff->pfx.ptr, root)) ||
+		(error = git_buf_joinpath(&diff->pfx, diff->pfx.ptr, tdiff.path)))
+		return error;
+
+	delta = file_delta_new(diff, &tdiff);
+	if (delta  == NULL)
+		error = GIT_ENOMEM;
+	else if ((error = git_vector_insert(&diff->files, delta)) < GIT_SUCCESS)
+		file_delta_free(delta);
+
+	git_buf_truncate(&diff->pfx, pfx_len);
+
+	return error;
+}
+
+static int tree_del_cb(const char *root, git_tree_entry *entry, void *data)
+{
+	int error;
+	git_diff_list *diff = data;
+	ssize_t pfx_len = diff->pfx.size;
+	git_tree_diff_data tdiff;
+	git_diff_delta *delta;
+
+	memset(&tdiff, 0, sizeof(tdiff));
+	tdiff.old_attr = git_tree_entry_attributes(entry);
+	if (S_ISDIR(tdiff.old_attr))
+		return GIT_SUCCESS;
+
+	git_oid_cpy(&tdiff.old_oid, git_tree_entry_id(entry));
+	tdiff.status = GIT_STATUS_DELETED;
+	tdiff.path = git_tree_entry_name(entry);
+
+	if ((error = git_buf_joinpath(&diff->pfx, diff->pfx.ptr, root)) ||
+		(error = git_buf_joinpath(&diff->pfx, diff->pfx.ptr, tdiff.path)))
+		return error;
+
+	delta = file_delta_new(diff, &tdiff);
+	if (delta  == NULL)
+		error = GIT_ENOMEM;
+	else if ((error = git_vector_insert(&diff->files, delta)) < GIT_SUCCESS)
+		file_delta_free(delta);
+
+	git_buf_truncate(&diff->pfx, pfx_len);
+
+	return error;
+}
+
 static int tree_diff_cb(const git_tree_diff_data *ptr, void *data)
 {
 	int error;
 	git_diff_list *diff = data;
+	ssize_t pfx_len = diff->pfx.size;
 
-	assert(S_ISDIR(ptr->old_attr) == S_ISDIR(ptr->new_attr));
+	error = git_buf_joinpath(&diff->pfx, diff->pfx.ptr, ptr->path);
+	if (error < GIT_SUCCESS)
+		return error;
 
-	if (S_ISDIR(ptr->old_attr)) {
+	/* there are 4 tree related cases:
+	 * - diff tree to tree, which just means we recurse
+	 * - tree was deleted
+	 * - tree was added
+	 * - tree became non-tree or vice versa, which git_tree_diff
+	 *   will already have converted into two calls: an addition
+	 *   and a deletion (thank you, git_tree_diff!)
+	 * otherwise, this is a blob-to-blob diff
+	 */
+	if (S_ISDIR(ptr->old_attr) && S_ISDIR(ptr->new_attr)) {
 		git_tree *old = NULL, *new = NULL;
-		ssize_t pfx_len = diff->pfx.size;
 
 		if (!(error = git_tree_lookup(&old, diff->repo, &ptr->old_oid)) &&
-			!(error = git_tree_lookup(&new, diff->repo, &ptr->new_oid)) &&
+			!(error = git_tree_lookup(&new, diff->repo, &ptr->new_oid)))
-			!(error = git_buf_joinpath(&diff->pfx, diff->pfx.ptr, ptr->path)))
 		{
 			error = git_tree_diff(old, new, tree_diff_cb, diff);
-			git_buf_truncate(&diff->pfx, pfx_len);
 		}
 
 		git_tree_free(old);
 		git_tree_free(new);
+	} else if (S_ISDIR(ptr->old_attr) && ptr->new_attr == 0) {
+		/* deleted a whole tree */
+		git_tree *old = NULL;
+		if (!(error = git_tree_lookup(&old, diff->repo, &ptr->old_oid))) {
+			error = git_tree_walk(old, tree_del_cb, GIT_TREEWALK_POST, diff);
+			git_tree_free(old);
+		}
+	} else if (S_ISDIR(ptr->new_attr) && ptr->old_attr == 0) {
+		/* added a whole tree */
+		git_tree *new = NULL;
+		if (!(error = git_tree_lookup(&new, diff->repo, &ptr->new_oid))) {
+			error = git_tree_walk(new, tree_add_cb, GIT_TREEWALK_POST, diff);
+			git_tree_free(new);
+		}
 	} else {
-		git_diff_delta *delta = new_file_delta(diff, ptr);
+		git_diff_delta *delta = file_delta_new(diff, ptr);
 		if (delta == NULL)
 			error = GIT_ENOMEM;
 		else if ((error = git_vector_insert(&diff->files, delta)) < GIT_SUCCESS)
-			git__free(delta);
+			file_delta_free(delta);
 	}
 
+	git_buf_truncate(&diff->pfx, pfx_len);
+
 	return error;
 }
 
@@ -91,9 +193,18 @@ static git_diff_list *git_diff_list_alloc(
 
 void git_diff_list_free(git_diff_list *diff)
 {
+	git_diff_delta *delta;
+	unsigned int i;
+
 	if (!diff)
 		return;
+
 	git_buf_free(&diff->pfx);
+	git_vector_foreach(&diff->files, i, delta) {
+		file_delta_free(delta);
+		diff->files.contents[i] = NULL;
+	}
+	git_vector_free(&diff->files);
 	git__free(diff);
 }
 
@@ -324,6 +435,11 @@ int git_diff_foreach(
 	return error;
 }
 
+typedef struct {
+	git_diff_output_fn print_cb;
+	void *cb_data;
+	git_buf *buf;
+} print_info;
 
 static char pick_suffix(int mode)
 {
@@ -337,7 +453,7 @@ static char pick_suffix(int mode)
 
 static int print_compact(void *data, git_diff_delta *delta, float progress)
 {
-	FILE *fp = data;
+	print_info *pi = data;
 	char code, old_suffix, new_suffix;
 
 	GIT_UNUSED_ARG(progress);
@@ -359,64 +475,118 @@ static int print_compact(void *data, git_diff_delta *delta, float progress)
 	old_suffix = pick_suffix(delta->old_attr);
 	new_suffix = pick_suffix(delta->new_attr);
 
+	git_buf_clear(pi->buf);
+
 	if (delta->new_path != NULL)
-		fprintf(fp, "%c\t%s%c -> %s%c\n", code,
+		git_buf_printf(pi->buf, "%c\t%s%c -> %s%c\n", code,
-				delta->path, old_suffix, delta->new_path, new_suffix);
+			delta->path, old_suffix, delta->new_path, new_suffix);
 	else if (delta->old_attr != delta->new_attr)
-		fprintf(fp, "%c\t%s%c (%o -> %o)\n", code,
+		git_buf_printf(pi->buf, "%c\t%s%c (%o -> %o)\n", code,
-				delta->path, new_suffix, delta->old_attr, delta->new_attr);
+			delta->path, new_suffix, delta->old_attr, delta->new_attr);
+	else if (old_suffix != ' ')
+		git_buf_printf(pi->buf, "%c\t%s%c\n", code, delta->path, old_suffix);
 	else
-		fprintf(fp, "%c\t%s%c\n", code, delta->path, old_suffix);
+		git_buf_printf(pi->buf, "%c\t%s\n", code, delta->path);
 
-	return GIT_SUCCESS;
+	if (git_buf_lasterror(pi->buf) != GIT_SUCCESS)
+		return git_buf_lasterror(pi->buf);
+
+	return pi->print_cb(pi->cb_data, GIT_DIFF_LINE_FILE_HDR, pi->buf->ptr);
 }
 
-int git_diff_print_compact(FILE *fp, git_diff_list *diff)
+int git_diff_print_compact(
+	git_diff_list *diff,
+	void *cb_data,
+	git_diff_output_fn print_cb)
 {
-	return git_diff_foreach(diff, fp, print_compact, NULL, NULL);
+	int error;
+	git_buf buf = GIT_BUF_INIT;
+	print_info pi;
+
+	pi.print_cb = print_cb;
+	pi.cb_data  = cb_data;
+	pi.buf      = &buf;
+
+	error = git_diff_foreach(diff, &pi, print_compact, NULL, NULL);
+
+	git_buf_free(&buf);
+
+	return error;
 }
 
-static int print_oid_range(FILE *fp, git_diff_delta *delta)
+
+static int print_oid_range(print_info *pi, git_diff_delta *delta)
 {
-	char start_oid[9], end_oid[9];
+	char start_oid[8], end_oid[8];
+
 	/* TODO: Determine a good actual OID range to print */
-	/* TODO: Print a real extra line here to match git diff */
 	git_oid_to_string(start_oid, sizeof(start_oid), &delta->old_oid);
 	git_oid_to_string(end_oid, sizeof(end_oid), &delta->new_oid);
-	if (delta->old_attr == delta->new_attr)
+
-		fprintf(fp, "index %s..%s %o\n",
+	/* TODO: Match git diff more closely */
+	if (delta->old_attr == delta->new_attr) {
+		git_buf_printf(pi->buf, "index %s..%s %o\n",
 			start_oid, end_oid, delta->old_attr);
-	else
+	} else {
-		fprintf(fp, "index %s..%s %o %o\n",
+		if (delta->old_attr == 0) {
-				start_oid, end_oid, delta->old_attr, delta->new_attr);
+			git_buf_printf(pi->buf, "new file mode %o\n", delta->new_attr);
-	return GIT_SUCCESS;
+		} else if (delta->new_attr == 0) {
+			git_buf_printf(pi->buf, "deleted file mode %o\n", delta->old_attr);
+		} else {
+			git_buf_printf(pi->buf, "old mode %o\n", delta->old_attr);
+			git_buf_printf(pi->buf, "new mode %o\n", delta->new_attr);
+		}
+		git_buf_printf(pi->buf, "index %s..%s\n", start_oid, end_oid);
+	}
+
+	return git_buf_lasterror(pi->buf);
 }
 
 static int print_patch_file(void *data, git_diff_delta *delta, float progress)
 {
-	FILE *fp = data;
+	int error;
+	print_info *pi = data;
+	const char *oldpfx = "a/";
+	const char *oldpath = delta->path;
+	const char *newpfx = "b/";
 	const char *newpath = delta->new_path ? delta->new_path : delta->path;
 
 	GIT_UNUSED_ARG(progress);
 
-	if (delta->old_blob && delta->new_blob) {
+	git_buf_clear(pi->buf);
-		fprintf(fp, "diff --git a/%s b/%s\n", delta->path, newpath);
+	git_buf_printf(pi->buf, "diff --git a/%s b/%s\n", delta->path, newpath);
-		print_oid_range(fp, delta);
+	if ((error = print_oid_range(pi, delta)) < GIT_SUCCESS)
-		fprintf(fp, "--- a/%s\n", delta->path);
+		return error;
-		fprintf(fp, "+++ b/%s\n", newpath);
+
-	} else if (delta->old_blob) {
+	if (delta->old_blob == NULL) {
-		fprintf(fp, "diff --git a/%s /dev/null\n", delta->path);
+		oldpfx = "";
-		print_oid_range(fp, delta);
+		oldpath = "/dev/null";
-		fprintf(fp, "--- a/%s\n", delta->path);
+	}
-		fputs("+++ /dev/null\n", fp);
+	if (delta->new_blob == NULL) {
-	} else if (delta->new_blob) {
+		oldpfx = "";
-		fprintf(fp, "diff --git /dev/null b/%s\n", newpath);
+		oldpath = "/dev/null";
-		print_oid_range(fp, delta);
-		fputs("--- /dev/null\n", fp);
-		fprintf(fp, "+++ b/%s\n", newpath);
 	}
 
-	return GIT_SUCCESS;
+	if (!delta->binary) {
+		git_buf_printf(pi->buf, "--- %s%s\n", oldpfx, oldpath);
+		git_buf_printf(pi->buf, "+++ %s%s\n", newpfx, newpath);
+	}
+
+	if (git_buf_lasterror(pi->buf) != GIT_SUCCESS)
+		return git_buf_lasterror(pi->buf);
+
+	error = pi->print_cb(pi->cb_data, GIT_DIFF_LINE_FILE_HDR, pi->buf->ptr);
+	if (error != GIT_SUCCESS || !delta->binary)
+		return error;
+
+	git_buf_clear(pi->buf);
+	git_buf_printf(
+		pi->buf, "Binary files %s%s and %s%s differ\n",
+		oldpfx, oldpath, newpfx, newpath);
+	if (git_buf_lasterror(pi->buf) != GIT_SUCCESS)
+		return git_buf_lasterror(pi->buf);
+
+	return pi->print_cb(pi->cb_data, GIT_DIFF_LINE_BINARY, pi->buf->ptr);
 }
 
 static int print_patch_hunk(
@@ -426,11 +596,17 @@ static int print_patch_hunk(
 	const char *header,
 	size_t header_len)
 {
-	FILE *fp = data;
+	print_info *pi = data;
+
 	GIT_UNUSED_ARG(d);
 	GIT_UNUSED_ARG(r);
-	fprintf(fp, "%.*s", (int)header_len, header);
+
-	return GIT_SUCCESS;
+	git_buf_clear(pi->buf);
+
+	if (git_buf_printf(pi->buf, "%.*s", (int)header_len, header) == GIT_SUCCESS)
+		return pi->print_cb(pi->cb_data, GIT_DIFF_LINE_HUNK_HDR, pi->buf->ptr);
+	else
+		return git_buf_lasterror(pi->buf);
 }
 
 static int print_patch_line(
@@ -440,21 +616,44 @@ static int print_patch_line(
 	const char *content,
 	size_t content_len)
 {
-	FILE *fp = data;
+	print_info *pi = data;
+
 	GIT_UNUSED_ARG(delta);
-	if (line_origin == GIT_DIFF_LINE_ADDITION)
+
-		fprintf(fp, "+%.*s", (int)content_len, content);
+	git_buf_clear(pi->buf);
-	else if (line_origin == GIT_DIFF_LINE_DELETION)
+
-		fprintf(fp, "-%.*s", (int)content_len, content);
+	if (line_origin == GIT_DIFF_LINE_ADDITION ||
+		line_origin == GIT_DIFF_LINE_DELETION ||
+		line_origin == GIT_DIFF_LINE_CONTEXT)
+		git_buf_printf(pi->buf, "%c%.*s", line_origin, (int)content_len, content);
 	else if (content_len > 0)
-		fprintf(fp, "%.*s", (int)content_len, content);
+		git_buf_printf(pi->buf, "%.*s", (int)content_len, content);
-	return GIT_SUCCESS;
+
+	if (git_buf_lasterror(pi->buf) != GIT_SUCCESS)
+		return git_buf_lasterror(pi->buf);
+
+	return pi->print_cb(pi->cb_data, line_origin, pi->buf->ptr);
 }
 
-int git_diff_print_patch(FILE *fp, git_diff_list *diff)
+int git_diff_print_patch(
+	git_diff_list *diff,
+	void *cb_data,
+	git_diff_output_fn print_cb)
 {
-	return git_diff_foreach(
+	int error;
-		diff, fp, print_patch_file, print_patch_hunk, print_patch_line);
+	git_buf buf = GIT_BUF_INIT;
+	print_info pi;
+
+	pi.print_cb = print_cb;
+	pi.cb_data  = cb_data;
+	pi.buf      = &buf;
+
+	error = git_diff_foreach(
+		diff, &pi, print_patch_file, print_patch_hunk, print_patch_line);
+
+	git_buf_free(&buf);
+
+	return error;
 }
 
 int git_diff_blobs(

tests-clar/diff/blob.c

@@ -0,0 +1,97 @@
+#include "clar_libgit2.h"
+#include "diff_helpers.h"
+
+static git_repository *g_repo = NULL;
+
+void test_diff_blob__initialize(void)
+{
+	cl_fixture_sandbox("attr");
+	cl_git_pass(p_rename("attr/.gitted", "attr/.git"));
+	cl_git_pass(p_rename("attr/gitattributes", "attr/.gitattributes"));
+	cl_git_pass(git_repository_open(&g_repo, "attr/.git"));
+}
+
+void test_diff_blob__cleanup(void)
+{
+	git_repository_free(g_repo);
+	g_repo = NULL;
+	cl_fixture_cleanup("attr");
+}
+
+void test_diff_blob__0(void)
+{
+	git_blob *a, *b, *c, *d;
+	git_oid a_oid, b_oid, c_oid, d_oid;
+	git_diff_options opts;
+	diff_expects exp;
+
+	/* tests/resources/attr/root_test1 */
+	cl_git_pass(git_oid_fromstrn(&a_oid, "45141a79", 8));
+	cl_git_pass(git_blob_lookup_prefix(&a, g_repo, &a_oid, 4));
+
+	/* tests/resources/attr/root_test2 */
+	cl_git_pass(git_oid_fromstrn(&b_oid, "4d713dc4", 8));
+	cl_git_pass(git_blob_lookup_prefix(&b, g_repo, &b_oid, 4));
+
+	/* tests/resources/attr/root_test3 */
+	cl_git_pass(git_oid_fromstrn(&c_oid, "c96bbb2c2557a832", 16));
+	cl_git_pass(git_blob_lookup_prefix(&c, g_repo, &c_oid, 8));
+
+	/* tests/resources/attr/root_test4.txt */
+	cl_git_pass(git_oid_fromstrn(&d_oid, "fe773770c5a6", 12));
+	cl_git_pass(git_blob_lookup_prefix(&d, g_repo, &d_oid, 6));
+
+	/* Doing the equivalent of a `git diff -U1` on these files */
+
+	opts.context_lines = 1;
+	opts.interhunk_lines = 0;
+	opts.ignore_whitespace = 0;
+
+	memset(&exp, 0, sizeof(exp));
+	cl_git_pass(git_diff_blobs(
+		g_repo, a, b, &opts, &exp, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.hunks == 1);
+	cl_assert(exp.lines == 6);
+	cl_assert(exp.line_ctxt == 1);
+	cl_assert(exp.line_adds == 5);
+	cl_assert(exp.line_dels == 0);
+
+	memset(&exp, 0, sizeof(exp));
+	cl_git_pass(git_diff_blobs(
+		g_repo, b, c, &opts, &exp, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.hunks == 1);
+	cl_assert(exp.lines == 15);
+	cl_assert(exp.line_ctxt == 3);
+	cl_assert(exp.line_adds == 9);
+	cl_assert(exp.line_dels == 3);
+
+	memset(&exp, 0, sizeof(exp));
+	cl_git_pass(git_diff_blobs(
+		g_repo, a, c, &opts, &exp, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.hunks == 1);
+	cl_assert(exp.lines == 13);
+	cl_assert(exp.line_ctxt == 0);
+	cl_assert(exp.line_adds == 12);
+	cl_assert(exp.line_dels == 1);
+
+	opts.context_lines = 1;
+
+	memset(&exp, 0, sizeof(exp));
+	cl_git_pass(git_diff_blobs(
+		g_repo, c, d, &opts, &exp, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.hunks == 2);
+	cl_assert(exp.lines == 14);
+	cl_assert(exp.line_ctxt == 4);
+	cl_assert(exp.line_adds == 6);
+	cl_assert(exp.line_dels == 4);
+
+	git_blob_free(a);
+	git_blob_free(b);
+	git_blob_free(c);
+	git_blob_free(d);
+}
+

tests-clar/diff/diff_helpers.c

@@ -20,3 +20,65 @@ git_tree *resolve_commit_oid_to_tree(
 	git_object_free(obj);
 	return tree;
 }
+
+int diff_file_fn(
+	void *cb_data,
+	git_diff_delta *delta,
+	float progress)
+{
+	diff_expects *e = cb_data;
+	(void)progress;
+	e->files++;
+	if (delta->old_attr == 0)
+		e->file_adds++;
+	else if (delta->new_attr == 0)
+		e->file_dels++;
+	else
+		e->file_mods++;
+	return 0;
+}
+
+int diff_hunk_fn(
+	void *cb_data,
+	git_diff_delta *delta,
+	git_diff_range *range,
+	const char *header,
+	size_t header_len)
+{
+	diff_expects *e = cb_data;
+	(void)delta;
+	(void)header;
+	(void)header_len;
+	e->hunks++;
+	e->hunk_old_lines += range->old_lines;
+	e->hunk_new_lines += range->new_lines;
+	return 0;
+}
+
+int diff_line_fn(
+	void *cb_data,
+	git_diff_delta *delta,
+	char line_origin,
+	const char *content,
+	size_t content_len)
+{
+	diff_expects *e = cb_data;
+	(void)delta;
+	(void)content;
+	(void)content_len;
+	e->lines++;
+	switch (line_origin) {
+	case GIT_DIFF_LINE_CONTEXT:
+		e->line_ctxt++;
+		break;
+	case GIT_DIFF_LINE_ADDITION:
+		e->line_adds++;
+		break;
+	case GIT_DIFF_LINE_DELETION:
+		e->line_dels++;
+		break;
+	default:
+		break;
+	}
+	return 0;
+}

tests-clar/diff/diff_helpers.h

@@ -1,4 +1,40 @@
 #include "fileops.h"
+#include "git2/diff.h"
 
 extern git_tree *resolve_commit_oid_to_tree(
 	git_repository *repo, const char *partial_oid);
+
+typedef struct {
+	int files;
+	int file_adds;
+	int file_dels;
+	int file_mods;
+
+	int hunks;
+	int hunk_new_lines;
+	int hunk_old_lines;
+
+	int lines;
+	int line_ctxt;
+	int line_adds;
+	int line_dels;
+} diff_expects;
+
+extern int diff_file_fn(
+	void *cb_data,
+	git_diff_delta *delta,
+	float progress);
+
+extern int diff_hunk_fn(
+	void *cb_data,
+	git_diff_delta *delta,
+	git_diff_range *range,
+	const char *header,
+	size_t header_len);
+
+extern int diff_line_fn(
+	void *cb_data,
+	git_diff_delta *delta,
+	char line_origin,
+	const char *content,
+	size_t content_len);

tests-clar/diff/tree.c

@@ -0,0 +1,105 @@
+#include "clar_libgit2.h"
+#include "diff_helpers.h"
+
+static git_repository *g_repo = NULL;
+
+void test_diff_tree__initialize(void)
+{
+	cl_fixture_sandbox("attr");
+	cl_git_pass(p_rename("attr/.gitted", "attr/.git"));
+	cl_git_pass(p_rename("attr/gitattributes", "attr/.gitattributes"));
+	cl_git_pass(git_repository_open(&g_repo, "attr/.git"));
+}
+
+void test_diff_tree__cleanup(void)
+{
+	git_repository_free(g_repo);
+	g_repo = NULL;
+	cl_fixture_cleanup("attr");
+}
+
+static git_tree *resolve_commit_oid_to_tree(const char *partial_oid)
+{
+	size_t len = strlen(partial_oid);
+	git_oid oid;
+	git_object *obj;
+	git_tree *tree;
+
+	if (git_oid_fromstrn(&oid, partial_oid, len) == 0)
+		git_object_lookup_prefix(&obj, g_repo, &oid, len, GIT_OBJ_ANY);
+	cl_assert(obj);
+	if (git_object_type(obj) == GIT_OBJ_TREE)
+		return (git_tree *)obj;
+	cl_assert(git_object_type(obj) == GIT_OBJ_COMMIT);
+	cl_git_pass(git_commit_tree(&tree, (git_commit *)obj));
+	git_object_free(obj);
+	return tree;
+}
+
+void test_diff_tree__0(void)
+{
+	/* grabbed a couple of commit oids from the history of the attr repo */
+	const char *a_commit = "605812a";
+	const char *b_commit = "370fe9ec22";
+	const char *c_commit = "f5b0af1fb4f5c";
+	git_tree *a = resolve_commit_oid_to_tree(a_commit);
+	git_tree *b = resolve_commit_oid_to_tree(b_commit);
+	git_tree *c = resolve_commit_oid_to_tree(c_commit);
+	git_diff_options opts;
+	git_diff_list *diff = NULL;
+	diff_expects exp;
+
+	cl_assert(a);
+	cl_assert(b);
+
+	opts.context_lines = 1;
+	opts.interhunk_lines = 0;
+	opts.ignore_whitespace = 0;
+
+	memset(&exp, 0, sizeof(exp));
+
+	cl_git_pass(git_diff_tree_to_tree(g_repo, &opts, a, b, &diff));
+
+	cl_git_pass(git_diff_foreach(
+		diff, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.files == 5);
+	cl_assert(exp.file_adds == 2);
+	cl_assert(exp.file_dels == 1);
+	cl_assert(exp.file_mods == 2);
+
+	cl_assert(exp.hunks == 5);
+
+	cl_assert(exp.lines == 7 + 24 + 1 + 6 + 6);
+	cl_assert(exp.line_ctxt == 1);
+	cl_assert(exp.line_adds == 24 + 1 + 5 + 5);
+	cl_assert(exp.line_dels == 7 + 1);
+
+	git_diff_list_free(diff);
+	diff = NULL;
+
+	memset(&exp, 0, sizeof(exp));
+
+	cl_git_pass(git_diff_tree_to_tree(g_repo, &opts, c, b, &diff));
+
+	cl_git_pass(git_diff_foreach(
+		diff, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.files == 2);
+	cl_assert(exp.file_adds == 0);
+	cl_assert(exp.file_dels == 0);
+	cl_assert(exp.file_mods == 2);
+
+	cl_assert(exp.hunks == 2);
+
+	cl_assert(exp.lines == 8 + 15);
+	cl_assert(exp.line_ctxt == 1);
+	cl_assert(exp.line_adds == 1);
+	cl_assert(exp.line_dels == 7 + 14);
+
+	git_diff_list_free(diff);
+
+	git_tree_free(a);
+	git_tree_free(b);
+	git_tree_free(c);
+}

tests-clay/diff/blob.c

@@ -1,181 +0,0 @@
-#include "clay_libgit2.h"
-#include "fileops.h"
-#include "git2/diff.h"
-
-static git_repository *g_repo = NULL;
-
-void test_diff_blob__initialize(void)
-{
-	cl_fixture_sandbox("attr");
-	cl_git_pass(p_rename("attr/.gitted", "attr/.git"));
-	cl_git_pass(p_rename("attr/gitattributes", "attr/.gitattributes"));
-	cl_git_pass(git_repository_open(&g_repo, "attr/.git"));
-}
-
-void test_diff_blob__cleanup(void)
-{
-	git_repository_free(g_repo);
-	g_repo = NULL;
-	cl_fixture_cleanup("attr");
-}
-
-typedef struct {
-	int files;
-	int hunks;
-	int hunk_new_lines;
-	int hunk_old_lines;
-	int lines;
-	int line_ctxt;
-	int line_adds;
-	int line_dels;
-} diff_expects;
-
-static void log(const char *str, int n)
-{
-	FILE *fp = fopen("/Users/rb/tmp/diff.log", "a");
-	if (n > 0)
-		fprintf(fp, "%.*s", n, str);
-	else
-		fputs(str, fp);
-	fclose(fp);
-}
-
-static int diff_file_fn(
-	void *cb_data,
-	const git_oid *old,
-	const char *old_path,
-	int old_mode,
-	const git_oid *new,
-	const char *new_path,
-	int new_mode)
-{
-	diff_expects *e = cb_data;
-	e->files++;
-	log("-- file --\n", 0);
-	return 0;
-}
-
-static int diff_hunk_fn(
-	void *cb_data,
-	int old_start,
-	int old_lines,
-	int new_start,
-	int new_lines)
-{
-	diff_expects *e = cb_data;
-	e->hunks++;
-	e->hunk_old_lines += old_lines;
-	e->hunk_new_lines += new_lines;
-	log("-- hunk --\n", 0);
-	return 0;
-}
-
-static int diff_line_fn(
-	void *cb_data,
-	int origin,
-	const char *content,
-	size_t content_len)
-{
-	diff_expects *e = cb_data;
-	e->lines++;
-	switch (origin) {
-	case GIT_DIFF_LINE_CONTEXT:
-		log("[ ]", 3);
-		e->line_ctxt++;
-		break;
-	case GIT_DIFF_LINE_ADDITION:
-		log("[+]", 3);
-		e->line_adds++;
-		break;
-	case GIT_DIFF_LINE_DELETION:
-		log("[-]", 3);
-		e->line_dels++;
-		break;
-	default:
-		cl_assert("Unknown diff line origin" == 0);
-	}
-	log(content, content_len);
-	return 0;
-}
-
-void test_diff_blob__0(void)
-{
-	int err;
-	git_blob *a, *b, *c, *d;
-	git_oid a_oid, b_oid, c_oid, d_oid;
-	git_diff_opts opts;
-	diff_expects exp;
-
-	/* tests/resources/attr/root_test1 */
-	cl_git_pass(git_oid_fromstrn(&a_oid, "45141a79", 8));
-	cl_git_pass(git_blob_lookup_prefix(&a, g_repo, &a_oid, 4));
-
-	/* tests/resources/attr/root_test2 */
-	cl_git_pass(git_oid_fromstrn(&b_oid, "4d713dc4", 8));
-	cl_git_pass(git_blob_lookup_prefix(&b, g_repo, &b_oid, 4));
-
-	/* tests/resources/attr/root_test3 */
-	cl_git_pass(git_oid_fromstrn(&c_oid, "c96bbb2c2557a832", 16));
-	cl_git_pass(git_blob_lookup_prefix(&c, g_repo, &c_oid, 8));
-
-	/* tests/resources/attr/root_test4.txt */
-	cl_git_pass(git_oid_fromstrn(&d_oid, "fe773770c5a6", 12));
-	cl_git_pass(git_blob_lookup_prefix(&d, g_repo, &d_oid, 6));
-
-	/* Doing the equivalent of a `diff -U 2` on these files */
-
-	opts.context_lines = 2;
-	opts.interhunk_lines = 0;
-	opts.ignore_whitespace = 0;
-	opts.file_cb = diff_file_fn;
-	opts.hunk_cb = diff_hunk_fn;
-	opts.line_cb = diff_line_fn;
-	opts.cb_data = &exp;
-
-	memset(&exp, 0, sizeof(exp));
-	cl_git_pass(git_diff_blobs(g_repo, a, b, &opts));
-
-	cl_assert(exp.files == 1);
-	cl_assert(exp.hunks == 1);
-	cl_assert(exp.lines == 6);
-	cl_assert(exp.line_ctxt == 1);
-	cl_assert(exp.line_adds == 5);
-	cl_assert(exp.line_dels == 0);
-
-	memset(&exp, 0, sizeof(exp));
-	cl_git_pass(git_diff_blobs(g_repo, b, c, &opts));
-
-	cl_assert(exp.files == 1);
-	cl_assert(exp.hunks == 1);
-	cl_assert(exp.lines == 15);
-	cl_assert(exp.line_ctxt == 3);
-	cl_assert(exp.line_adds == 9);
-	cl_assert(exp.line_dels == 3);
-
-	memset(&exp, 0, sizeof(exp));
-	cl_git_pass(git_diff_blobs(g_repo, a, c, &opts));
-
-	cl_assert(exp.files == 1);
-	cl_assert(exp.hunks == 1);
-	cl_assert(exp.lines == 13);
-	cl_assert(exp.line_ctxt == 0);
-	cl_assert(exp.line_adds == 12);
-	cl_assert(exp.line_dels == 1);
-
-	opts.context_lines = 2;
-
-	memset(&exp, 0, sizeof(exp));
-	cl_git_pass(git_diff_blobs(g_repo, c, d, &opts));
-
-	cl_assert(exp.files == 1);
-	cl_assert(exp.hunks == 2);
-	cl_assert(exp.lines == 16);
-	cl_assert(exp.line_ctxt == 6);
-	cl_assert(exp.line_adds == 6);
-	cl_assert(exp.line_dels == 4);
-
-	git_blob_free(a);
-	git_blob_free(b);
-	git_blob_free(c);
-}
-