From: Mark Jamsek Subject: Re: show base commit markers in tog log view To: Mark Jamsek Cc: Stefan Sperling , gameoftrees@openbsd.org Date: Sun, 23 Jul 2023 00:19:31 +1000 Mark Jamsek wrote: > Mark Jamsek wrote: > > Mark Jamsek wrote: > > > Stefan Sperling wrote: > > > > On Fri, Jul 21, 2023 at 10:54:58PM +1000, Mark Jamsek wrote: > > > > > This is a simple change along the same lines of showing reference labels > > > > > next to tog log messages insofar as it improves situational awareness, > > > > > which can be handy at times. > > > > > > > > > > For example, in the case where you want to fold or drop some commits. > > > > > After 'got up -c:head:-N', I often like to confirm I've updated to the > > > > > correct commit before invoking histedit. With this, you can open tog and > > > > > at a glance confirm that your base commit is indeed the correct one for > > > > > the desired histedit operation. Similarly, when your base commit is > > > > > something other than the tip, it's nice to know where the base commit is > > > > > in relation to the rest when browsing the log view. This can interop > > > > > well with the new :base keyword feature. > > > > > > > > > > The change will be immediately obvious upon running tog with the patch > > > > > inside a work tree, but to summarise: we show the "[base commit]" label > > > > > in the log view headline if the currently selected commit is the work > > > > > tree's base commit; and, irrespective of which commit is selected, we > > > > > prepend the base commit's summary log message with the '@' symbol. There > > > > > is no change to the log view when tog is invoked outside a work tree. > > > > > > > > I agree that this is useful. > > > > > > > > Some things I would suggest to change: > > > > > > > > Could we use * instead of @, for consistency with got branch -l? > > > > And like got branch -l, could tog show ~ if the work tree is out of > > > > date with respect to the branch tip? > > > > > > > > A problem that applies to both this patch and got branch -l: > > > > A mixed-commit work tree will be displayed as up-to-date since the current > > > > logic only considers the work tree's global base commit. > > > > Could we iterate over the file index and check whether any file is based > > > > on a different commit, and if so show the out-of-date symbol ~? > > > > > > > > It is a matter of taste but the [base commit] marker seems redundant to me > > > > and makes the display seem too crowded. I would omit this marker for now. > > > > > > Yes, sure! The reason I wasn't concerned with distinguishing between > > > out of date and up-to-date trees is it's easy to see in the log view in > > > the general case because it won't be the topmost commit; but that's a > > > good idea about mixed commits--there's no other way to see this without > > > different symbols. > > > > > > The below diffs implement your suggestions: the first adds a new lib > > > routine to get the tree state, and shows the * or ~ marker in tog (the > > > headline label has also been removed); the second gives the same > > > treatment to 'got branch -l' including test coverage. > > > > > > I've also added a couple got_worktree_close() calls to cmd_ref() and > > > cmd_tree() that we were missing; if we hit an error early in these > > > functions we were leaking the worktree. > > > > I'm not sure if the tweak to 'got branch -l' output necessitates a > > change to the docs too? Just in case, here's a minor addition to > > mention the mixed commit case. > > The previous version of the tog base commit marker diff was inefficient; > we would compare the id of each commit being drawn on every page being > rendered even after we'd already drawn the base commit or were rendering > a page that the base commit was not on. > > The below diff improves this by finding the base commit index when > queuing commits so once we have identified the base commit, we don't > do any more id comparisons. It's a marginal optimisation that would be > of more benefit on larger monitors and terminals with many rows, but is > still worth it imo. > > I've also folded the got(1) doc change into the related diff with the > 'got branch -l' tweak. I left behind a superfluous tog_base_commit.idx != -1 check that has now been removed: ----------------------------------------------- commit d1ff26edd0234c2837a5850d3272ba0b88054af7 from: Mark Jamsek date: Sat Jul 22 14:04:44 2023 UTC tog: show work tree base commit marker in log view If tog is invoked in a work tree, prefix the base commit log message summary line with a '*' if the work tree is up-to-date, and with a '~' if the base commit is not up-to-date with respect to the branch tip or it contains mixed commits. While here, plug a couple worktree leaks in cmd_ref() and cmd_tree(). M include/got_worktree.h | 12+ 0- M lib/worktree.c | 47+ 0- M regress/tog/log.sh | 90+ 1- M tog/tog.c | 75+ 4- 4 files changed, 224 insertions(+), 5 deletions(-) diff 9c986b77d3f71b37ff5b5c6b4ec58b495ad8f312 d1ff26edd0234c2837a5850d3272ba0b88054af7 commit - 9c986b77d3f71b37ff5b5c6b4ec58b495ad8f312 commit + d1ff26edd0234c2837a5850d3272ba0b88054af7 blob - 53e320eef8622a3384a31606d0cd2fe64d9ea60a blob + 07cb22fd98f7b10c110a8a5d0c0627770c5b7e76 --- include/got_worktree.h +++ include/got_worktree.h @@ -134,6 +134,18 @@ const struct got_error *got_worktree_set_base_commit_i struct got_repository *, struct got_object_id *); /* + * Get the state of the work tree. If the work tree's global base commit is + * the tip of the work tree's current branch, and each file in the index is + * based on this same commit, the char out parameter will be + * GOT_WORKTREE_UPTODATE, else it will be GOT_WORKTREE_OUTOFDATE. + */ +const struct got_error *got_worktree_get_state(char *, + struct got_repository *, struct got_worktree *); + +#define GOT_WORKTREE_UPTODATE '*' +#define GOT_WORKTREE_OUTOFDATE '~' + +/* * Obtain a parsed representation of this worktree's got.conf file. * Return NULL if this configuration file could not be read. */ blob - 5400e1336138a373e1307ff721ae4d68d5633075 blob + 6b14bc178bf5fbc6065088656b0ecae4a8fd2aad --- lib/worktree.c +++ lib/worktree.c @@ -3251,6 +3251,53 @@ struct check_merge_conflicts_arg { return NULL; } +const struct got_error * +got_worktree_get_state(char *state, struct got_repository *repo, + struct got_worktree *worktree) +{ + const struct got_error *err; + struct got_object_id *base_id, *head_id = NULL; + struct got_reference *head_ref; + struct got_fileindex *fileindex = NULL; + char *fileindex_path = NULL; + + if (worktree == NULL) + return got_error(GOT_ERR_NOT_WORKTREE); + + err = got_ref_open(&head_ref, repo, + got_worktree_get_head_ref_name(worktree), 0); + if (err) + return err; + + err = got_ref_resolve(&head_id, repo, head_ref); + if (err) + goto done; + + *state = GOT_WORKTREE_OUTOFDATE; + base_id = got_worktree_get_base_commit_id(worktree); + + if (got_object_id_cmp(base_id, head_id) == 0) { + err = open_fileindex(&fileindex, &fileindex_path, worktree); + if (err) + goto done; + + err = got_fileindex_for_each_entry_safe(fileindex, + check_mixed_commits, worktree); + if (err == NULL) + *state = GOT_WORKTREE_UPTODATE; + else if (err->code == GOT_ERR_MIXED_COMMITS) + err = NULL; + } + +done: + free(head_id); + free(fileindex_path); + got_ref_close(head_ref); + if (fileindex != NULL) + got_fileindex_free(fileindex); + return err; +} + struct check_merge_conflicts_arg { struct got_worktree *worktree; struct got_fileindex *fileindex; blob - 2a28c65c9bf9e227cba943ce259438976ac67c02 blob + d10c8e4e269c29522a88fa8b7f94b5658cacd469 --- regress/tog/log.sh +++ regress/tog/log.sh @@ -448,7 +448,7 @@ test_log_commit_keywords() commit $(pop_id 5 $ids) [1/5] $ymd $(pop_id 5 $short_ids) flan_hacker commit 4 $ymd $(pop_id 4 $short_ids) flan_hacker commit 3 - $ymd $(pop_id 3 $short_ids) flan_hacker commit 2 + $ymd $(pop_id 3 $short_ids) flan_hacker ~commit 2 $ymd $(pop_id 2 $short_ids) flan_hacker commit 1 $ymd $(pop_id 1 $short_ids) flan_hacker adding the test tree @@ -499,6 +499,94 @@ test_parseargs "$@" test_done "$testroot" "$ret" } +test_log_show_base_commit() +{ + # make view wide enough to show full headline + test_init log_show_base_commit 80 3 + local repo="$testroot/repo" + local id=$(git_show_head "$repo") + + echo "alpha" >> "$repo/alpha" + git_commit "$repo" -m "base commit" + + got checkout "$repo" "$testroot/wt" > /dev/null + ret=$? + if [ $ret -ne 0 ]; then + echo "got checkout failed unexpectedly" + test_done "$testroot" "$ret" + return 1 + fi + + # move into the work tree (test is run in a subshell) + cd "$testroot/wt" + + local head_id=$(git_show_head "$repo") + local author_time=$(git_show_author_time "$repo") + local ymd=$(date -u -r "$author_time" +"%G-%m-%d") + + # check up-to-date base commit marker prefixes base commit log message + cat <<-EOF >$TOG_TEST_SCRIPT + SCREENDUMP + EOF + + cat <<-EOF >$testroot/view.expected + commit $head_id [1/2] master + $ymd flan_hacker *[master] base commit + $ymd flan_hacker adding the test tree + EOF + + tog log + 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 + + # check marker is not drawn when not in a work tree + cat <<-EOF >$testroot/view.expected + commit $head_id [1/2] master + $ymd flan_hacker [master] base commit + $ymd flan_hacker adding the test tree + EOF + + tog log -r "$repo" + 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 + + # check out-of-date marker is shown with a mixed-commit tree + echo "mixed" > alpha + got commit -m "new base mixed-commit" > /dev/null + head_id=$(git_show_head "$repo") + + cat <<-EOF >$TOG_TEST_SCRIPT + SCREENDUMP + EOF + + cat <<-EOF >$testroot/view.expected + commit $head_id [1/3] master + $ymd flan_hacker ~[master] new base mixed-commit + $ymd flan_hacker base commit + EOF + + tog log + 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_log_hsplit_diff run_test test_log_vsplit_diff @@ -508,3 +596,4 @@ run_test test_log_commit_keywords run_test test_log_hsplit_tree run_test test_log_logmsg_widechar run_test test_log_commit_keywords +run_test test_log_show_base_commit blob - 55201d879df128d62d2a76ec797059af85649fa8 blob + 8a1028bb4f2e258d908ee56b92451061bcca21f7 --- tog/tog.c +++ tog/tog.c @@ -151,6 +151,11 @@ static enum got_diff_algorithm tog_diff_algo = GOT_DIF static struct got_reflist_head tog_refs = TAILQ_HEAD_INITIALIZER(tog_refs); static struct got_reflist_object_id_map *tog_refs_idmap; +static struct { + struct got_object_id *id; + int idx; + char marker; +} tog_base_commit; static enum got_diff_algorithm tog_diff_algo = GOT_DIFF_ALGORITHM_MYERS; static const struct got_error * @@ -2399,7 +2404,7 @@ draw_commit(struct tog_view *view, struct got_commit_o static const struct got_error * draw_commit(struct tog_view *view, struct got_commit_object *commit, struct got_object_id *id, const size_t date_display_cols, - int author_display_cols) + int author_display_cols, int entry_idx) { struct tog_log_view_state *s = &view->state.log; const struct got_error *err = NULL; @@ -2411,7 +2416,7 @@ draw_commit(struct tog_view *view, struct got_commit_o int author_width, refstr_width, logmsg_width; char *newline, *line = NULL; int col, limit, scrollx, logmsg_x; - const int avail = view->ncols; + const int avail = view->ncols, marker_column = author_display_cols + 1; struct tm tm; time_t committer_time; struct tog_color *tc; @@ -2476,7 +2481,12 @@ draw_commit(struct tog_view *view, struct got_commit_o waddwstr(view->window, wauthor); col += author_width; while (col < avail && author_width < author_display_cols + 2) { - waddch(view->window, ' '); + if (tog_base_commit.id != NULL && + author_width == marker_column && + entry_idx == tog_base_commit.idx) + waddch(view->window, tog_base_commit.marker); + else + waddch(view->window, ' '); col++; author_width++; } @@ -2691,6 +2701,10 @@ queue_commits(struct tog_log_thread_args *a) TAILQ_INSERT_TAIL(&a->real_commits->head, entry, entry); a->real_commits->ncommits++; + if (tog_base_commit.id != NULL && tog_base_commit.idx == -1 && + got_object_id_cmp(&id, tog_base_commit.id) == 0) + tog_base_commit.idx = entry->idx; + if (*a->limiting) { err = match_commit(&limit_match, &id, commit, a->limit_regex); @@ -2949,7 +2963,7 @@ draw_commits(struct tog_view *view) if (ncommits == s->selected) wstandout(view->window); err = draw_commit(view, entry->commit, entry->id, - date_display_cols, author_cols); + date_display_cols, author_cols, entry->idx); if (ncommits == s->selected) wstandend(view->window); if (err) @@ -4365,6 +4379,20 @@ get_in_repo_path_from_argv0(char **in_repo_path, int a } static const struct got_error * +set_tog_base_commit(struct got_repository *repo, struct got_worktree *worktree) +{ + tog_base_commit.id = got_object_id_dup( + got_worktree_get_base_commit_id(worktree)); + if (tog_base_commit.id == NULL) + return got_error_from_errno( "got_object_id_dup"); + + tog_base_commit.idx = -1; + + return got_worktree_get_state(&tog_base_commit.marker, repo, + worktree); +} + +static const struct got_error * get_in_repo_path_from_argv0(char **in_repo_path, int argc, char *argv[], struct got_repository *repo, struct got_worktree *worktree) { @@ -4518,13 +4546,21 @@ cmd_log(int argc, char *argv[]) in_repo_path, log_branches); if (error) goto done; + if (worktree) { + error = set_tog_base_commit(repo, worktree); + if (error != NULL) + goto done; + /* Release work tree lock. */ got_worktree_close(worktree); worktree = NULL; } + error = view_loop(view); + done: + free(tog_base_commit.id); free(keyword_idstr); free(in_repo_path); free(repo_path); @@ -6046,8 +6082,17 @@ cmd_diff(int argc, char *argv[]) ignore_whitespace, force_text_diff, NULL, repo); if (error) goto done; + + if (worktree) { + error = set_tog_base_commit(repo, worktree); + if (error != NULL) + goto done; + } + error = view_loop(view); + done: + free(tog_base_commit.id); free(keyword_idstr1); free(keyword_idstr2); free(label1); @@ -7194,13 +7239,21 @@ cmd_blame(int argc, char *argv[]) view_close(view); goto done; } + if (worktree) { + error = set_tog_base_commit(repo, worktree); + if (error != NULL) + goto done; + /* Release work tree lock. */ got_worktree_close(worktree); worktree = NULL; } + error = view_loop(view); + done: + free(tog_base_commit.id); free(repo_path); free(in_repo_path); free(link_target); @@ -8183,12 +8236,19 @@ cmd_tree(int argc, char *argv[]) } if (worktree) { + error = set_tog_base_commit(repo, worktree); + if (error != NULL) + goto done; + /* Release work tree lock. */ got_worktree_close(worktree); worktree = NULL; } + error = view_loop(view); + done: + free(tog_base_commit.id); free(keyword_idstr); free(repo_path); free(cwd); @@ -8196,6 +8256,8 @@ done: free(label); if (ref) got_ref_close(ref); + if (worktree != NULL) + got_worktree_close(worktree); if (repo) { const struct got_error *close_err = got_repo_close(repo); if (error == NULL) @@ -9032,14 +9094,23 @@ cmd_ref(int argc, char *argv[]) goto done; if (worktree) { + error = set_tog_base_commit(repo, worktree); + if (error != NULL) + goto done; + /* Release work tree lock. */ got_worktree_close(worktree); worktree = NULL; } + error = view_loop(view); + done: + free(tog_base_commit.id); free(repo_path); free(cwd); + if (worktree != NULL) + got_worktree_close(worktree); if (repo) { const struct got_error *close_err = got_repo_close(repo); if (close_err) -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68