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

The below diff is borne out of previous discussions with stsp about eliminating the cost of computing the diff twice when displaying the diffstat in 'got log -d' and tog diff views. It takes the same approach as the 'got diff -d' case where we use a temp file for the diff so that we can compute the diffstat with the same result from building the diff, and still draw the diffstat first once the diff is complete. This has the additional benefit in tog (and got log -d vs got log -P -p) of not running the get_changed_paths() -> got_diff_tree_collect_changed_paths() code path to get the changeset before computing the diff (which traversed much the same code twice). It also results in net less code :) From a performance perspective: at the extreme end, if you load tog with b5500b9ca0102f1c in OpenBSD src.git with and without this patch you will notice the difference. For the average diff it's not nearly as noticeable but is an improvement nonetheless. 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. diffstat /home/mark/src/got M got/got.1 | 11+ 1- M got/got.c | 77+ 105- M gotwebd/got_operations.c | 3+ 3- M include/got_diff.h | 6+ 7- M lib/diff.c | 30+ 107- M lib/worktree.c | 2+ 2- M tog/tog.c | 95+ 108- 7 files changed, 224 insertions(+), 333 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,13 @@ 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 before the diff of modifications by annotating each file path +modified in the commit with the total number of lines added and removed. +A summary line will display the total number of changes across all files. +If a +.Ar path +is specified, only show the diffstat and diff of modifications at or within +this path. Cannot be used with the .Fl s option. @@ -879,9 +885,13 @@ is specified. .Ar search-pattern . Lines in committed patches will be matched if .Fl p +or +.Fl d is specified. File paths changed by a commit will be matched if .Fl P +or +.Fl d is specified. Regular expression syntax is documented in .Xr re_format 7 . 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; @@ -3742,42 +3742,13 @@ 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) + struct got_commit_object *commit, struct got_repository *repo) { 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; - got_diff_blob_cb cb = got_diff_tree_collect_changed_paths; - FILE *f1 = NULL, *f2 = NULL; - int fd1 = -1, fd2 = -1; - if (dsa) { - cb = got_diff_tree_compute_diffstat; - - 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; @@ -3807,29 +3778,21 @@ get_changed_paths(struct got_pathlist_head *paths, if (err) goto done; - err = got_diff_tree(tree1, tree2, f1, f2, fd1, fd2, "", "", repo, - cb, dsa ? (void *)dsa : paths, dsa ? 1 : 0); + err = got_diff_tree(tree1, tree2, NULL, NULL, -1, -1, "", "", repo, + got_diff_tree_collect_changed_paths, paths, 0); 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 * 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 +3843,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 +3865,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 +3957,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; @@ -4179,6 +4142,30 @@ 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, @@ -4186,6 +4173,7 @@ print_commit(struct got_commit_object *commit, struct 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 +4240,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 && dsa == NULL) { struct got_pathlist_entry *pe; TAILQ_FOREACH(pe, changed_paths, entry) { @@ -4266,15 +4250,33 @@ 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 (show_patch || dsa != NULL) { + f = got_opentemp(); + if (f == NULL) { + err = got_error_from_errno("got_opentemp"); + goto done; + } + + err = print_patch(commit, id, path, diff_context, dsa, repo, + dsa == NULL ? stdout : f); + if (err) + goto done; + if (dsa != NULL) { + err = print_diffstat(dsa, changed_paths, NULL); + if (err) + goto done; + err = printfile(f); + if (err) + goto done; + } + 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,10 +4333,9 @@ print_commits(struct got_object_id *root_id, struct go if (err) break; - if ((show_changed_paths || show_diffstat) && - !reverse_display_order) { - err = get_changed_paths(&changed_paths, commit, repo, - show_diffstat ? &dsa : NULL); + if (show_changed_paths && !reverse_display_order && + !show_diffstat) { + err = get_changed_paths(&changed_paths, commit, repo); if (err) break; } @@ -4348,10 +4349,9 @@ print_commits(struct got_object_id *root_id, struct go if (have_match == 0 && show_changed_paths) match_changed_paths(&have_match, &changed_paths, &regex); - if (have_match == 0 && show_patch) { + if (have_match == 0 && (show_patch || show_diffstat)) { err = match_patch(&have_match, commit, &id, - path, diff_context, repo, &regex, - tmpfile); + path, diff_context, repo, &regex, tmpfile); if (err) break; } @@ -4398,9 +4398,9 @@ print_commits(struct got_object_id *root_id, struct go &qid->id); if (err) break; - if (show_changed_paths || show_diffstat) { + if (show_changed_paths && !show_diffstat) { err = get_changed_paths(&changed_paths, - commit, repo, show_diffstat ? &dsa : NULL); + commit, repo); if (err) break; } @@ -4673,7 +4673,7 @@ cmd_log(int argc, char *argv[]) worktree = NULL; } - if (search_pattern && show_patch) { + if (search_pattern && (show_patch || show_diffstat)) { tmpfile = got_opentemp(); if (tmpfile == NULL) { error = got_error_from_errno("got_opentemp"); @@ -4730,7 +4730,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 +4872,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 +4962,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 +4979,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 +5206,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; @@ -5396,14 +5368,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 +5383,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: 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 * @@ -762,78 +760,6 @@ got_diff_tree_compute_diffstat(void *arg, struct got_b } const struct got_error * -got_diff_tree_compute_diffstat(void *arg, struct got_blob_object *blob1, - struct got_blob_object *blob2, FILE *f1, FILE *f2, - struct got_object_id *id1, struct got_object_id *id2, - const char *label1, const char *label2, - mode_t mode1, mode_t mode2, struct got_repository *repo) -{ - const struct got_error *err = NULL; - struct got_diffreg_result *result = NULL; - struct got_diffstat_cb_arg *a = arg; - char *path = NULL; - int status = GOT_STATUS_NO_CHANGE; - - path = strdup(label2 ? label2 : label1); - if (path == NULL) - return got_error_from_errno("strdup"); - - if (id1 == NULL) - status = GOT_STATUS_ADD; - else if (id2 == NULL) - status = GOT_STATUS_DELETE; - else { - if (got_object_id_cmp(id1, id2) != 0) - status = GOT_STATUS_MODIFY; - else if (mode1 != mode2) - status = GOT_STATUS_MODE_CHANGE; - } - - if (f1) { - err = got_opentemp_truncate(f1); - if (err) - goto done; - } - if (f2) { - err = got_opentemp_truncate(f2); - if (err) - goto done; - } - - if (blob1) { - err = got_object_blob_dump_to_file(NULL, NULL, NULL, f1, - blob1); - if (err) - goto done; - } - if (blob2) { - err = got_object_blob_dump_to_file(NULL, NULL, NULL, f2, - blob2); - if (err) - goto done; - } - - err = got_diffreg(&result, f1, f2, a->diff_algo, a->ignore_ws, - a->force_text); - if (err) - goto done; - - err = get_diffstat(a, path, result->result, a->force_text, status); - -done: - if (result) { - const struct got_error *free_err; - - free_err = got_diffreg_result_free(result); - if (free_err && err == NULL) - err = free_err; - } - if (err) - free(path); - return err; -} - -const struct got_error * got_diff_tree_collect_changed_paths(void *arg, struct got_blob_object *blob1, struct got_blob_object *blob2, FILE *f1, FILE *f2, struct got_object_id *id1, struct got_object_id *id2, @@ -995,8 +921,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 +942,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 +1116,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 +1142,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 +1175,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 +1218,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 +1230,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 +1273,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 prepends an empty line when the array is not + * already populated, 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