From: Mark Jamsek Subject: Re: tog diff lacks diffstat To: Mark Jamsek Cc: Christian Weisgerber , gameoftrees@openbsd.org Date: Thu, 15 Aug 2024 23:56:11 +1000 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. Thanks again for highlighting its absence, naddy! M regress/tog/diff.sh | 107+ 7- M regress/tog/log.sh | 18+ 2- M tog/tog.c | 106+ 65- 3 files changed, 231 insertions(+), 74 deletions(-) commit - 1420a6d42d9ffe5e2fcf77760d2068bfaf9c8757 commit + eba95b8a4f303376c3833adad398feb8e300854a 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 - 13586fd308da97f395a46278d336699dc1aa3eac blob + 3c2a7c4f7c4a6865f154ca10ac9c0ba1e7775be1 --- 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,8 @@ 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; + err = write_diffstat(outfile, lines, nlines, dsa); - 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); @@ -5377,8 +5399,16 @@ 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; + int do_diffstat = 1; 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)); @@ -5395,10 +5425,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 @@ -5408,44 +5451,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. */ @@ -5455,6 +5487,7 @@ create_diff(struct tog_diff_view_state *s) s->force_text_diff, &dsa, s->f); if (err) goto done; + do_diffstat = 0; } else { err = got_object_open_as_commit(&commit2, s->repo, s->id2); @@ -5470,19 +5503,27 @@ create_diff(struct tog_diff_view_state *s) s->force_text_diff, &dsa, s->f); if (err) goto done; + do_diffstat = 0; break; } } } - - 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; } + + if (do_diffstat) { + 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