tog: limit feature

On 22-08-17 08:14PM, Mikhail wrote: > This version incorporates yours and Omar's suggestions. > > Appreciated for the suggestions and the review. I was able to test this a bit further and have found a couple minor issues; the first is only cosmetic but the other produces invalid search results. The first is a stale index: $ tog # 80x24 in got.git & # input: bug return # load limit view G & # input: "" (empty string) to load all commits return *index in headline shows [1/4510] but the selected entry is 23/4510* Second is invalid search: $ tog # 80x24 & # input: bug return # load limit view 15G & # input: "" return / # input: seg *commit 20282b029a is selected but does not contain "seg"* The problem is that we're _not_ saving the selected line index--only the selected_entry pointer--so in the first case we're keeping the limit view index when we return to the view of all commits, and this also causes the second bug as we're traversing search results from a bogus start index. The below diff applied on top of yours fixes these by saving and restoring the selected index from the real_commits view. Also, regarding the previous suggestion of displaying "no matches", do you think this can be shown in the current log view (of real_commits) such that an empty log isn't displayed at all? It would be a nice convenience to save the user from entering &<ret> to go back after encountering an empty log. Actually, this could even be added later tbh. I think this works really well! There are some minor nits and suggestions annotated inline, after which, with the abovementioned fix, I think it's ok :) diff /home/mark/src/got commit - ffc5154c8d703e82c65a7abc5e06fc493db458e1 path + /home/mark/src/got blob - 389d3fc95356d38745aa3ae0ea5769c703bef6a8 file + tog/tog.c --- tog/tog.c +++ tog/tog.c @@ -371,6 +371,7 @@ struct tog_log_view_state { struct commit_queue_entry *selected_entry; struct commit_queue real_commits; int selected; + int saved_selected; char *in_repo_path; char *head_ref_name; int log_branches; @@ -2993,6 +2994,7 @@ limit_log_view(struct tog_view *view) s->saved_last_displayed_entry; s->selected_entry = s->saved_selected_entry; s->commits = &s->real_commits; + s->selected = s->saved_selected; s->limit_view = 0; } return NULL; @@ -3010,6 +3012,7 @@ limit_log_view(struct tog_view *view) s->last_displayed_entry; s->saved_selected_entry = s->selected_entry; + s->saved_selected = s->selected; } s->first_displayed_entry = NULL; s->last_displayed_entry = NULL; @@ -3221,6 +3224,7 @@ open_log_view(struct tog_view *view, struct got_object s->saved_first_displayed_entry = NULL; s->saved_last_displayed_entry = NULL; s->saved_selected_entry = NULL; + s->saved_selected = 0; s->limit_commits.ncommits = 0; s->repo = repo; > diff refs/heads/main refs/heads/limit3 > commit - 4fcc9f7404ca2e0dd2ee085f09d6246587c6c503 > commit + e97eba33d529e9107268ef7d013c9d84b6a1710b > blob - cbd5233dd61d780f245c5bcb24d95b030a5a7676 > blob + de7505037613146ca4b909f7375df7a15f95fbdb > --- tog/tog.1 > +++ tog/tog.1 > @@ -220,6 +220,9 @@ This can then be used to open a new > view for arbitrary branches and tags. > .It Cm @ > Toggle between showing the author and the committer name. > +.It Cm & > +Limit commits to the subset of matching the pattern. This is a bit terse as there's no mention of where the pattern comes from or what it matches; perhaps something similar to the '/' docs: Prompt for a pattern to display a log view limited to the subset of commits matching the provided pattern, which is an extended regular expression matched against a commit's author name, committer name, log message, and commit ID SHA1 hash. > +Use empty string as a pattern to display all commits. > .El > .Pp > The options for > blob - 6ec8b5f0c155fafe55ee2aab0653d259876e6254 > blob + 389d3fc95356d38745aa3ae0ea5769c703bef6a8 > --- tog/tog.c > +++ tog/tog.c > @@ -356,13 +356,20 @@ struct tog_log_thread_args { > int *searching; > int *search_next_done; > regex_t *regex; > + int *limiting; > + int limit_match; > + regex_t *limit_regex; > + struct commit_queue *limit_commits; > + struct commit_queue_entry **saved_first_displayed_entry; > + struct commit_queue_entry **saved_selected_entry; > }; > > struct tog_log_view_state { > - struct commit_queue commits; > + struct commit_queue *commits; > struct commit_queue_entry *first_displayed_entry; > struct commit_queue_entry *last_displayed_entry; > struct commit_queue_entry *selected_entry; > + struct commit_queue real_commits; > int selected; > char *in_repo_path; > char *head_ref_name; > @@ -376,6 +383,13 @@ struct tog_log_view_state { > struct commit_queue_entry *search_entry; > struct tog_colors colors; > int use_committer; > + int limit_view; > + regex_t limit_regex; > + struct commit_queue limit_commits; > + struct commit_queue *saved_commits; > + struct commit_queue_entry *saved_first_displayed_entry; > + struct commit_queue_entry *saved_last_displayed_entry; > + struct commit_queue_entry *saved_selected_entry; > }; > > #define TOG_COLOR_DIFF_MINUS 1 > @@ -935,8 +949,8 @@ resize_log_view(struct tog_view *view, int increase) > * Request commits to account for the increased > * height so we have enough to populate the view. > */ > - if (s->commits.ncommits < n) { > - view->nscrolled = n - s->commits.ncommits + increase + 1; > + if (s->commits->ncommits < n) { > + view->nscrolled = n - s->commits->ncommits + increase + 1; > err = request_log_commits(view); > } > > @@ -2167,6 +2181,7 @@ queue_commits(struct tog_log_thread_args *a) > struct got_object_id *id; > struct got_commit_object *commit; > struct commit_queue_entry *entry; > + int limit_match = 0; > int errcode; > > err = got_commit_graph_iter_next(&id, a->graph, a->repo, > @@ -2194,14 +2209,49 @@ queue_commits(struct tog_log_thread_args *a) > TAILQ_INSERT_TAIL(&a->commits->head, entry, entry); > a->commits->ncommits++; > > + if (*a->limiting) { > + err = match_commit(&limit_match, id, commit, > + a->limit_regex); > + if (err) > + break; > + > + if (limit_match) { > + struct commit_queue_entry *matched; > + > + matched = > + malloc(sizeof(struct commit_queue_entry)); > + memcpy(matched, entry, > + sizeof(struct commit_queue_entry)); > + > + matched->idx = a->limit_commits->ncommits; > + TAILQ_INSERT_TAIL(&a->limit_commits->head, matched, > + entry); > + a->limit_commits->ncommits++; > + } > + > + /* > + * This is how we signal to log_thread(), that we have > + * found match, and that it should be accounted as new > + * entry for the view. > + */ grammar: /* * This is how we signal log_thread() that we * have found a match, and that it should be * counted as a new entry for the view. */ > + a->limit_match = limit_match; > + > + } delete extra '\n' between `a->limit_match = limit_match;` and `}` > if (*a->searching == TOG_SEARCH_FORWARD && > !*a->search_next_done) { > int have_match; > err = match_commit(&have_match, id, commit, a->regex); > if (err) > break; > - if (have_match) > - *a->search_next_done = TOG_SEARCH_HAVE_MORE; > + > + if (*a->limiting) { > + if (limit_match && have_match) > + *a->search_next_done = > + TOG_SEARCH_HAVE_MORE; > + } else if (have_match) > + *a->search_next_done = > + TOG_SEARCH_HAVE_MORE; > } deindent and remove wrap: } else if (have_match) *a->search_next_done = TOG_SEARCH_HAVE_MORE; > > errcode = pthread_mutex_unlock(&tog_mutex); > @@ -2271,7 +2321,7 @@ draw_commits(struct tog_view *view) > > if (s->thread_args.commits_needed > 0 || s->thread_args.load_all) { > if (asprintf(&ncommits_str, " [%d/%d] %s", > - entry ? entry->idx + 1 : 0, s->commits.ncommits, > + entry ? entry->idx + 1 : 0, s->commits->ncommits, > (view->searching && !view->search_next_done) ? > "searching..." : "loading...") == -1) { > err = got_error_from_errno("asprintf"); > @@ -2279,6 +2329,7 @@ draw_commits(struct tog_view *view) > } > } else { > const char *search_str = NULL; > + const char *limit_str = NULL; > > if (view->searching) { > if (view->search_next_done == TOG_SEARCH_NO_MORE) > @@ -2289,10 +2340,15 @@ draw_commits(struct tog_view *view) > search_str = "searching..."; > } > > - if (asprintf(&ncommits_str, " [%d/%d] %s", > - entry ? entry->idx + 1 : 0, s->commits.ncommits, > + if (s->limit_view) > + if (s->commits->ncommits == 0) > + limit_str = "no limit matches found"; > + > + if (asprintf(&ncommits_str, " [%d/%d] %s %s", > + entry ? entry->idx + 1 : 0, s->commits->ncommits, > search_str ? search_str : > - (refs_str ? refs_str : "")) == -1) { > + (refs_str ? refs_str : ""), > + limit_str ? limit_str : "") == -1) { > err = got_error_from_errno("asprintf"); > goto done; > } > @@ -2419,7 +2475,7 @@ log_scroll_up(struct tog_log_view_state *s, int maxscr > struct commit_queue_entry *entry; > int nscrolled = 0; > > - entry = TAILQ_FIRST(&s->commits.head); > + entry = TAILQ_FIRST(&s->commits->head); > if (s->first_displayed_entry == entry) > return; > > @@ -2504,13 +2560,13 @@ log_scroll_down(struct tog_view *view, int maxscroll) > return NULL; > > ncommits_needed = s->last_displayed_entry->idx + 1 + maxscroll; > - if (s->commits.ncommits < ncommits_needed && > + if (s->commits->ncommits < ncommits_needed && > !s->thread_args.log_complete) { > /* > * Ask the log thread for required amount of commits. > */ > s->thread_args.commits_needed += > - ncommits_needed - s->commits.ncommits; > + ncommits_needed - s->commits->ncommits; > err = trigger_log_thread(view, 1); > if (err) > return err; > @@ -2750,8 +2806,13 @@ log_thread(void *arg) > goto done; > err = NULL; > done = 1; > - } else if (a->commits_needed > 0 && !a->load_all) > - a->commits_needed--; > + } else if (a->commits_needed > 0 && !a->load_all) { > + if (*a->limiting) { > + if (a->limit_match) > + a->commits_needed--; > + } else > + a->commits_needed--; I'm not averse to one lengthy predicate but it's subjective: } else if (a->commits_needed > 0 && !a->load_all && (!*a->limiting || (*a->limiting && a->limit_match))) a->commits_needed--; > + } > > errcode = pthread_mutex_lock(&tog_mutex); > if (errcode) { > @@ -2760,6 +2821,13 @@ log_thread(void *arg) > goto done; > } else if (*a->quit) > done = 1; > + else if (*a->limiting && > + *a->first_displayed_entry == NULL) { > + *a->first_displayed_entry = > + TAILQ_FIRST(&a->limit_commits->head); > + *a->selected_entry = > + *a->first_displayed_entry; > + } > else if (*a->first_displayed_entry == NULL) { > *a->first_displayed_entry = > TAILQ_FIRST(&a->commits->head); style(9): indent, word wrap, and `} else if` on same line: else if (*a->limiting && *a->first_displayed_entry == NULL) { *a->first_displayed_entry = TAILQ_FIRST(&a->limit_commits->head); *a->selected_entry = *a->first_displayed_entry; } else if (*a->first_displayed_entry == NULL) { *a->first_displayed_entry = TAILQ_FIRST(&a->commits->head); > @@ -2867,7 +2935,7 @@ close_log_view(struct tog_view *view) > if (errcode && err == NULL) > err = got_error_set_errno(errcode, "pthread_cond_destroy"); > > - free_commits(&s->commits); > + free_commits(s->commits); > free(s->in_repo_path); > s->in_repo_path = NULL; > free(s->start_id); > @@ -2877,7 +2945,128 @@ close_log_view(struct tog_view *view) > return err; > } > > +/* > + * We use two queues to implement limit feature, first one consists of commits > + * matching current limit_regex, second one is the real queue, of all known > + * commits (real_commits). Then, when use starts the limiting we swap the > + * queues, because of that all movement and displaying functionality works with > + * very slight change. > + */ grammar: * We use two queues to implement the limit feature: first consists of commits * matching the current limit_regex; second is the real queue of all known * commits (real_commits). When the user starts limiting, we swap queues such * that all movement and displaying functionality works with very slight change. > static const struct got_error * > +limit_log_view(struct tog_view *view) > +{ > + struct tog_log_view_state *s = &view->state.log; > + struct commit_queue_entry *entry; > + struct tog_view *v = view; > + const struct got_error *err = NULL; > + char pattern[1024]; > + int ret; > + > + if (view_is_hsplit_top(view)) > + v = view->child; > + else if (view->mode == TOG_VIEW_SPLIT_VERT && view->parent) > + v = view->parent; > + > + /* Get the pattern */ > + wmove(v->window, v->nlines - 1, 0); > + wclrtoeol(v->window); > + mvwaddstr(v->window, v->nlines - 1, 0, "limit: "); > + nodelay(v->window, FALSE); > + nocbreak(); > + echo(); > + ret = wgetnstr(v->window, pattern, sizeof(pattern)); > + cbreak(); > + noecho(); > + nodelay(v->window, TRUE); > + if (ret == ERR) > + return NULL; > + > + if (strcmp(pattern, "") == 0) { With op's suggestion now applied, I would drop the call to strcmp() and check directly: if (*pattern == '\0') { > + /* > + * Safety measures for situation when user limit 'all' without > + * previously limiting anything. > + */ The comment is a bit stale with op's suggestion applied as the special keyword "all" is no longer used: * Safety measure for the situation where the user * resets limit without previously limiting anything. > + if (s->limit_view == 1) { > + s->first_displayed_entry = > + s->saved_first_displayed_entry; > + s->last_displayed_entry = > + s->saved_last_displayed_entry; > + s->selected_entry = s->saved_selected_entry; > + s->commits = &s->real_commits; > + s->limit_view = 0; > + } > + return NULL; I would check if we're not in a limit view and return early so we can deindent and don't have to wrap: if (!s->limit_view) return NULL; s->first_displayed_entry = s->saved_first_displayed_entry; s->last_displayed_entry = s->saved_last_displayed_entry; s->selected_entry = s->saved_selected_entry; s->commits = &s->real_commits; s->selected = s->saved_selected; s->limit_view = 0; return NULL; > + } else { Similarly, we can drop the else and deindent its scope, which removes a lot of wrapping: > + /* > + * This check is needed because user can issue limit "inside" > + * limit, in this case don't overwrite saved entries, and start > + * the procedure again. > + */ > + if (s->limit_view == 0) { > + s->limit_view = 1; > + s->saved_first_displayed_entry = s->first_displayed_entry; > + s->saved_last_displayed_entry = s->last_displayed_entry; > + s->saved_selected_entry = s->selected_entry; > + } > + s->first_displayed_entry = NULL; > + s->last_displayed_entry = NULL; > + s->selected_entry = NULL; > + s->commits = &s->limit_commits; > + regcomp(&s->limit_regex, pattern, REG_EXTENDED | REG_NEWLINE); We should check regcomp() for error, and I would hoist it above where we set s->limit_view so that, if it fails, we don't load the limit view (i.e., it's a no-op like view_search_start() when regcomp() fails): if (regcomp(&s->limit_regex, pattern, REG_EXTENDED | REG_NEWLINE)) return NULL; /* * This check is needed because user can issue limit "inside" * limit, in this case don't overwrite saved entries, and start * the procedure again. */ if (s->limit_view == 0) { s->limit_view = 1; ... > + > + /* Prepare limit queue for new search */ > + while ((entry = TAILQ_FIRST(&s->limit_commits.head))) { > + TAILQ_REMOVE(&s->limit_commits.head, entry, entry); > + free(entry); > + } > + s->limit_commits.ncommits = 0; > + > + /* First process commits, which are in queue already */ > + TAILQ_FOREACH(entry, &s->real_commits.head, entry) { > + struct commit_queue_entry *matched; > + int have_match = 0; > + > + err = match_commit(&have_match, entry->id, entry->commit, > + &s->limit_regex); > + if (err) > + return err; > + > + if (have_match) { > + matched = malloc(sizeof(struct commit_queue_entry)); > + memcpy(matched, entry, > + sizeof(struct commit_queue_entry)); > + > + matched->idx = s->limit_commits.ncommits; > + TAILQ_INSERT_TAIL(&s->limit_commits.head, matched, > + entry); > + s->limit_commits.ncommits++; > + } > + > + } > + > + /* Second process all the commits, until we fill the screen */ > + while (1) { > + if (s->limit_commits.ncommits < view->nlines - 1 && > + !s->thread_args.log_complete) { > + s->thread_args.commits_needed++; > + trigger_log_thread(view, 1); > + } else > + break; > + } trigger_log_thread() needs an error check, but I would also simplify this by dropping the while loop and computing total commits_needed: if (s->limit_commits.ncommits < view->nlines - 1 && !s->thread_args.log_complete) { s->thread_args.commits_needed += view->nlines - s->limit_commits.ncommits - 1; err = trigger_log_thread(view, 1); if (err) return err; } > + } > + > + s->first_displayed_entry = TAILQ_FIRST(&s->commits->head); > + s->selected_entry = TAILQ_FIRST(&s->commits->head); > + s->selected = 0; > + > + return NULL; > +} > + > +static const struct got_error * > search_start_log_view(struct tog_view *view) > { > struct tog_log_view_state *s = &view->state.log; > @@ -3023,9 +3212,17 @@ open_log_view(struct tog_view *view, struct got_object > } > > /* The commit queue only contains commits being displayed. */ > - TAILQ_INIT(&s->commits.head); > - s->commits.ncommits = 0; > + TAILQ_INIT(&s->real_commits.head); > + s->real_commits.ncommits = 0; > + s->commits = &s->real_commits; > > + TAILQ_INIT(&s->limit_commits.head); > + s->limit_view = 0; > + s->saved_first_displayed_entry = NULL; > + s->saved_last_displayed_entry = NULL; > + s->saved_selected_entry = NULL; > + s->limit_commits.ncommits = 0; > + > s->repo = repo; > if (head_ref_name) { > s->head_ref_name = strdup(head_ref_name); > @@ -3099,7 +3296,7 @@ open_log_view(struct tog_view *view, struct got_object > > s->thread_args.commits_needed = view->nlines; > s->thread_args.graph = thread_graph; > - s->thread_args.commits = &s->commits; > + s->thread_args.commits = s->commits; > s->thread_args.in_repo_path = s->in_repo_path; > s->thread_args.start_id = s->start_id; > s->thread_args.repo = thread_repo; > @@ -3110,6 +3307,13 @@ open_log_view(struct tog_view *view, struct got_object > s->thread_args.searching = &view->searching; > s->thread_args.search_next_done = &view->search_next_done; > s->thread_args.regex = &view->regex; > + 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.saved_first_displayed_entry = > + &s->saved_first_displayed_entry; > + s->thread_args.saved_selected_entry = > + &s->saved_selected_entry; > done: > if (err) > close_log_view(view); > @@ -3142,19 +3346,19 @@ log_move_cursor_up(struct tog_view *view, int page, in > { > struct tog_log_view_state *s = &view->state.log; > > + if (s->first_displayed_entry == NULL) > + return; > if (s->selected_entry->idx == 0) > view->count = 0; > - if (s->first_displayed_entry == NULL) > - return; > > - if ((page && TAILQ_FIRST(&s->commits.head) == s->first_displayed_entry) > + if ((page && TAILQ_FIRST(&s->commits->head) == s->first_displayed_entry) > || home) > s->selected = home ? 0 : MAX(0, s->selected - page - 1); > > if (!page && !home && s->selected > 0) > --s->selected; > else > - log_scroll_up(s, home ? s->commits.ncommits : MAX(page, 1)); > + log_scroll_up(s, home ? s->commits->ncommits : MAX(page, 1)); > > select_commit(s); > return; > @@ -3167,15 +3371,18 @@ log_move_cursor_down(struct tog_view *view, int page) > const struct got_error *err = NULL; > int eos = view->nlines - 2; > > + if (s->first_displayed_entry == NULL) > + return NULL; > + > if (s->thread_args.log_complete && > - s->selected_entry->idx >= s->commits.ncommits - 1) > + s->selected_entry->idx >= s->commits->ncommits - 1) > return NULL; > > if (view_is_hsplit_top(view)) > --eos; /* border consumes the last line */ > > if (!page) { > - if (s->selected < MIN(eos, s->commits.ncommits - 1)) > + if (s->selected < MIN(eos, s->commits->ncommits - 1)) > ++s->selected; > else > err = log_scroll_down(view, 1); > @@ -3184,7 +3391,7 @@ log_move_cursor_down(struct tog_view *view, int page) > int n; > > s->selected = 0; > - entry = TAILQ_LAST(&s->commits.head, commit_queue_head); > + entry = TAILQ_LAST(&s->commits->head, commit_queue_head); > s->last_displayed_entry = entry; > for (n = 0; n <= eos; n++) { > if (entry == NULL) > @@ -3195,10 +3402,10 @@ log_move_cursor_down(struct tog_view *view, int page) > if (n > 0) > s->selected = n - 1; > } else { > - if (s->last_displayed_entry->idx == s->commits.ncommits - 1 && > + if (s->last_displayed_entry->idx == s->commits->ncommits - 1 && > s->thread_args.log_complete) > s->selected += MIN(page, > - s->commits.ncommits - s->selected_entry->idx - 1); > + s->commits->ncommits - s->selected_entry->idx - 1); > else > err = log_scroll_down(view, page); > } > @@ -3218,7 +3425,7 @@ log_move_cursor_down(struct tog_view *view, int page) > select_commit(s); > > if (s->thread_args.log_complete && > - s->selected_entry->idx == s->commits.ncommits - 1) > + s->selected_entry->idx == s->commits->ncommits - 1) > view->count = 0; > > return NULL; > @@ -3314,7 +3521,7 @@ input_log_view(struct tog_view **new_view, struct tog_ > if (ch == CTRL('g') || ch == KEY_BACKSPACE) > s->thread_args.load_all = 0; > else if (s->thread_args.log_complete) { > - err = log_move_cursor_down(view, s->commits.ncommits); > + err = log_move_cursor_down(view, s->commits->ncommits); > s->thread_args.load_all = 0; > } > if (err) > @@ -3329,6 +3536,9 @@ input_log_view(struct tog_view **new_view, struct tog_ > return log_goto_line(view, eos); > > switch (ch) { > + case '&': > + err = limit_log_view(view); > + break; > case 'q': > s->quit = 1; > break; > @@ -3391,7 +3601,7 @@ input_log_view(struct tog_view **new_view, struct tog_ > s->thread_args.load_all = 1; > if (!s->thread_args.log_complete) > return trigger_log_thread(view, 0); > - err = log_move_cursor_down(view, s->commits.ncommits); > + err = log_move_cursor_down(view, s->commits->ncommits); > s->thread_args.load_all = 0; > break; > } > @@ -3408,13 +3618,13 @@ input_log_view(struct tog_view **new_view, struct tog_ > case KEY_RESIZE: > if (s->selected > view->nlines - 2) > s->selected = view->nlines - 2; > - if (s->selected > s->commits.ncommits - 1) > - s->selected = s->commits.ncommits - 1; > + if (s->selected > s->commits->ncommits - 1) > + s->selected = s->commits->ncommits - 1; > select_commit(s); > - if (s->commits.ncommits < view->nlines - 1 && > + if (s->commits->ncommits < view->nlines - 1 && > !s->thread_args.log_complete) { > s->thread_args.commits_needed += (view->nlines - 1) - > - s->commits.ncommits; > + s->commits->ncommits; > err = trigger_log_thread(view, 1); > } > break; > @@ -3484,7 +3694,7 @@ input_log_view(struct tog_view **new_view, struct tog_ > s->start_id, s->repo, NULL, NULL); > if (err) > return err; > - free_commits(&s->commits); > + free_commits(s->commits); > s->first_displayed_entry = NULL; > s->last_displayed_entry = NULL; > s->selected_entry = NULL; > -- Mark Jamsek <fnc.bsdbox.org> GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68

2022-08-17 15:33 Mark Jamsek:
tog: limit feature
- 2022-08-17 17:14 Mikhail:
  tog: limit feature
- - 2022-08-19 12:28 Mark Jamsek:
    tog: limit feature
  - - 2022-08-19 14:48 Mikhail:
      tog: limit feature
    - - 2022-08-19 15:55 Mark Jamsek:
        tog: limit feature