refactor diffstat to compute diff once in got log -d and tog diff view

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

2023-01-16 14:13 Mark Jamsek:
refactor diffstat to compute diff once in got log -d and tog diff view
- 2023-01-16 17:34 Stefan Sperling:
  refactor diffstat to compute diff once in got log -d and tog diff view
- - 2023-01-17 07:19 Mark Jamsek:
    refactor diffstat to compute diff once in got log -d and tog diff view
  - - 2023-01-17 08:40 Stefan Sperling:
      refactor diffstat to compute diff once in got log -d and tog diff view