Fix bugs in new diff patch code

This fixes all the bugs in the new diff patch code.  The only
really interesting one is that when we merge two diffs, we now
have to actually exclude diff delta records that are not supposed
to be tracked, as opposed to before where they could be included
because they would be skipped silently by `git_diff_foreach()`.
Other than that, there are just minor errors.

include/git2/diff.h

@@ -401,6 +401,20 @@ GIT_EXTERN(int) git_diff_print_compact(
 	git_diff_data_fn print_cb);
 
 /**
+ * Look up the single character abbreviation for a delta status code.
+ *
+ * When you call `git_diff_print_compact` it prints single letter codes into
+ * the output such as 'A' for added, 'D' for deleted, 'M' for modified, etc.
+ * It is sometimes convenient to convert a git_delta_t value into these
+ * letters for your own purposes.  This function does just that.  By the
+ * way, unmodified will return a space (i.e. ' ').
+ *
+ * @param delta_t The git_delta_t value to look up
+ * @return The single character label for that code
+ */
+GIT_EXTERN(char) git_diff_status_char(git_delta_t status);
+
+/**
  * Iterate over a diff generating text output like "git diff".
  *
  * This is a super easy way to generate a patch from a diff.
@@ -453,9 +467,9 @@ GIT_EXTERN(size_t) git_diff_num_deltas_of_type(
  * done with it.  You can use the patch object to loop over all the hunks
  * and lines in the diff of the one delta.
  *
- * For a binary file, no `git_diff_patch` will be created, the output will
- * be set to NULL, and the `binary` flag will be set true in the
- * `git_diff_delta` structure.
+ * For an unchanged file or a binary file, no `git_diff_patch` will be
+ * created, the output will be set to NULL, and the `binary` flag will be
+ * set true in the `git_diff_delta` structure.
  *
  * The `git_diff_delta` pointer points to internal data and you do not have
  * to release it when you are done with it.  It will go away when the

src/diff.c

@@ -807,6 +807,28 @@ on_error:
 	return -1;
 }
 
+
+bool git_diff_delta__should_skip(
+	git_diff_options *opts, git_diff_delta *delta)
+{
+	uint32_t flags = opts ? opts->flags : 0;
+
+	if (delta->status == GIT_DELTA_UNMODIFIED &&
+		(flags & GIT_DIFF_INCLUDE_UNMODIFIED) == 0)
+		return true;
+
+	if (delta->status == GIT_DELTA_IGNORED &&
+		(flags & GIT_DIFF_INCLUDE_IGNORED) == 0)
+		return true;
+
+	if (delta->status == GIT_DELTA_UNTRACKED &&
+		(flags & GIT_DIFF_INCLUDE_UNTRACKED) == 0)
+		return true;
+
+	return false;
+}
+
+
 int git_diff_merge(
 	git_diff_list *onto,
 	const git_diff_list *from)
@@ -843,6 +865,14 @@ int git_diff_merge(
 			j++;
 		}
 
+		/* the ignore rules for the target may not match the source
+		 * or the result of a merged delta could be skippable...
+		 */
+		if (git_diff_delta__should_skip(&onto->opts, delta)) {
+			git__free(delta);
+			continue;
+		}
+
 		if ((error = !delta ? -1 : git_vector_insert(&onto_new, delta)) < 0)
 			break;
 	}

src/diff.h

@@ -50,5 +50,8 @@ extern void git_diff__cleanup_modes(
 
 extern void git_diff_list_addref(git_diff_list *diff);
 
+extern bool git_diff_delta__should_skip(
+	git_diff_options *opts, git_diff_delta *delta);
+
 #endif
 

src/diff_output.c

@@ -51,26 +51,6 @@ static int parse_hunk_header(git_diff_range *range, const char *header)
 	return 0;
 }
 
-static bool diff_delta_should_skip(
-	diff_context *ctxt, git_diff_delta *delta)
-{
-	uint32_t flags = ctxt->opts ? ctxt->opts->flags : 0;
-
-	if (delta->status == GIT_DELTA_UNMODIFIED &&
-		(flags & GIT_DIFF_INCLUDE_UNMODIFIED) == 0)
-		return true;
-
-	if (delta->status == GIT_DELTA_IGNORED &&
-		(flags & GIT_DIFF_INCLUDE_IGNORED) == 0)
-		return true;
-
-	if (delta->status == GIT_DELTA_UNTRACKED &&
-		(flags & GIT_DIFF_INCLUDE_UNTRACKED) == 0)
-		return true;
-
-	return false;
-}
-
 #define KNOWN_BINARY_FLAGS (GIT_DIFF_FILE_BINARY|GIT_DIFF_FILE_NOT_BINARY)
 #define NOT_BINARY_FLAGS   (GIT_DIFF_FILE_NOT_BINARY|GIT_DIFF_FILE_NO_DATA)
 
@@ -411,7 +391,7 @@ static void diff_context_init(
 	setup_xdiff_options(ctxt->opts, &ctxt->xdiff_config, &ctxt->xdiff_params);
 }
 
-static bool diff_delta_file_callback(
+static int diff_delta_file_callback(
 	diff_context *ctxt, git_diff_delta *delta, size_t idx)
 {
 	float progress;
@@ -419,18 +399,18 @@ static bool diff_delta_file_callback(
 	if (!ctxt->file_cb)
 		return 0;
 
-	progress = (float)idx / ctxt->diff->deltas.length;
+	progress = ctxt->diff ? ((float)idx / ctxt->diff->deltas.length) : 1.0f;
 
 	if (ctxt->file_cb(ctxt->cb_data, delta, progress) != 0)
-		return GIT_EUSER;
+		ctxt->cb_error = GIT_EUSER;
 
-	return 0;
+	return ctxt->cb_error;
 }
 
 static void diff_patch_init(
 	diff_context *ctxt, git_diff_patch *patch)
 {
-	memset(patch, 0, sizeof(*patch));
+	memset(patch, 0, sizeof(git_diff_patch));
 
 	patch->diff = ctxt->diff;
 	patch->ctxt = ctxt;
@@ -454,7 +434,7 @@ static git_diff_patch *diff_patch_alloc(
 
 	git_diff_list_addref(patch->diff);
 
-	GIT_REFCOUNT_INC(&patch);
+	GIT_REFCOUNT_INC(patch);
 
 	patch->delta = delta;
 	patch->flags = GIT_DIFF_PATCH_ALLOCATED;
@@ -836,6 +816,8 @@ static int diff_patch_line_cb(
 		}
 	}
 
+	hunk->line_count++;
+
 	return 0;
 }
 
@@ -847,7 +829,7 @@ int git_diff_foreach(
 	git_diff_hunk_fn hunk_cb,
 	git_diff_data_fn data_cb)
 {
-	int error;
+	int error = 0;
 	diff_context ctxt;
 	size_t idx;
 	git_diff_patch patch;
@@ -861,14 +843,20 @@ int git_diff_foreach(
 	git_vector_foreach(&diff->deltas, idx, patch.delta) {
 
 		/* check flags against patch status */
-		if (diff_delta_should_skip(&ctxt, patch.delta))
+		if (git_diff_delta__should_skip(ctxt.opts, patch.delta))
 			continue;
 
-		if (!(error = diff_patch_load(&ctxt, &patch)) &&
-			!(error = diff_delta_file_callback(&ctxt, patch.delta, idx)))
-			error = diff_patch_generate(&ctxt, &patch);
+		if (!(error = diff_patch_load(&ctxt, &patch))) {
 
-		diff_patch_unload(&patch);
+			/* invoke file callback */
+			error = diff_delta_file_callback(&ctxt, patch.delta, idx);
+
+			/* generate diffs and invoke hunk and line callbacks */
+			if (!error)
+				error = diff_patch_generate(&ctxt, &patch);
+
+			diff_patch_unload(&patch);
+		}
 
 		if (error < 0)
 			break;
@@ -899,25 +887,32 @@ static char pick_suffix(int mode)
 		return ' ';
 }
 
+char git_diff_status_char(git_delta_t status)
+{
+	char code;
+
+	switch (status) {
+	case GIT_DELTA_ADDED:     code = 'A'; break;
+	case GIT_DELTA_DELETED:   code = 'D'; break;
+	case GIT_DELTA_MODIFIED:  code = 'M'; break;
+	case GIT_DELTA_RENAMED:   code = 'R'; break;
+	case GIT_DELTA_COPIED:    code = 'C'; break;
+	case GIT_DELTA_IGNORED:   code = 'I'; break;
+	case GIT_DELTA_UNTRACKED: code = '?'; break;
+	default:                  code = ' '; break;
+	}
+
+	return code;
+}
+
 static int print_compact(void *data, git_diff_delta *delta, float progress)
 {
 	diff_print_info *pi = data;
-	char code, old_suffix, new_suffix;
+	char old_suffix, new_suffix, code = git_diff_status_char(delta->status);
 
 	GIT_UNUSED(progress);
 
-	switch (delta->status) {
-	case GIT_DELTA_ADDED: code = 'A'; break;
-	case GIT_DELTA_DELETED: code = 'D'; break;
-	case GIT_DELTA_MODIFIED: code = 'M'; break;
-	case GIT_DELTA_RENAMED: code = 'R'; break;
-	case GIT_DELTA_COPIED: code = 'C'; break;
-	case GIT_DELTA_IGNORED: code = 'I'; break;
-	case GIT_DELTA_UNTRACKED: code = '?'; break;
-	default: code = 0;
-	}
-
-	if (!code)
+	if (code == ' ')
 		return 0;
 
 	old_suffix = pick_suffix(delta->old_file.mode);
@@ -1288,7 +1283,7 @@ int git_diff_get_patch(
 		&ctxt, diff, diff->repo, &diff->opts,
 		NULL, NULL, diff_patch_hunk_cb, diff_patch_line_cb);
 
-	if (diff_delta_should_skip(&ctxt, delta))
+	if (git_diff_delta__should_skip(ctxt.opts, delta))
 		return 0;
 
 	patch = diff_patch_alloc(&ctxt, delta);

tests-clar/diff/diff_helpers.c

@@ -120,7 +120,7 @@ int diff_foreach_via_iterator(
 		size_t h, num_h;
 
 		cl_git_pass(git_diff_get_patch(&patch, &delta, diff, d));
-		cl_assert(delta && patch);
+		cl_assert(delta);
 
 		/* call file_cb for this file */
 		if (file_cb != NULL && file_cb(data, delta, (float)d / num_d) != 0) {
@@ -128,6 +128,12 @@ int diff_foreach_via_iterator(
 			goto abort;
 		}
 
+		/* if there are no changes, then the patch will be NULL */
+		if (!patch) {
+			cl_assert(delta->status == GIT_DELTA_UNMODIFIED || delta->binary == 1);
+			continue;
+		}
+
 		if (!hunk_cb && !line_cb) {
 			git_diff_patch_free(patch);
 			continue;

tests-clar/diff/diffiter.c

@@ -256,21 +256,23 @@ void test_diff_diffiter__iterate_all(void)
 
 	cl_assert_equal_i(13, exp.files);
 	cl_assert_equal_i(8, exp.hunks);
-	cl_assert_equal_i(13, exp.lines);
+	cl_assert_equal_i(14, exp.lines);
 
 	git_diff_list_free(diff);
 }
 
 static void iterate_over_patch(git_diff_patch *patch, diff_expects *exp)
 {
-	size_t h, num_h = git_diff_patch_num_hunks(patch);
+	size_t h, num_h = git_diff_patch_num_hunks(patch), num_l;
 
 	exp->files++;
 	exp->hunks += num_h;
 
 	/* let's iterate in reverse, just because we can! */
-	for (h = 1; h <= num_h; ++h)
-		exp->lines += git_diff_patch_num_lines_in_hunk(patch, num_h - h);
+	for (h = 1, num_l = 0; h <= num_h; ++h)
+		num_l += git_diff_patch_num_lines_in_hunk(patch, num_h - h);
+
+	exp->lines += num_l;
 }
 
 #define PATCH_CACHE 5
@@ -338,5 +340,5 @@ void test_diff_diffiter__iterate_randomly_while_saving_state(void)
 	/* hopefully it all still added up right */
 	cl_assert_equal_i(13, exp.files);
 	cl_assert_equal_i(8, exp.hunks);
-	cl_assert_equal_i(13, exp.lines);
+	cl_assert_equal_i(14, exp.lines);
 }