"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Mark Jamsek <mark@jamsek.com>
Subject:
Re: refactor diffstat to compute diff once in got log -d and tog diff view
To:
Game of Trees <gameoftrees@openbsd.org>
Date:
Tue, 17 Jan 2023 18:19:43 +1100

Download raw body.

Thread
On 23-01-16 06:34PM, Stefan Sperling wrote:
> On Tue, Jan 17, 2023 at 01:13:20AM +1100, Mark Jamsek wrote:
> > I've also taken the liberty of making 'got log -d' behave like 'got diff
> > -d' such that the actual diff is displayed too. stsp made the point when
> > first reviewing got diff -d code that the diffstat per se isn't all that
> > meaningful--it's most useful in the context of the diff. Regress is
> > passing, but assuming this is ok, I'll follow up with a change to the
> > test_log_diffstat regress to include testing for diff output. Otherwise
> > I can revert this part of the diff and stick to just displaying the
> > diffstat with 'got log -d', in which case the current regress provides
> > sufficient coverage.
> 
> I would argue that a log message already provides good enough context
> for a diffstat. A commit log message which implies a large change but
> comes with a tiny-looking diffstat would stand out, and vice versa.
> The 'got diff -d' case was different since it lacked context entirely.
> 
> And users are already able to quickly combine both options as "diff -dp"
> if desired.
> 
> I see at least one possible use case for -d without -p style output:
> Consider post-commit email notifications, where a log message with a
> diffstat attached would provide a suitable amount of information, and
> won't blow up peoples' inboxes when someone imports a new version of
> a large vendor-imported subtree such as gcc or LLVM.
> Granted, this use case is a bit contrived right now (gotd has no hook scripts,
> and people running git-daemon would likely be using 'git log' to generate
> an email body). But it shows that 'log -d' by itself can be useful.

I agree on all counts; I wasn't too sure about that change so really
appreciate your feedback. Thanks!

Updated diff keeps existing behaviour for `got log -d` (i.e., only the
diffstat is displayed); however, it has been optimised slightly such
that when `got log -dp` is invoked, we now only compute the diff once.
The downside is there's no longer an appreciable net LOC decrease.

Nonetheless, this eliminates the double-diff-computation performance
cost in tog and 'got log' wrt displaying the diffstat with the diff :)

diffstat /home/mark/src/got
 M  got/got.1                 |   1+    1-
 M  got/got.c                 |  83+   68-
 M  gotwebd/got_operations.c  |   3+    3-
 M  include/got_diff.h        |   6+    7-
 M  lib/diff.c                |  30+   35-
 M  lib/worktree.c            |   2+    2-
 M  tog/tog.c                 |  95+  108-

7 files changed, 220 insertions(+), 224 deletions(-)

diff /home/mark/src/got
commit - 4a1a737306fe863c1d6378370d345fae962a2cad
path + /home/mark/src/got
blob - a694c0e1f36f3891e4f55c84802c58833a6445a1
file + got/got.1
--- got/got.1
+++ got/got.1
@@ -830,7 +830,7 @@ Display diffstat of changes introduced in the commit.
 If this option is not specified, default to the work tree's current branch
 if invoked in a work tree, or to the repository's HEAD reference.
 .It Fl d
-Display diffstat of changes introduced in the commit.
+Display diffstat of changes introduced in each commit.
 Cannot be used with the
 .Fl s
 option.
blob - e432b18620ea668642106a045956a1a96bef04bc
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -3610,8 +3610,8 @@ diff_blobs(struct got_object_id *blob_id1, struct got_
 static const struct got_error *
 diff_blobs(struct got_object_id *blob_id1, struct got_object_id *blob_id2,
     const char *path, int diff_context, int ignore_whitespace,
-    int force_text_diff, int show_diffstat, struct got_repository *repo,
-    FILE *outfile)
+    int force_text_diff, struct got_diffstat_cb_arg *dsa,
+    struct got_repository *repo, FILE *outfile)
 {
 	const struct got_error *err = NULL;
 	struct got_blob_object *blob1 = NULL, *blob2 = NULL;
@@ -3653,7 +3653,7 @@ diff_blobs(struct got_object_id *blob_id1, struct got_
 		path++;
 	err = got_diff_blob(NULL, NULL, blob1, blob2, f1, f2, path, path,
 	    GOT_DIFF_ALGORITHM_PATIENCE, diff_context, ignore_whitespace,
-	    force_text_diff, show_diffstat, NULL, outfile);
+	    force_text_diff, dsa, outfile);
 done:
 	if (fd1 != -1 && close(fd1) == -1 && err == NULL)
 		err = got_error_from_errno("close");
@@ -3672,7 +3672,8 @@ diff_trees(struct got_object_id *tree_id1, struct got_
 static const struct got_error *
 diff_trees(struct got_object_id *tree_id1, struct got_object_id *tree_id2,
     const char *path, int diff_context, int ignore_whitespace,
-    int force_text_diff, struct got_repository *repo, FILE *outfile)
+    int force_text_diff, struct got_diffstat_cb_arg *dsa,
+    struct got_repository *repo, FILE *outfile)
 {
 	const struct got_error *err = NULL;
 	struct got_tree_object *tree1 = NULL, *tree2 = NULL;
@@ -3714,8 +3715,7 @@ diff_trees(struct got_object_id *tree_id1, struct got_
 	arg.diff_context = diff_context;
 	arg.ignore_whitespace = ignore_whitespace;
 	arg.force_text_diff = force_text_diff;
-	arg.show_diffstat = 0;
-	arg.diffstat = NULL;
+	arg.diffstat = dsa;
 	arg.diff_algo = GOT_DIFF_ALGORITHM_PATIENCE;
 	arg.outfile = outfile;
 	arg.lines = NULL;
@@ -3828,8 +3828,8 @@ print_patch(struct got_commit_object *commit, struct g
 
 static const struct got_error *
 print_patch(struct got_commit_object *commit, struct got_object_id *id,
-    const char *path, int diff_context, struct got_repository *repo,
-    FILE *outfile)
+    const char *path, int diff_context, struct got_diffstat_cb_arg *dsa,
+    struct got_repository *repo, FILE *outfile)
 {
 	const struct got_error *err = NULL;
 	struct got_commit_object *pcommit = NULL;
@@ -3880,11 +3880,11 @@ print_patch(struct got_commit_object *commit, struct g
 		switch (obj_type) {
 		case GOT_OBJ_TYPE_BLOB:
 			err = diff_blobs(obj_id1, obj_id2, path, diff_context,
-			    0, 0, 0, repo, outfile);
+			    0, 0, dsa, repo, outfile);
 			break;
 		case GOT_OBJ_TYPE_TREE:
 			err = diff_trees(obj_id1, obj_id2, path, diff_context,
-			    0, 0, repo, outfile);
+			    0, 0, dsa, repo, outfile);
 			break;
 		default:
 			err = got_error(GOT_ERR_OBJ_TYPE);
@@ -3902,7 +3902,7 @@ print_patch(struct got_commit_object *commit, struct g
 		    id_str1 ? id_str1 : "/dev/null");
 		fprintf(outfile, "commit + %s\n", id_str2);
 		err = diff_trees(obj_id1, obj_id2, "", diff_context, 0, 0,
-		    repo, outfile);
+		    dsa, repo, outfile);
 	}
 done:
 	free(id_str1);
@@ -3994,7 +3994,7 @@ match_patch(int *have_match, struct got_commit_object 
 	if (err)
 		return err;
 
-	err = print_patch(commit, id, path, diff_context, repo, f);
+	err = print_patch(commit, id, path, diff_context, NULL, repo, f);
 	if (err)
 		goto done;
 
@@ -4153,15 +4153,14 @@ print_diffstat(struct got_diffstat_cb_arg *dsa, struct
 }
 
 static const struct got_error *
-print_diffstat(struct got_diffstat_cb_arg *dsa, struct got_pathlist_head *paths,
-    const char *header)
+print_diffstat(struct got_diffstat_cb_arg *dsa, const char *header)
 {
 	struct got_pathlist_entry *pe;
 
 	if (header != NULL)
 		printf("%s\n", header);
 
-	TAILQ_FOREACH(pe, paths, entry) {
+	TAILQ_FOREACH(pe, dsa->paths, entry) {
 		struct got_diff_changed_path *cp = pe->data;
 		int pad = dsa->max_path_len - pe->path_len + 1;
 
@@ -4179,13 +4178,38 @@ print_commit(struct got_commit_object *commit, struct 
 }
 
 static const struct got_error *
+printfile(FILE *f)
+{
+	char	buf[8192];
+	size_t	r;
+
+	if (fseeko(f, 0L, SEEK_SET) == -1)
+		return got_error_from_errno("fseek");
+
+	for (;;) {
+		r = fread(buf, 1, sizeof(buf), f);
+		if (r == 0) {
+			if (ferror(f))
+				return got_error_from_errno("fread");
+			if (feof(f))
+				break;
+		}
+		if (fwrite(buf, 1, r, stdout) != r)
+			return got_ferror(stdout, GOT_ERR_IO);
+	}
+
+	return NULL;
+}
+
+static const struct got_error *
 print_commit(struct got_commit_object *commit, struct got_object_id *id,
     struct got_repository *repo, const char *path,
-    struct got_pathlist_head *changed_paths, struct got_diffstat_cb_arg *dsa,
-    int show_patch, int diff_context,
+    struct got_pathlist_head *changed_paths,
+    struct got_diffstat_cb_arg *diffstat, int show_patch, int diff_context,
     struct got_reflist_object_id_map *refs_idmap, const char *custom_refs_str)
 {
 	const struct got_error *err = NULL;
+	FILE *f = NULL;
 	char *id_str, *datestr, *logmsg0, *logmsg, *line;
 	char datebuf[26];
 	time_t committer_time;
@@ -4252,11 +4276,7 @@ print_commit(struct got_commit_object *commit, struct 
 	} while (line);
 	free(logmsg0);
 
-	if (dsa && changed_paths) {
-		err = print_diffstat(dsa, changed_paths, NULL);
-		if (err)
-			goto done;
-	} else if (changed_paths) {
+	if (changed_paths && diffstat == NULL) {
 		struct got_pathlist_entry *pe;
 
 		TAILQ_FOREACH(pe, changed_paths, entry) {
@@ -4267,14 +4287,37 @@ print_commit(struct got_commit_object *commit, struct 
 		printf("\n");
 	}
 	if (show_patch) {
-		err = print_patch(commit, id, path, diff_context, repo, stdout);
-		if (err == 0)
-			printf("\n");
+		if (diffstat) {
+			f = got_opentemp();
+			if (f == NULL) {
+				err = got_error_from_errno("got_opentemp");
+				goto done;
+			}
+		}
+
+		err = print_patch(commit, id, path, diff_context, diffstat,
+		    repo, diffstat == NULL ? stdout : f);
+		if (err)
+			goto done;
 	}
+	if (diffstat) {
+		err = print_diffstat(diffstat, NULL);
+		if (err)
+			goto done;
+		if (show_patch) {
+			err = printfile(f);
+			if (err)
+				goto done;
+		}
+	}
+	if (show_patch)
+		printf("\n");
 
 	if (fflush(stdout) != 0 && err == NULL)
 		err = got_error_from_errno("fflush");
 done:
+	if (f && fclose(f) == EOF && err == NULL)
+		err = got_error_from_errno("fclose");
 	free(id_str);
 	free(refs_str);
 	return err;
@@ -4331,8 +4374,8 @@ print_commits(struct got_object_id *root_id, struct go
 		if (err)
 			break;
 
-		if ((show_changed_paths || show_diffstat) &&
-		    !reverse_display_order) {
+		if ((show_changed_paths || (show_diffstat && !show_patch))
+		    && !reverse_display_order) {
 			err = get_changed_paths(&changed_paths, commit, repo,
 			    show_diffstat ? &dsa : NULL);
 			if (err)
@@ -4350,8 +4393,7 @@ print_commits(struct got_object_id *root_id, struct go
 				    &changed_paths, &regex);
 			if (have_match == 0 && show_patch) {
 				err = match_patch(&have_match, commit, &id,
-				    path, diff_context, repo, &regex,
-				    tmpfile);
+				    path, diff_context, repo, &regex, tmpfile);
 				if (err)
 					break;
 			}
@@ -4398,9 +4440,10 @@ print_commits(struct got_object_id *root_id, struct go
 			    &qid->id);
 			if (err)
 				break;
-			if (show_changed_paths || show_diffstat) {
-				err = get_changed_paths(&changed_paths,
-				    commit, repo, show_diffstat ? &dsa : NULL);
+			if (show_changed_paths ||
+			    (show_diffstat && !show_patch)) {
+				err = get_changed_paths(&changed_paths, commit,
+				    repo, show_diffstat ? &dsa : NULL);
 				if (err)
 					break;
 			}
@@ -4730,7 +4773,6 @@ struct print_diff_arg {
 	enum got_diff_algorithm diff_algo;
 	int ignore_whitespace;
 	int force_text_diff;
-	int show_diffstat;
 	FILE *f1;
 	FILE *f2;
 	FILE *outfile;
@@ -4873,8 +4915,7 @@ print_diff(void *arg, unsigned char status, unsigned c
 		err = got_diff_objects_as_blobs(NULL, NULL, a->f1, a->f2,
 		    fd1, fd2, blob_id, staged_blob_id, label1, label2,
 		    a->diff_algo, a->diff_context, a->ignore_whitespace,
-		    a->force_text_diff, a->show_diffstat, a->diffstat, a->repo,
-		    a->outfile);
+		    a->force_text_diff, a->diffstat, a->repo, a->outfile);
 		goto done;
 	}
 
@@ -4964,8 +5005,7 @@ print_diff(void *arg, unsigned char status, unsigned c
 
 	err = got_diff_blob_file(blob1, a->f1, size1, label1, f2 ? f2 : a->f2,
 	    f2_exists, &sb, path, GOT_DIFF_ALGORITHM_PATIENCE, a->diff_context,
-	    a->ignore_whitespace, a->force_text_diff, a->show_diffstat,
-	    a->diffstat, a->outfile);
+	    a->ignore_whitespace, a->force_text_diff, a->diffstat, a->outfile);
 done:
 	if (fd1 != -1 && close(fd1) == -1 && err == NULL)
 		err = got_error_from_errno("close");
@@ -4982,30 +5022,6 @@ printfile(FILE *f)
 }
 
 static const struct got_error *
-printfile(FILE *f)
-{
-	char	buf[8192];
-	size_t	r;
-
-	if (fseeko(f, 0L, SEEK_SET) == -1)
-		return got_error_from_errno("fseek");
-
-	for (;;) {
-		r = fread(buf, 1, sizeof(buf), f);
-		if (r == 0) {
-			if (ferror(f))
-				return got_error_from_errno("fread");
-			if (feof(f))
-				break;
-		}
-		if (fwrite(buf, 1, r, stdout) != r)
-			return got_ferror(stdout, GOT_ERR_IO);
-	}
-
-	return NULL;
-}
-
-static const struct got_error *
 cmd_diff(int argc, char *argv[])
 {
 	const struct got_error *error;
@@ -5233,8 +5249,7 @@ cmd_diff(int argc, char *argv[])
 		arg.diff_staged = diff_staged;
 		arg.ignore_whitespace = ignore_whitespace;
 		arg.force_text_diff = force_text_diff;
-		arg.show_diffstat = show_diffstat;
-		arg.diffstat = &dsa;
+		arg.diffstat = show_diffstat ? &dsa : NULL;
 		arg.f1 = f1;
 		arg.f2 = f2;
 		arg.outfile = outfile;
@@ -5255,7 +5270,7 @@ cmd_diff(int argc, char *argv[])
 				goto done;
 			}
 
-			error = print_diffstat(&dsa, &diffstat_paths, header);
+			error = print_diffstat(&dsa, header);
 			free(header);
 			if (error)
 				goto done;
@@ -5396,14 +5411,14 @@ cmd_diff(int argc, char *argv[])
 		error = got_diff_objects_as_blobs(NULL, NULL, f1, f2,
 		    fd1, fd2, ids[0], ids[1], NULL, NULL,
 		    GOT_DIFF_ALGORITHM_PATIENCE, diff_context,
-		    ignore_whitespace, force_text_diff, show_diffstat,
+		    ignore_whitespace, force_text_diff,
 		    show_diffstat ? &dsa : NULL, repo, outfile);
 		break;
 	case GOT_OBJ_TYPE_TREE:
 		error = got_diff_objects_as_trees(NULL, NULL, f1, f2, fd1, fd2,
 		    ids[0], ids[1], &paths, "", "",
 		    GOT_DIFF_ALGORITHM_PATIENCE, diff_context,
-		    ignore_whitespace, force_text_diff, show_diffstat,
+		    ignore_whitespace, force_text_diff,
 		    show_diffstat ? &dsa : NULL, repo, outfile);
 		break;
 	case GOT_OBJ_TYPE_COMMIT:
@@ -5411,7 +5426,7 @@ cmd_diff(int argc, char *argv[])
 		error = got_diff_objects_as_commits(NULL, NULL, f1, f2,
 		    fd1, fd2, ids[0], ids[1], &paths,
 		    GOT_DIFF_ALGORITHM_PATIENCE, diff_context,
-		    ignore_whitespace, force_text_diff, show_diffstat,
+		    ignore_whitespace, force_text_diff,
 		    show_diffstat ? &dsa : NULL, repo, outfile);
 		break;
 	default:
@@ -5429,7 +5444,7 @@ cmd_diff(int argc, char *argv[])
 			goto done;
 		}
 
-		error = print_diffstat(&dsa, &diffstat_paths, header);
+		error = print_diffstat(&dsa, header);
 		free(header);
 		if (error)
 			goto done;
blob - 63fc429b8a15be040dbfbb4a339c47974a9d29e6
file + gotwebd/got_operations.c
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -1290,17 +1290,17 @@ got_open_diff_for_output(FILE **fp, int *fd, struct re
 	case GOT_OBJ_TYPE_BLOB:
 		error = got_diff_objects_as_blobs(NULL, NULL, f1, f2, fd4, fd5,
 		     id1, id2, NULL, NULL, GOT_DIFF_ALGORITHM_MYERS, 3, 0, 0,
-		     0, NULL, repo, f3);
+		     NULL, repo, f3);
 		break;
 	case GOT_OBJ_TYPE_TREE:
 		error = got_diff_objects_as_trees(NULL, NULL, f1, f2, fd4, fd5,
 		    id1, id2, NULL, "", "",  GOT_DIFF_ALGORITHM_MYERS, 3, 0, 0,
-		    0, NULL, repo, f3);
+		    NULL, repo, f3);
 		break;
 	case GOT_OBJ_TYPE_COMMIT:
 		error = got_diff_objects_as_commits(NULL, NULL, f1, f2, fd4,
 		    fd5, id1, id2, NULL,  GOT_DIFF_ALGORITHM_MYERS, 3, 0, 0,
-		    0, NULL, repo, f3);
+		    NULL, repo, f3);
 		break;
 	default:
 		error = got_error(GOT_ERR_OBJ_TYPE);
blob - ba881aed5067fd49ac9a5f284d1df97cc01b1def
file + include/got_diff.h
--- include/got_diff.h
+++ include/got_diff.h
@@ -63,7 +63,7 @@ const struct got_error *got_diff_blob(struct got_diff_
  */
 const struct got_error *got_diff_blob(struct got_diff_line **, size_t *,
     struct got_blob_object *, struct got_blob_object *, FILE *, FILE *,
-    const char *, const char *, enum got_diff_algorithm, int, int, int, int,
+    const char *, const char *, enum got_diff_algorithm, int, int, int,
     struct got_diffstat_cb_arg *, FILE *);
 
 /*
@@ -77,7 +77,7 @@ const struct got_error *got_diff_blob_file(struct got_
  */
 const struct got_error *got_diff_blob_file(struct got_blob_object *, FILE *,
     off_t, const char *, FILE *, int, struct stat *, const char *,
-    enum got_diff_algorithm, int, int, int, int, struct got_diffstat_cb_arg *,
+    enum got_diff_algorithm, int, int, int, struct got_diffstat_cb_arg *,
     FILE *);
 
 /*
@@ -108,8 +108,7 @@ struct got_diff_blob_output_unidiff_arg {
 	int diff_context;	/* Sets the number of context lines. */
 	int ignore_whitespace;	/* Ignore whitespace differences. */
 	int force_text_diff;	/* Assume text even if binary data detected. */
-	int show_diffstat;	/* Compute diffstat of changes */
-	struct got_diffstat_cb_arg *diffstat;
+	struct got_diffstat_cb_arg *diffstat; /* Compute diffstat of changes */
 	enum got_diff_algorithm diff_algo; /* Diffing algorithm to use. */
 
 	/*
@@ -209,7 +208,7 @@ const struct got_error *got_diff_objects_as_blobs(stru
 const struct got_error *got_diff_objects_as_blobs(struct got_diff_line **,
     size_t *, FILE *, FILE *, int, int, struct got_object_id *,
     struct got_object_id *, const char *, const char *, enum got_diff_algorithm,
-    int, int, int, int, struct got_diffstat_cb_arg *, struct got_repository *,
+    int, int, int, struct got_diffstat_cb_arg *, struct got_repository *,
     FILE *);
 
 struct got_pathlist_head;
@@ -232,7 +231,7 @@ const struct got_error *got_diff_objects_as_trees(stru
 const struct got_error *got_diff_objects_as_trees(struct got_diff_line **,
     size_t *, FILE *, FILE *, int, int, struct got_object_id *,
     struct got_object_id *, struct got_pathlist_head *, const char *,
-    const char *, enum got_diff_algorithm, int, int, int, int,
+    const char *, enum got_diff_algorithm, int, int, int,
     struct got_diffstat_cb_arg *, struct got_repository *, FILE *);
 
 /*
@@ -250,7 +249,7 @@ const struct got_error *got_diff_objects_as_commits(st
 const struct got_error *got_diff_objects_as_commits(struct got_diff_line **,
     size_t *, FILE *, FILE *, int, int, struct got_object_id *,
     struct got_object_id *, struct got_pathlist_head *, enum got_diff_algorithm,
-    int, int, int, int, struct got_diffstat_cb_arg *, struct got_repository *,
+    int, int, int, struct got_diffstat_cb_arg *, struct got_repository *,
     FILE *);
 
 #define GOT_DIFF_MAX_CONTEXT	64
blob - 6f9c397cb5beed5d6de4ed2b82014650732b3318
file + lib/diff.c
--- lib/diff.c
+++ lib/diff.c
@@ -132,7 +132,7 @@ diff_blobs(struct got_diff_line **lines, size_t *nline
     struct got_blob_object *blob2, FILE *f1, FILE *f2,
     const char *label1, const char *label2, mode_t mode1, mode_t mode2,
     int diff_context, int ignore_whitespace, int force_text_diff,
-    int show_diffstat, struct got_diffstat_cb_arg *ds, FILE *outfile,
+    struct got_diffstat_cb_arg *diffstat, FILE *outfile,
     enum got_diff_algorithm diff_algo)
 {
 	const struct got_error *err = NULL, *free_err;
@@ -242,21 +242,20 @@ diff_blobs(struct got_diff_line **lines, size_t *nline
 	if (err)
 		goto done;
 
-	if (show_diffstat) {
+	if (diffstat) {
 		char	*path = NULL;
 		int	 status = GOT_STATUS_NO_CHANGE;
 
-		/*
-		 * Ignore 'm'ode status change: if there's no accompanying
-		 * content change, there'll be no diffstat, and if there
-		 * are actual changes, 'M'odified takes precedence.
-		 */
 		if (blob1 == NULL)
 			status = GOT_STATUS_ADD;
 		else if (blob2 == NULL)
 			status = GOT_STATUS_DELETE;
-		else
-			status = GOT_STATUS_MODIFY;
+		else {
+			if (strcmp(idstr1, idstr2) != 0)
+				status = GOT_STATUS_MODIFY;
+			else if (mode1 != mode2)
+				status = GOT_STATUS_MODE_CHANGE;
+		}
 
 		if (label1 == NULL && label2 == NULL) {
 			/* diffstat of blobs, show hash instead of path */
@@ -277,8 +276,8 @@ diff_blobs(struct got_diff_line **lines, size_t *nline
 			}
 		}
 
-		err = get_diffstat(ds, path, result->result, force_text_diff,
-		    status);
+		err = get_diffstat(diffstat, path, result->result,
+		    force_text_diff, status);
 		if (err) {
 			free(path);
 			goto done;
@@ -320,8 +319,8 @@ got_diff_blob_output_unidiff(void *arg, struct got_blo
 
 	return diff_blobs(&a->lines, &a->nlines, NULL,
 	    blob1, blob2, f1, f2, label1, label2, mode1, mode2, a->diff_context,
-	    a->ignore_whitespace, a->force_text_diff, a->show_diffstat,
-	    a->diffstat, a->outfile, a->diff_algo);
+	    a->ignore_whitespace, a->force_text_diff, a->diffstat, a->outfile,
+	    a->diff_algo);
 }
 
 const struct got_error *
@@ -329,12 +328,12 @@ got_diff_blob(struct got_diff_line **lines, size_t*nli
     struct got_blob_object *blob1, struct got_blob_object *blob2,
     FILE *f1, FILE *f2, const char *label1, const char *label2,
     enum got_diff_algorithm diff_algo, int diff_context,
-    int ignore_whitespace, int force_text_diff, int show_diffstat,
+    int ignore_whitespace, int force_text_diff,
     struct got_diffstat_cb_arg *ds, FILE *outfile)
 {
 	return diff_blobs(lines, nlines, NULL, blob1, blob2, f1, f2,
 	    label1, label2, 0, 0, diff_context, ignore_whitespace,
-	    force_text_diff, show_diffstat, ds, outfile, diff_algo);
+	    force_text_diff, ds, outfile, diff_algo);
 }
 
 static const struct got_error *
@@ -342,8 +341,7 @@ diff_blob_file(struct got_diffreg_result **resultp,
     struct got_blob_object *blob1, FILE *f1, off_t size1, const char *label1,
     FILE *f2, int f2_exists, struct stat *sb2, const char *label2,
     enum got_diff_algorithm diff_algo, int diff_context, int ignore_whitespace,
-    int force_text_diff, int show_diffstat, struct got_diffstat_cb_arg *ds,
-    FILE *outfile)
+    int force_text_diff, struct got_diffstat_cb_arg *diffstat, FILE *outfile)
 {
 	const struct got_error *err = NULL, *free_err;
 	char hex1[SHA1_DIGEST_STRING_LENGTH];
@@ -392,7 +390,7 @@ diff_blob_file(struct got_diffreg_result **resultp,
 			goto done;
 	}
 
-	if (show_diffstat) {
+	if (diffstat) {
 		char	*path = NULL;
 		int	 status = GOT_STATUS_NO_CHANGE;
 
@@ -418,8 +416,8 @@ diff_blob_file(struct got_diffreg_result **resultp,
 			goto done;
 		}
 
-		err = get_diffstat(ds, path, result->result, force_text_diff,
-		    status);
+		err = get_diffstat(diffstat, path, result->result,
+		    force_text_diff, status);
 		if (err) {
 			free(path);
 			goto done;
@@ -441,12 +439,12 @@ got_diff_blob_file(struct got_blob_object *blob1, FILE
 got_diff_blob_file(struct got_blob_object *blob1, FILE *f1, off_t size1,
     const char *label1, FILE *f2, int f2_exists, struct stat *sb2,
     const char *label2, enum got_diff_algorithm diff_algo, int diff_context,
-    int ignore_whitespace, int force_text_diff, int show_diffstat,
+    int ignore_whitespace, int force_text_diff,
     struct got_diffstat_cb_arg *ds, FILE *outfile)
 {
 	return diff_blob_file(NULL, blob1, f1, size1, label1, f2, f2_exists,
 	    sb2, label2, diff_algo, diff_context, ignore_whitespace,
-	    force_text_diff, show_diffstat, ds, outfile);
+	    force_text_diff, ds, outfile);
 }
 
 static const struct got_error *
@@ -995,8 +993,8 @@ got_diff_objects_as_blobs(struct got_diff_line **lines
     struct got_object_id *id1, struct got_object_id *id2,
     const char *label1, const char *label2,
     enum got_diff_algorithm diff_algo, int diff_context,
-    int ignore_whitespace, int force_text_diff, int show_diffstat,
-    struct got_diffstat_cb_arg *ds, struct got_repository *repo, FILE *outfile)
+    int ignore_whitespace, int force_text_diff, struct got_diffstat_cb_arg *ds,
+    struct got_repository *repo, FILE *outfile)
 {
 	const struct got_error *err;
 	struct got_blob_object *blob1 = NULL, *blob2 = NULL;
@@ -1016,7 +1014,7 @@ got_diff_objects_as_blobs(struct got_diff_line **lines
 	}
 	err = got_diff_blob(lines, nlines, blob1, blob2, f1, f2, label1, label2,
 	    diff_algo, diff_context, ignore_whitespace, force_text_diff,
-	    show_diffstat, ds, outfile);
+	    ds, outfile);
 done:
 	if (blob1)
 		got_object_blob_close(blob1);
@@ -1190,9 +1188,8 @@ diff_objects_as_trees(struct got_diff_line **lines, si
     struct got_object_id *id1, struct got_object_id *id2,
     struct got_pathlist_head *paths, const char *label1, const char *label2,
     int diff_context, int ignore_whitespace, int force_text_diff,
-    int show_diffstat, struct got_diffstat_cb_arg *dsa,
-    struct got_repository *repo, FILE *outfile,
-    enum got_diff_algorithm diff_algo)
+    struct got_diffstat_cb_arg *dsa, struct got_repository *repo,
+    FILE *outfile, enum got_diff_algorithm diff_algo)
 {
 	const struct got_error *err;
 	struct got_tree_object *tree1 = NULL, *tree2 = NULL;
@@ -1217,7 +1214,6 @@ diff_objects_as_trees(struct got_diff_line **lines, si
 	arg.diff_context = diff_context;
 	arg.ignore_whitespace = ignore_whitespace;
 	arg.force_text_diff = force_text_diff;
-	arg.show_diffstat = show_diffstat;
 	arg.diffstat = dsa;
 	arg.outfile = outfile;
 	if (want_linemeta) {
@@ -1251,7 +1247,7 @@ got_diff_objects_as_trees(struct got_diff_line **lines
     struct got_object_id *id1, struct got_object_id *id2,
     struct got_pathlist_head *paths, const char *label1, const char *label2,
     enum got_diff_algorithm diff_algo, int diff_context, int ignore_whitespace,
-    int force_text_diff, int show_diffstat, struct got_diffstat_cb_arg *dsa,
+    int force_text_diff, struct got_diffstat_cb_arg *dsa,
     struct got_repository *repo, FILE *outfile)
 {
 	const struct got_error *err;
@@ -1294,7 +1290,7 @@ got_diff_objects_as_trees(struct got_diff_line **lines
 
 	err = diff_objects_as_trees(lines, nlines, f1, f2, fd1, fd2, id1, id2,
 	    paths, label1, label2, diff_context, ignore_whitespace,
-	    force_text_diff, show_diffstat, dsa, repo, outfile, diff_algo);
+	    force_text_diff, dsa, repo, outfile, diff_algo);
 done:
 	free(idstr);
 	return err;
@@ -1306,8 +1302,7 @@ got_diff_objects_as_commits(struct got_diff_line **lin
     struct got_object_id *id1, struct got_object_id *id2,
     struct got_pathlist_head *paths, enum got_diff_algorithm diff_algo,
     int diff_context, int ignore_whitespace, int force_text_diff,
-    int show_diffstat, struct got_diffstat_cb_arg *dsa,
-    struct got_repository *repo, FILE *outfile)
+    struct got_diffstat_cb_arg *dsa, struct got_repository *repo, FILE *outfile)
 {
 	const struct got_error *err;
 	struct got_commit_object *commit1 = NULL, *commit2 = NULL;
@@ -1350,8 +1345,8 @@ got_diff_objects_as_commits(struct got_diff_line **lin
 	err = diff_objects_as_trees(lines, nlines, f1, f2, fd1, fd2,
 	    commit1 ? got_object_commit_get_tree_id(commit1) : NULL,
 	    got_object_commit_get_tree_id(commit2), paths, "", "",
-	    diff_context, ignore_whitespace, force_text_diff, show_diffstat,
-	    dsa, repo, outfile, diff_algo);
+	    diff_context, ignore_whitespace, force_text_diff, dsa, repo,
+	    outfile, diff_algo);
 done:
 	if (commit1)
 		got_object_commit_close(commit1);
blob - 58188a9add771b65d02a736d15bc55f34425aba4
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -5073,7 +5073,7 @@ append_ct_diff(struct got_commitable *ct, int *diff_he
 		}
 		err = got_diff_objects_as_blobs(NULL, NULL, f1, f2,
 		    fd1, fd2, ct->base_blob_id, ct->staged_blob_id,
-		    label1, label2, GOT_DIFF_ALGORITHM_PATIENCE, 3, 0, 0, 0,
+		    label1, label2, GOT_DIFF_ALGORITHM_PATIENCE, 3, 0, 0,
 		    NULL, repo, diff_outfile);
 		goto done;
 	}
@@ -5144,7 +5144,7 @@ append_ct_diff(struct got_commitable *ct, int *diff_he
 
 	err = got_diff_blob_file(blob1, f1, size1, label1,
 	    ondisk_file ? ondisk_file : f2, f2_exists, &sb, ct->path,
-	    GOT_DIFF_ALGORITHM_PATIENCE, 3, 0, 0, 0, NULL, diff_outfile);
+	    GOT_DIFF_ALGORITHM_PATIENCE, 3, 0, 0, NULL, diff_outfile);
 done:
 	if (fd1 != -1 && close(fd1) == -1 && err == NULL)
 		err = got_error_from_errno("close");
blob - 8766355007a2f2ff1724f384dc2bae3e21660b51
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -4460,88 +4460,6 @@ get_changed_paths(struct got_pathlist_head *paths,
 }
 
 static const struct got_error *
-get_changed_paths(struct got_pathlist_head *paths,
-    struct got_commit_object *commit, struct got_repository *repo,
-    struct got_diffstat_cb_arg *dsa)
-{
-	const struct got_error *err = NULL;
-	struct got_object_id *tree_id1 = NULL, *tree_id2 = NULL;
-	struct got_tree_object *tree1 = NULL, *tree2 = NULL;
-	struct got_object_qid *qid;
-	FILE *f1 = NULL, *f2 = NULL;
-	int fd1 = -1, fd2 = -1;
-
-	f1 = got_opentemp();
-	if (f1 == NULL) {
-		err = got_error_from_errno("got_opentemp");
-		goto done;
-	}
-	f2 = got_opentemp();
-	if (f2 == NULL) {
-		err = got_error_from_errno("got_opentemp");
-		goto done;
-	}
-
-	fd1 = got_opentempfd();
-	if (fd1 == -1) {
-		err = got_error_from_errno("got_opentempfd");
-		goto done;
-	}
-	fd2 = got_opentempfd();
-	if (fd2 == -1) {
-		err = got_error_from_errno("got_opentempfd");
-		goto done;
-	}
-
-	qid = STAILQ_FIRST(got_object_commit_get_parent_ids(commit));
-	if (qid != NULL) {
-		struct got_commit_object *pcommit;
-		err = got_object_open_as_commit(&pcommit, repo,
-		    &qid->id);
-		if (err)
-			return err;
-
-		tree_id1 = got_object_id_dup(
-		    got_object_commit_get_tree_id(pcommit));
-		if (tree_id1 == NULL) {
-			got_object_commit_close(pcommit);
-			return got_error_from_errno("got_object_id_dup");
-		}
-		got_object_commit_close(pcommit);
-
-	}
-
-	if (tree_id1) {
-		err = got_object_open_as_tree(&tree1, repo, tree_id1);
-		if (err)
-			goto done;
-	}
-
-	tree_id2 = got_object_commit_get_tree_id(commit);
-	err = got_object_open_as_tree(&tree2, repo, tree_id2);
-	if (err)
-		goto done;
-
-	err = got_diff_tree(tree1, tree2, f1, f2, fd1, fd2, "", "", repo,
-	    got_diff_tree_compute_diffstat, dsa, 1);
-done:
-	if (tree1)
-		got_object_tree_close(tree1);
-	if (tree2)
-		got_object_tree_close(tree2);
-	if (fd1 != -1 && close(fd1) == -1 && err == NULL)
-		err = got_error_from_errno("close");
-	if (fd2 != -1 && close(fd2) == -1 && err == NULL)
-		err = got_error_from_errno("close");
-	if (f1 && fclose(f1) == EOF && err == NULL)
-		err = got_error_from_errno("fclose");
-	if (f2 && fclose(f2) == EOF && err == NULL)
-		err = got_error_from_errno("fclose");
-	free(tree_id1);
-	return err;
-}
-
-static const struct got_error *
 add_line_metadata(struct got_diff_line **lines, size_t *nlines,
     off_t off, uint8_t type)
 {
@@ -4559,10 +4477,56 @@ write_commit_info(struct got_diff_line **lines, size_t
 }
 
 static const struct got_error *
+cat_diff(FILE *dst, FILE *src, struct got_diff_line **d_lines, size_t *d_nlines,
+    struct got_diff_line *s_lines, size_t s_nlines)
+{
+	struct got_diff_line	*p;
+	char			 buf[BUFSIZ];
+	size_t			 i, r;
+
+	if (fseeko(src, 0L, SEEK_SET) == -1)
+		return got_error_from_errno("fseeko");
+
+	for (;;) {
+		r = fread(buf, 1, sizeof(buf), src);
+		if (r == 0) {
+			if (ferror(src))
+				return got_error_from_errno("fread");
+			if (feof(src))
+				break;
+		}
+		if (fwrite(buf, 1, r, dst) != r)
+			return got_ferror(dst, GOT_ERR_IO);
+	}
+
+	/*
+	 * The diff driver initialises the first line at offset zero when the
+	 * array isn't prepopulated, skip it; we already have it in *d_lines.
+	 */
+	for (i = 1; i < s_nlines; ++i)
+		s_lines[i].offset += (*d_lines)[*d_nlines - 1].offset;
+
+	--s_nlines;
+
+	p = reallocarray(*d_lines, *d_nlines + s_nlines, sizeof(*p));
+	if (p == NULL) {
+		/* d_lines is freed in close_diff_view() */
+		return got_error_from_errno("reallocarray");
+	}
+
+	*d_lines = p;
+
+	memcpy(*d_lines + *d_nlines, s_lines + 1, s_nlines * sizeof(*s_lines));
+	*d_nlines += s_nlines;
+
+	return NULL;
+}
+
+static const struct got_error *
 write_commit_info(struct got_diff_line **lines, size_t *nlines,
     struct got_object_id *commit_id, struct got_reflist_head *refs,
     struct got_repository *repo, int ignore_ws, int force_text_diff,
-    FILE *outfile)
+    struct got_diffstat_cb_arg *dsa, FILE *outfile)
 {
 	const struct got_error *err = NULL;
 	char datebuf[26], *datestr;
@@ -4571,15 +4535,10 @@ write_commit_info(struct got_diff_line **lines, size_t
 	time_t committer_time;
 	const char *author, *committer;
 	char *refs_str = NULL;
-	struct got_pathlist_head changed_paths;
 	struct got_pathlist_entry *pe;
-	struct got_diffstat_cb_arg dsa = { 0, 0, 0, 0, 0, 0, &changed_paths,
-	    ignore_ws, force_text_diff, tog_diff_algo };
 	off_t outoff = 0;
 	int n;
 
-	TAILQ_INIT(&changed_paths);
-
 	if (refs) {
 		err = build_refs_str(&refs_str, refs, commit_id, repo);
 		if (err)
@@ -4691,17 +4650,13 @@ write_commit_info(struct got_diff_line **lines, size_t
 			goto done;
 	}
 
-	err = get_changed_paths(&changed_paths, commit, repo, &dsa);
-	if (err)
-		goto done;
-
-	TAILQ_FOREACH(pe, &changed_paths, entry) {
+	TAILQ_FOREACH(pe, dsa->paths, entry) {
 		struct got_diff_changed_path *cp = pe->data;
-		int pad = dsa.max_path_len - pe->path_len + 1;
+		int pad = dsa->max_path_len - pe->path_len + 1;
 
 		n = fprintf(outfile, "%c  %s%*c | %*d+ %*d-\n", cp->status,
-		    pe->path, pad, ' ', dsa.add_cols + 1, cp->add,
-		    dsa.rm_cols + 1, cp->rm);
+		    pe->path, pad, ' ', dsa->add_cols + 1, cp->add,
+		    dsa->rm_cols + 1, cp->rm);
 		if (n < 0) {
 			err = got_error_from_errno("fprintf");
 			goto done;
@@ -4721,8 +4676,8 @@ write_commit_info(struct got_diff_line **lines, size_t
 
 	n = fprintf(outfile,
 	    "%d file%s changed, %d insertion%s(+), %d deletion%s(-)\n",
-	    dsa.nfiles, dsa.nfiles > 1 ? "s" : "", dsa.ins,
-	    dsa.ins != 1 ? "s" : "", dsa.del, dsa.del != 1 ? "s" : "");
+	    dsa->nfiles, dsa->nfiles > 1 ? "s" : "", dsa->ins,
+	    dsa->ins != 1 ? "s" : "", dsa->del, dsa->del != 1 ? "s" : "");
 	if (n < 0) {
 		err = got_error_from_errno("fprintf");
 		goto done;
@@ -4736,7 +4691,6 @@ done:
 	outoff++;
 	err = add_line_metadata(lines, nlines, outoff, GOT_DIFF_LINE_NONE);
 done:
-	got_pathlist_free(&changed_paths, GOT_PATHLIST_FREE_ALL);
 	free(id_str);
 	free(logmsg);
 	free(refs_str);
@@ -4753,9 +4707,13 @@ create_diff(struct tog_diff_view_state *s)
 create_diff(struct tog_diff_view_state *s)
 {
 	const struct got_error *err = NULL;
-	FILE *f = NULL;
+	FILE *f = NULL, *tmp_diff_file = NULL;
 	int obj_type;
+	struct got_diff_line *lines = NULL;
+	struct got_pathlist_head changed_paths;
 
+	TAILQ_INIT(&changed_paths);
+
 	free(s->lines);
 	s->lines = malloc(sizeof(*s->lines));
 	if (s->lines == NULL)
@@ -4767,6 +4725,11 @@ create_diff(struct tog_diff_view_state *s)
 		err = got_error_from_errno("got_opentemp");
 		goto done;
 	}
+	tmp_diff_file = got_opentemp();
+	if (tmp_diff_file == NULL) {
+		err = got_error_from_errno("got_opentemp");
+		goto done;
+	}
 	if (s->f && fclose(s->f) == EOF) {
 		err = got_error_from_errno("fclose");
 		goto done;
@@ -4785,21 +4748,43 @@ create_diff(struct tog_diff_view_state *s)
 		err = got_diff_objects_as_blobs(&s->lines, &s->nlines,
 		    s->f1, s->f2, s->fd1, s->fd2, s->id1, s->id2,
 		    s->label1, s->label2, tog_diff_algo, s->diff_context,
-		    s->ignore_whitespace, s->force_text_diff, 0, NULL, s->repo,
+		    s->ignore_whitespace, s->force_text_diff, NULL, s->repo,
 		    s->f);
 		break;
 	case GOT_OBJ_TYPE_TREE:
 		err = got_diff_objects_as_trees(&s->lines, &s->nlines,
 		    s->f1, s->f2, s->fd1, s->fd2, s->id1, s->id2, NULL, "", "",
 		    tog_diff_algo, s->diff_context, s->ignore_whitespace,
-		    s->force_text_diff, 0, NULL, s->repo, s->f);
+		    s->force_text_diff, NULL, s->repo, s->f);
 		break;
 	case GOT_OBJ_TYPE_COMMIT: {
 		const struct got_object_id_queue *parent_ids;
 		struct got_object_qid *pid;
 		struct got_commit_object *commit2;
 		struct got_reflist_head *refs;
+		size_t nlines = 0;
+		struct got_diffstat_cb_arg dsa = {
+			0, 0, 0, 0, 0, 0,
+			&changed_paths,
+			s->ignore_whitespace,
+			s->force_text_diff,
+			tog_diff_algo
+		};
 
+		lines = malloc(sizeof(*lines));
+		if (lines == NULL) {
+			err = got_error_from_errno("malloc");
+			goto done;
+		}
+
+		/* build diff first in tmp file then append to commit info */
+		err = got_diff_objects_as_commits(&lines, &nlines,
+		    s->f1, s->f2, s->fd1, s->fd2, s->id1, s->id2, NULL,
+		    tog_diff_algo, s->diff_context, s->ignore_whitespace,
+		    s->force_text_diff, &dsa, s->repo, tmp_diff_file);
+		if (err)
+			break;
+
 		err = got_object_open_as_commit(&commit2, s->repo, s->id2);
 		if (err)
 			goto done;
@@ -4808,7 +4793,7 @@ create_diff(struct tog_diff_view_state *s)
 		if (s->id1 == NULL) {
 			err = write_commit_info(&s->lines, &s->nlines, s->id2,
 			    refs, s->repo, s->ignore_whitespace,
-			    s->force_text_diff, s->f);
+			    s->force_text_diff, &dsa, s->f);
 			if (err)
 				goto done;
 		} else {
@@ -4818,7 +4803,7 @@ create_diff(struct tog_diff_view_state *s)
 					err = write_commit_info(&s->lines,
 					    &s->nlines, s->id2, refs, s->repo,
 					    s->ignore_whitespace,
-					    s->force_text_diff, s->f);
+					    s->force_text_diff, &dsa, s->f);
 					if (err)
 						goto done;
 					break;
@@ -4827,10 +4812,8 @@ create_diff(struct tog_diff_view_state *s)
 		}
 		got_object_commit_close(commit2);
 
-		err = got_diff_objects_as_commits(&s->lines, &s->nlines,
-		    s->f1, s->f2, s->fd1, s->fd2, s->id1, s->id2, NULL,
-		    tog_diff_algo, s->diff_context, s->ignore_whitespace,
-		    s->force_text_diff, 0, NULL, s->repo, s->f);
+		err = cat_diff(s->f, tmp_diff_file, &s->lines, &s->nlines,
+		    lines, nlines);
 		break;
 	}
 	default:
@@ -4838,8 +4821,12 @@ done:
 		break;
 	}
 done:
+	free(lines);
+	got_pathlist_free(&changed_paths, GOT_PATHLIST_FREE_ALL);
 	if (s->f && fflush(s->f) != 0 && err == NULL)
 		err = got_error_from_errno("fflush");
+	if (tmp_diff_file && fclose(tmp_diff_file) == EOF && err == NULL)
+		err = got_error_from_errno("fclose");
 	return err;
 }
 

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68