From: Mark Jamsek Subject: Re: tog diff lacks diffstat To: Mark Jamsek Cc: Christian Weisgerber , gameoftrees@openbsd.org Date: Fri, 16 Aug 2024 01:14:55 +1000 Mark Jamsek wrote: > Mark Jamsek wrote: > > Christian Weisgerber wrote: > > > I don't use "tog diff" in practice, so it's only with the new mark > > > feature that I have noticed that there is no diffstat if the diff > > > doesn't correspond to a single commit. Mark, can you add this? > > > > Sure! I'll do it tonight. Thanks for pointing it out :) > > The below diff adds the diffstat to diff views of arbitrary commits, not > just parent:child commits. I figured we should also show it in diffs of > blobs and trees too, so have added that; regress has been adapted and > expanded to cover the change and is passing in sha1 and sha256 mode. > > The diff is rebased on main but is on top of the tog diff 'p' keymap > commit in my tree thought it should still apply without it. Updated/actual diff. Previous diff sent was the wrong commit: after adding the diffstat to blobs and trees, we don't need the do_diffstat flag as the diffstat is now shown in all diff types so written unconditionally. M regress/tog/diff.sh | 107+ 7- M regress/tog/log.sh | 18+ 2- M tog/tog.c | 100+ 66- 3 files changed, 225 insertions(+), 75 deletions(-) commit - a0eec9526dea03232daf4c7fc4e84ea24a06d786 commit + 7ea98496b009d71032480f2f134304929b9e7f6d blob - c31964ba473354a0a9622d6c1724f4705d6d3dcd blob + d8d0d214f46d115de70295102f416c8efa700d8d --- regress/tog/diff.sh +++ regress/tog/diff.sh @@ -76,7 +76,7 @@ EOF test_diff_arbitrary_commits() { - test_init diff_arbitrary_commits 80 18 + test_init diff_arbitrary_commits local commit_id1=`git_show_head $testroot/repo` local alpha_id_old=`get_blob_id $testroot/repo "" alpha` @@ -99,7 +99,12 @@ SCREENDUMP EOF cat <$testroot/view.expected -$(trim 80 "[1/16] diff $commit_id1 $head_id_truncated") +$(trim 80 "[1/21] diff $commit_id1 $head_id_truncated") +M alpha | 1+ 1- +A new | 1+ 0- + +2 files changed, 2 insertions(+), 1 deletion(-) + commit - $commit_id1 commit + $head_id blob - $alpha_id_old @@ -116,6 +121,7 @@ $(trim 80 "blob + $new_id (mode 644)") @@ -0,0 +1 @@ +new file + (END) EOF @@ -404,7 +410,11 @@ test_diff_commit_keywords() rhs_id=$(pop_idx 8 $ids) cat <<-EOF >$testroot/view.expected - $(trim 120 "[1/10] diff $lhs_id $rhs_id") + $(trim 120 "[1/14] diff $lhs_id $rhs_id") + M alpha | 1+ 1- + + 1 file changed, 1 insertion(+), 1 deletion(-) + commit - $lhs_id commit + $rhs_id blob - $(pop_idx 5 $alpha_ids) @@ -423,10 +433,6 @@ test_diff_commit_keywords() - - - - (END) EOF @@ -546,9 +552,103 @@ EOF test_done "$testroot" "$ret" } +test_diff_blobs() +{ + test_init diff_blobs 148 14 + + local blob_alpha_root=$(get_blob_id $testroot/repo "" alpha) + + echo "new alpha" > $testroot/repo/alpha + git_commit $testroot/repo -m "new alpha" + + local blob_alpha_head=$(get_blob_id $testroot/repo "" alpha) + + cat <$TOG_TEST_SCRIPT +SCREENDUMP +EOF + + cat <$testroot/view.expected +[1/12] diff $blob_alpha_root $blob_alpha_head +M $blob_alpha_head | 1+ 1- + +1 file changed, 1 insertion(+), 1 deletion(-) + +blob - $blob_alpha_root +blob + $blob_alpha_head +--- $blob_alpha_root ++++ $blob_alpha_head +@@ -1 +1 @@ +-alpha ++new alpha + +(END) +EOF + + cd $testroot/repo && tog diff $blob_alpha_root $blob_alpha_head + cmp -s $testroot/view.expected $testroot/view + ret=$? + if [ $ret -ne 0 ]; then + diff -u $testroot/view.expected $testroot/view + test_done "$testroot" $ret + return 1 + fi + + test_done "$testroot" $ret +} + +test_diff_trees() +{ + test_init diff_trees 148 16 + + local tree_gamma_root=$(get_blob_id $testroot/repo "" gamma/) + local blob_delta_root=$(get_blob_id $testroot/repo gamma delta) + + echo "new delta" > $testroot/repo/gamma/delta + git_commit $testroot/repo -m "new delta" + + local tree_gamma_head=$(get_blob_id $testroot/repo "" gamma/) + local blob_delta_head=$(get_blob_id $testroot/repo gamma delta) + + cat <$TOG_TEST_SCRIPT +SCREENDUMP +EOF + + cat <$testroot/view.expected +[1/14] diff $tree_gamma_root $tree_gamma_head +M delta | 1+ 1- + +1 file changed, 1 insertion(+), 1 deletion(-) + +tree - $tree_gamma_root +tree + $tree_gamma_head +blob - $blob_delta_root +blob + $blob_delta_head +--- delta ++++ delta +@@ -1 +1 @@ +-delta ++new delta + +(END) +EOF + + cd $testroot/repo && tog diff $tree_gamma_root $tree_gamma_head + cmp -s $testroot/view.expected $testroot/view + ret=$? + if [ $ret -ne 0 ]; then + diff -u $testroot/view.expected $testroot/view + test_done "$testroot" $ret + return 1 + fi + + test_done "$testroot" $ret +} + test_parseargs "$@" run_test test_diff_contiguous_commits run_test test_diff_arbitrary_commits run_test test_diff_J_keymap run_test test_diff_commit_keywords run_test test_diff_horizontal_scroll +run_test test_diff_blobs +run_test test_diff_trees blob - 67bde569cd6803d9fc6f03e82a82db50051d61c3 blob + 0b8323f8cea5abb4777743d902c28ee9388c52f4 --- regress/tog/log.sh +++ regress/tog/log.sh @@ -728,7 +728,7 @@ test_log_search() test_log_mark_keymap() { - test_init log_mark_keymap 141 10 + test_init log_mark_keymap 141 14 local repo="$testroot/repo" local wt="$testroot/wt" @@ -781,6 +781,10 @@ test_log_mark_keymap() + + + + EOF tog log @@ -811,6 +815,10 @@ test_log_mark_keymap() + + + + EOF tog log @@ -839,6 +847,10 @@ test_log_mark_keymap() + + + + EOF tog log @@ -860,7 +872,11 @@ test_log_mark_keymap() EOF cat <<-EOF >$testroot/view.expected - [1/10] diff $id_head $id_root + [1/14] diff $id_head $id_root + M alpha | 1+ 1- + + 1 file changed, 1 insertion(+), 1 deletion(-) + commit - $id_head commit + $id_root blob - $alpha_head blob - 0f0c5142c308899d47b16880bf3b26d868b72c92 blob + e15f571c7eea4eb3da1738e1d2233258b41c72db --- tog/tog.c +++ tog/tog.c @@ -5190,6 +5190,67 @@ cat_diff(FILE *dst, FILE *src, struct got_diff_line ** } static const struct got_error * +write_diffstat(FILE *outfile, struct got_diff_line **lines, size_t *nlines, + struct got_diffstat_cb_arg *dsa) +{ + const struct got_error *err; + struct got_pathlist_entry *pe; + off_t offset; + int n; + + if (*nlines == 0) { + err = add_line_metadata(lines, nlines, 0, GOT_DIFF_LINE_NONE); + if (err != NULL) + return err; + offset = 0; + } else + offset = (*lines)[*nlines - 1].offset; + + TAILQ_FOREACH(pe, dsa->paths, entry) { + struct got_diff_changed_path *cp = pe->data; + 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); + if (n < 0) + return got_error_from_errno("fprintf"); + + offset += n; + err = add_line_metadata(lines, nlines, offset, + GOT_DIFF_LINE_CHANGES); + if (err != NULL) + return err; + } + + if (fputc('\n', outfile) == EOF) + return got_error_from_errno("fputc"); + + offset++; + err = add_line_metadata(lines, nlines, offset, GOT_DIFF_LINE_NONE); + if (err != NULL) + return err; + + 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" : ""); + if (n < 0) + return got_error_from_errno("fprintf"); + + offset += n; + err = add_line_metadata(lines, nlines, offset, GOT_DIFF_LINE_NONE); + if (err != NULL) + return err; + + if (fputc('\n', outfile) == EOF) + return got_error_from_errno("fputc"); + + offset++; + return add_line_metadata(lines, nlines, offset, GOT_DIFF_LINE_NONE); +} + +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, @@ -5202,7 +5263,6 @@ 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_entry *pe; off_t outoff = 0; int n; @@ -5315,46 +5375,6 @@ write_commit_info(struct got_diff_line **lines, size_t goto done; } - TAILQ_FOREACH(pe, dsa->paths, entry) { - struct got_diff_changed_path *cp = pe->data; - 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); - if (n < 0) { - err = got_error_from_errno("fprintf"); - goto done; - } - outoff += n; - err = add_line_metadata(lines, nlines, outoff, - GOT_DIFF_LINE_CHANGES); - if (err) - goto done; - } - - fputc('\n', outfile); - outoff++; - err = add_line_metadata(lines, nlines, outoff, GOT_DIFF_LINE_NONE); - if (err) - goto done; - - 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" : ""); - if (n < 0) { - err = got_error_from_errno("fprintf"); - goto done; - } - outoff += n; - err = add_line_metadata(lines, nlines, outoff, GOT_DIFF_LINE_NONE); - if (err) - goto done; - - fputc('\n', outfile); - outoff++; - err = add_line_metadata(lines, nlines, outoff, GOT_DIFF_LINE_NONE); done: free(id_str); free(logmsg); @@ -5372,8 +5392,15 @@ create_diff(struct tog_diff_view_state *s) struct got_diff_line *lines = NULL; struct got_pathlist_head changed_paths; struct got_commit_object *commit2 = NULL; + struct got_diffstat_cb_arg dsa; + size_t nlines = 0; TAILQ_INIT(&changed_paths); + memset(&dsa, 0, sizeof(dsa)); + dsa.paths = &changed_paths; + dsa.diff_algo = tog_diff_algo; + dsa.force_text = s->force_text_diff; + dsa.ignore_ws = s->ignore_whitespace; free(s->lines); s->lines = malloc(sizeof(*s->lines)); @@ -5390,10 +5417,23 @@ create_diff(struct tog_diff_view_state *s) if (s->f == NULL) return got_error_from_errno("got_opentemp"); + /* + * The diffstat requires the diff to be built first, but we want the + * diffstat to prepend the diff when displayed. Build the diff first + * in the temporary file and write the diffstat and/or commit info to + * the persistent file (s->f) from which views are drawn, then append + * the diff from the temp file to the diffstat/commit info in s->f. + */ tmp_diff_file = got_opentemp(); if (tmp_diff_file == NULL) return got_error_from_errno("got_opentemp"); + lines = malloc(sizeof(*lines)); + if (lines == NULL) { + err = got_error_from_errno("malloc"); + goto done; + } + if (s->id1) err = got_object_get_type(&obj_type, s->repo, s->id1); else @@ -5403,44 +5443,33 @@ create_diff(struct tog_diff_view_state *s) switch (obj_type) { case GOT_OBJ_TYPE_BLOB: - err = got_diff_objects_as_blobs(&s->lines, &s->nlines, + err = got_diff_objects_as_blobs(&lines, &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, NULL, s->repo, - s->f); + s->ignore_whitespace, s->force_text_diff, &dsa, s->repo, + tmp_diff_file); + if (err != NULL) + goto done; break; case GOT_OBJ_TYPE_TREE: - err = got_diff_objects_as_trees(&s->lines, &s->nlines, + err = got_diff_objects_as_trees(&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, NULL, s->repo, s->f); + s->force_text_diff, &dsa, s->repo, tmp_diff_file); + if (err != NULL) + goto done; break; case GOT_OBJ_TYPE_COMMIT: { const struct got_object_id_queue *parent_ids; struct got_object_qid *pid; 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; + goto done; refs = got_reflist_object_id_map_lookup(tog_refs_idmap, s->id2); /* Show commit info if we're diffing to a parent/root commit. */ @@ -5469,15 +5498,20 @@ create_diff(struct tog_diff_view_state *s) } } } - - err = cat_diff(s->f, tmp_diff_file, &s->lines, &s->nlines, - lines, nlines); break; } default: err = got_error(GOT_ERR_OBJ_TYPE); - break; + goto done; } + + err = write_diffstat(s->f, &s->lines, &s->nlines, &dsa); + if (err != NULL) + goto done; + + err = cat_diff(s->f, tmp_diff_file, &s->lines, &s->nlines, + lines, nlines); + done: free(lines); if (commit2 != NULL) -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68