Download raw body.
show base commit markers in tog log view
Stefan Sperling <stsp@stsp.name> wrote:
> On Fri, Jul 21, 2023 at 03:14:06PM +0200, Stefan Sperling wrote:
> > 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 ~?
>
> The above suggestion I made results in a performance regression,
> as Omar pointed out on IRC:
>
> 20:47 <@op2> one consequence of the new work tree base commit marker
> in tog is that it is slower to open in src.git :/
> 20:48 <@op2> xterm stays blank for ~2 seconds before it start listing
> commits, while in 0.90 it was istantaneous
>
> The patch below attempts to fix this regression by letting the
> log thread walk the file index once it has fetched the initial
> batch of commits required to fill the display. In my testing this
> lets tog start up as fast at it did before and already accept input
> before the marker appears in a large work tree.
> The delay is still noticable in some cases, e.g. when pressing page-down
> right after starting tog, the display will only scroll further once the
> commit marker has been drawn. But this is a small one-time lag and I could
> live with it.
I actually tested with src.git and didn't notice such a delay, but
that's likely due to machine differences. I'm on a T14s Gen 3 i7-1260P
and the load delay is negligible for me; not instantaneous but sub 0.5s.
This approach is much better though!
The diff works as expected for me but we need to keep one check that's
needed else we draw garbage when tog_base_commit.marker is uninitialised
(annotated below). However, we also need to add the wait instruction to
tog log because it breaks regress as it is. I can investigate this in an
hour or two though.
> Regarding 'got branch -l' I am podering moving mixed-commit detection
> to 'got status' since this command is already walking the file index.
> Or will we accept branch -l being slower now?
Likewise, the performance impact on 'got branch -l' is negligible for me
so I'm equally happy to keep it as is or move it to 'got status' :)
I suppose, now that we show it in tog, it won't be as missed from
'branch -l'? Or perhaps we could use another flag for annotating the
state of the work tree's current branch like we do now (got branch -s)?
Although I'm not really a fan of that last idea tbh.
> -----------------------------------------------
>
> load tog's worktree base commit marker in the log thread for startup speed
>
> diff ccdddf69864eb9ac5c10c0f5d58b68df1cf26ea1 762988f1190e744a7116962daa9baf8d00f8c1ec
> commit - ccdddf69864eb9ac5c10c0f5d58b68df1cf26ea1
> commit + 762988f1190e744a7116962daa9baf8d00f8c1ec
> blob - 07cb22fd98f7b10c110a8a5d0c0627770c5b7e76
> blob + 02e203f9626eba60a84658a1f9e7a8d0fb043418
> --- include/got_worktree.h
> +++ include/got_worktree.h
> @@ -137,13 +137,14 @@ const struct got_error *got_worktree_set_base_commit_i
> * 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.
> + * GOT_WORKTREE_STATE_UPTODATE, else it will be GOT_WORKTREE_STATE_OUTOFDATE.
> */
> const struct got_error *got_worktree_get_state(char *,
> struct got_repository *, struct got_worktree *);
>
> -#define GOT_WORKTREE_UPTODATE '*'
> -#define GOT_WORKTREE_OUTOFDATE '~'
> +#define GOT_WORKTREE_STATE_UNKNOWN ' '
> +#define GOT_WORKTREE_STATE_UPTODATE '*'
> +#define GOT_WORKTREE_STATE_OUTOFDATE '~'
>
> /*
> * Obtain a parsed representation of this worktree's got.conf file.
> blob - 6b14bc178bf5fbc6065088656b0ecae4a8fd2aad
> blob + e0c7318204bffd754947e3848dcb9e7b6ad3ae9e
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -3273,7 +3273,7 @@ got_worktree_get_state(char *state, struct got_reposit
> if (err)
> goto done;
>
> - *state = GOT_WORKTREE_OUTOFDATE;
> + *state = GOT_WORKTREE_STATE_UNKNOWN;
> base_id = got_worktree_get_base_commit_id(worktree);
>
> if (got_object_id_cmp(base_id, head_id) == 0) {
> @@ -3284,10 +3284,13 @@ got_worktree_get_state(char *state, struct got_reposit
> 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)
> + *state = GOT_WORKTREE_STATE_UPTODATE;
> + else if (err->code == GOT_ERR_MIXED_COMMITS) {
> + *state = GOT_WORKTREE_STATE_OUTOFDATE;
> err = NULL;
> - }
> + }
> + } else
> + *state = GOT_WORKTREE_STATE_OUTOFDATE;
>
> done:
> free(head_id);
> blob - 897b0c14098c0b8440dbdfbedd2bb8c0792e6efb
> blob + 118398e8a4c1738580fccf502b243173d3cd685a
> --- tog/tog.c
> +++ tog/tog.c
> @@ -380,6 +380,8 @@ struct tog_log_thread_args {
> int limit_match;
> regex_t *limit_regex;
> struct commit_queue *limit_commits;
> + struct got_worktree *worktree;
> + int need_commit_marker;
> };
>
> struct tog_log_view_state {
> @@ -730,7 +732,7 @@ static const struct got_error *open_log_view(struct to
>
> static const struct got_error *open_log_view(struct tog_view *,
> struct got_object_id *, struct got_repository *,
> - const char *, const char *, int);
> + const char *, const char *, int, struct got_worktree *);
> static const struct got_error * show_log_view(struct tog_view *);
> static const struct got_error *input_log_view(struct tog_view **,
> struct tog_view *, int);
> @@ -2423,6 +2425,10 @@ draw_commit(struct tog_view *view, struct commit_queue
> struct tog_color *tc;
> struct got_reflist_head *refs;
>
> + 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;
> +
> committer_time = got_object_commit_get_committer_time(commit);
> if (gmtime_r(&committer_time, &tm) == NULL)
> return got_error_from_errno("gmtime_r");
> @@ -2482,7 +2488,7 @@ draw_commit(struct tog_view *view, struct commit_queue
> waddwstr(view->window, wauthor);
> col += author_width;
> while (col < avail && author_width < author_display_cols + 2) {
> - if (tog_base_commit.id != NULL &&
> + if (tog_base_commit.marker != GOT_WORKTREE_STATE_UNKNOWN &&
we need to keep the tog_base_commit.id != NULL check here otherwise we
draw garbage in 'tog log -r repo' cases because tog_base_commit.marker
is uninitialised:
if (tog_base_commit.id != NULL &&
tog_base_commit.marker != GOT_WORKTREE_STATE_UNKNOWN &&
author_width == marker_column &&
entry->idx == tog_base_commit.idx) {
...
}
> author_width == marker_column &&
> entry->idx == tog_base_commit.idx) {
> tc = get_color(&s->colors, TOG_COLOR_COMMIT);
> @@ -2709,10 +2715,6 @@ 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);
> @@ -3360,6 +3362,34 @@ log_thread(void *arg)
> goto done;
> }
>
> + if (a->commits_needed == 0 &&
> + a->need_commit_marker && a->worktree) {
> + errcode = pthread_mutex_unlock(&tog_mutex);
> + if (errcode) {
> + err = got_error_set_errno(errcode,
> + "pthread_mutex_unlock");
> + goto done;
> + }
> + err = got_worktree_get_state(
> + &tog_base_commit.marker,
> + a->repo, a->worktree);
The got_worktree_get_state() call can be reflowed to two lines:
err = got_worktree_get_state(&tog_base_commit.marker,
a->repo, a->worktree);
> + if (err)
> + goto done;
> + errcode = pthread_mutex_lock(&tog_mutex);
> + if (errcode) {
> + err = got_error_set_errno(errcode,
> + "pthread_mutex_lock");
> + goto done;
> + }
> + a->need_commit_marker = 0;
> + /*
> + * The main thread did not close this
> + * work tree yet. Close it now.
> + */
> + got_worktree_close(a->worktree);
> + a->worktree = NULL;
> + }
> +
> if (done)
> a->commits_needed = 0;
> else {
> @@ -3385,6 +3415,10 @@ done:
> if (err) {
> tog_thread_error = 1;
> pthread_cond_signal(&a->commit_loaded);
> + if (a->worktree) {
> + got_worktree_close(a->worktree);
> + a->worktree = NULL;
> + }
> }
> return (void *)err;
> }
> @@ -3705,7 +3739,8 @@ open_log_view(struct tog_view *view, struct got_object
> static const struct got_error *
> open_log_view(struct tog_view *view, struct got_object_id *start_id,
> struct got_repository *repo, const char *head_ref_name,
> - const char *in_repo_path, int log_branches)
> + const char *in_repo_path, int log_branches,
> + struct got_worktree *worktree)
> {
> const struct got_error *err = NULL;
> struct tog_log_view_state *s = &view->state.log;
> @@ -3820,6 +3855,9 @@ done:
> s->thread_args.limiting = &s->limit_view;
> s->thread_args.limit_regex = &s->limit_regex;
> s->thread_args.limit_commits = &s->limit_commits;
> + s->thread_args.worktree = worktree;
> + if (worktree)
> + s->thread_args.need_commit_marker = 1;
> done:
> if (err) {
> if (view->close == NULL)
> @@ -4394,9 +4432,8 @@ set_tog_base_commit(struct got_repository *repo, struc
> 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);
> + tog_base_commit.marker = GOT_WORKTREE_STATE_UNKNOWN;
> + return NULL;
> }
>
> static const struct got_error *
> @@ -4551,18 +4588,20 @@ cmd_log(int argc, char *argv[])
> error = got_error_from_errno("view_open");
> goto done;
> }
> +
> + if (worktree) {
> + error = set_tog_base_commit(repo, worktree);
> + if (error != NULL)
> + goto done;
> + }
> +
> error = open_log_view(view, start_id, repo, head_ref_name,
> - in_repo_path, log_branches);
> + in_repo_path, log_branches, worktree);
> 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);
> + /* The work tree will be closed by the log thread. */
> worktree = NULL;
> }
>
> @@ -6814,7 +6853,7 @@ log_annotated_line(struct tog_view **new_view, int beg
> if (log_view == NULL)
> return got_error_from_errno("view_open");
>
> - err = open_log_view(log_view, id, repo, GOT_REF_HEAD, "", 0);
> + err = open_log_view(log_view, id, repo, GOT_REF_HEAD, "", 0, NULL);
> if (err)
> view_close(log_view);
> else
> @@ -7616,7 +7655,7 @@ log_selected_tree_entry(struct tog_view **new_view, in
> return err;
>
> err = open_log_view(log_view, s->commit_id, s->repo, s->head_ref_name,
> - path, 0);
> + path, 0, NULL);
> if (err)
> view_close(log_view);
> else
> @@ -8474,7 +8513,7 @@ log_ref_entry(struct tog_view **new_view, int begin_y,
> }
>
> err = open_log_view(log_view, commit_id, repo,
> - got_ref_get_name(re->ref), "", 0);
> + got_ref_get_name(re->ref), "", 0, NULL);
> done:
> if (err)
> view_close(log_view);
--
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68
show base commit markers in tog log view