Download raw body.
tog: limit feature
On 22-09-11 12:18PM, Omar Polo wrote:
> First of all, sorry for all this back and forward. I love the feature
> introduced by the diff but I haven't had the time to properly review
> it before (was side-tracked by other things) and this thread has
> already dragged on too much. Sorry for this, and thanks for
> continuing working on it!
>
> There is however a couple of points that i'd like to address before
> this is committed. Some comments inline and an updated diff attached
> below.
>
> > +}
> > blob - 65405972315ce384c286567d1765d330d8d9a13f
> > blob + 7f5838ca56a87f272be0f2ddb8af1b6feb75c238
> > --- tog/tog.1
> > +++ tog/tog.1
> > @@ -226,6 +226,12 @@ 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 &
> > +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.
>
> nitpick: what if we move this after the /-key description and just say
> that has the same behavior but hides the non-matching commits? We
> could drop some copy-paste then.
Nice! I like this idea :)
>
> I'm also not sure of the error handling in this function. If one of
> these function fails we exit the loop, leaking the items processed so
> far. I'm not fixing this in the diff below and can be sorted out
> later in-tree after this gets committed.
Agree with this too.
> The rest looks ok for me. Here's a diff that addresses the mentioned
> points.
This is ok for me. Thanks, op!
> As a follow-up i think that upon C-l during & we should free also the
> original list of commits, not only the matched queue.
>
> diff /home/op/w/got
> commit - d9787ed86ecec0bda7a570181d86c44ba80bd583
> path + /home/op/w/got
> blob - 4e49d36a9dfb5392b25c430b1fa90b30c898c3ed
> file + include/got_object.h
> --- include/got_object.h
> +++ include/got_object.h
> @@ -352,3 +352,6 @@ const struct got_error *got_object_commit_add_parent(s
> const struct got_error *got_object_tag_create(struct got_object_id **,
> const char *, struct got_object_id *, const char *,
> time_t, const char *, const char *, struct got_repository *, int verbosity);
> +
> +const struct got_error *got_object_commit_dup(struct got_commit_object **,
> + struct got_repository *, struct got_commit_object *);
> blob - 27d10eb82fe55e97ef790f60755120c3b6381917
> file + lib/object.c
> --- lib/object.c
> +++ lib/object.c
> @@ -2353,3 +2353,12 @@ done:
> free(path_packfile);
> return err;
> }
> +
> +const struct got_error *
> +got_object_commit_dup(struct got_commit_object **ret,
> + struct got_repository *repo, struct got_commit_object *commit)
> +{
> + *ret = commit;
> + commit->refcnt++;
> + return NULL;
> +}
> blob - 65405972315ce384c286567d1765d330d8d9a13f
> file + tog/tog.1
> --- tog/tog.1
> +++ tog/tog.1
> @@ -189,6 +189,10 @@ against a commit's author name, committer name, log me
> commit ID SHA1 hash.
> Regular expression syntax is documented in
> .Xr re_format 7 .
> +.It Cm &
> +Like
> +.Cm /
> +but hides the non-matching commits.
> .It Cm n
> Find the Nth next commit which matches the current search pattern (default: 1).
> .br
> blob - 7dd9d7c85c373e47b6c6c9bd21a4109d8af43d19
> file + tog/tog.c
> --- tog/tog.c
> +++ tog/tog.c
> @@ -344,7 +344,7 @@ struct tog_log_thread_args {
> int commits_needed;
> int load_all;
> struct got_commit_graph *graph;
> - struct commit_queue *commits;
> + struct commit_queue *real_commits;
> const char *in_repo_path;
> struct got_object_id *start_id;
> struct got_repository *repo;
> @@ -356,13 +356,18 @@ 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 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 +381,9 @@ 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;
> };
>
> #define TOG_COLOR_DIFF_MINUS 1
> @@ -935,8 +943,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);
> }
>
> @@ -2174,6 +2182,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,
> @@ -2197,17 +2206,64 @@ queue_commits(struct tog_log_thread_args *a)
> break;
> }
>
> - entry->idx = a->commits->ncommits;
> - TAILQ_INSERT_TAIL(&a->commits->head, entry, entry);
> - a->commits->ncommits++;
> + entry->idx = a->real_commits->ncommits;
> + TAILQ_INSERT_TAIL(&a->real_commits->head, entry, entry);
> + a->real_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;
> + struct got_object_id *newid;
> +
> + matched = calloc(1, sizeof(*matched));
> + if (matched == NULL)
> + return got_error_from_errno("malloc");
> +
> + newid = calloc(1, sizeof(*newid));
> + if (newid == NULL) {
> + free(matched);
> + return got_error_from_errno("calloc");
> + }
> + matched->id = newid;
> + memcpy(matched->id->sha1, entry->id->sha1,
> + SHA1_DIGEST_LENGTH);
> +
> + err = got_object_commit_dup(&matched->commit,
> + a->repo, entry->commit);
> + if (err)
> + break;
> +
> + matched->idx = a->limit_commits->ncommits;
> + TAILQ_INSERT_TAIL(&a->limit_commits->head,
> + matched, entry);
> + a->limit_commits->ncommits++;
> + }
> +
> + /*
> + * 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;
> + }
> +
> 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)
> +
> + 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;
> }
>
> @@ -2278,7 +2334,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");
> @@ -2286,6 +2342,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)
> @@ -2296,10 +2353,13 @@ draw_commits(struct tog_view *view)
> search_str = "searching...";
> }
>
> - if (asprintf(&ncommits_str, " [%d/%d] %s",
> - entry ? entry->idx + 1 : 0, s->commits.ncommits,
> - search_str ? search_str :
> - (refs_str ? refs_str : "")) == -1) {
> + if (s->limit_view && s->commits->ncommits == 0)
> + limit_str = "no 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 : ""),
> + limit_str ? limit_str : "") == -1) {
> err = got_error_from_errno("asprintf");
> goto done;
> }
> @@ -2424,7 +2484,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;
>
> @@ -2509,13 +2569,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;
> @@ -2755,8 +2815,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--;
> + }
>
> errcode = pthread_mutex_lock(&tog_mutex);
> if (errcode) {
> @@ -2765,10 +2830,14 @@ log_thread(void *arg)
> goto done;
> } else if (*a->quit)
> done = 1;
> - else if (*a->first_displayed_entry == NULL) {
> + else if (*a->limiting && *a->first_displayed_entry == NULL) {
> *a->first_displayed_entry =
> - TAILQ_FIRST(&a->commits->head);
> + 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->real_commits->head);
> + *a->selected_entry = *a->first_displayed_entry;
> }
>
> errcode = pthread_cond_signal(&a->commit_loaded);
> @@ -2872,7 +2941,8 @@ 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->limit_commits);
> + free_commits(&s->real_commits);
> free(s->in_repo_path);
> s->in_repo_path = NULL;
> free(s->start_id);
> @@ -2882,7 +2952,141 @@ close_log_view(struct tog_view *view)
> return err;
> }
>
> +/*
> + * 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, "&/");
> + 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 (*pattern == '\0') {
> + /*
> + * Safety measure for the situation where the user
> + * resets limit without previously limiting anything.
> + */
> + if (!s->limit_view)
> + return NULL;
> +
> + /*
> + * User could have pressed Ctrl+L, which refreshed the
> + * commit queues, it means we can't save previously
> + * (before limit took place) displayed entries,
> + * because they would point to already free'ed memory,
> + * so we are forced to always select first entry of
> + * the queue.
> + */
> + s->commits = &s->real_commits;
> + s->first_displayed_entry = TAILQ_FIRST(&s->real_commits.head);
> + s->selected_entry = s->first_displayed_entry;
> + s->selected = 0;
> + s->limit_view = 0;
> +
> + return NULL;
> + }
> +
> + if (regcomp(&s->limit_regex, pattern, REG_EXTENDED | REG_NEWLINE))
> + return NULL;
> +
> + s->limit_view = 1;
> +
> + /* Clear the screen while loading limit view */
> + s->first_displayed_entry = NULL;
> + s->last_displayed_entry = NULL;
> + s->selected_entry = NULL;
> + s->commits = &s->limit_commits;
> +
> + /* Prepare limit queue for new search */
> + free_commits(&s->limit_commits);
> + s->limit_commits.ncommits = 0;
> +
> + /* First process commits, which are in queue already */
> + TAILQ_FOREACH(entry, &s->real_commits.head, entry) {
> + int have_match = 0;
> +
> + err = match_commit(&have_match, entry->id,
> + entry->commit, &s->limit_regex);
> + if (err)
> + return err;
> +
> + if (have_match) {
> + struct commit_queue_entry *matched;
> + struct got_object_id *newid;
> +
> + matched = malloc(sizeof(*matched));
> + if (matched == NULL)
> + return got_error_from_errno("malloc");
> +
> + newid = calloc(1, sizeof(*newid));
> + if (newid == NULL) {
> + free(matched);
> + return got_error_from_errno("calloc");
> + }
> + matched->id = newid;
> + memcpy(matched->id->sha1, entry->id->sha1,
> + SHA1_DIGEST_LENGTH);
> +
> + err = got_object_commit_dup(&matched->commit,
> + s->repo, entry->commit);
> + if (err) {
> + free(matched);
> + free(newid);
> + return err;
> + }
> +
> + 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 */
> + 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;
> @@ -3028,9 +3232,14 @@ 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->limit_commits.ncommits = 0;
> +
> s->repo = repo;
> if (head_ref_name) {
> s->head_ref_name = strdup(head_ref_name);
> @@ -3104,7 +3313,8 @@ 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.real_commits = &s->real_commits;
> + s->thread_args.limit_commits = &s->limit_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;
> @@ -3115,6 +3325,9 @@ 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;
> done:
> if (err)
> close_log_view(view);
> @@ -3147,19 +3360,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;
> @@ -3172,15 +3385,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);
> @@ -3189,7 +3405,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)
> @@ -3200,10 +3416,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);
> }
> @@ -3223,7 +3439,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;
> @@ -3319,7 +3535,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)
> @@ -3334,6 +3550,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;
> @@ -3396,7 +3615,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;
> }
> @@ -3413,13 +3632,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;
> @@ -3489,7 +3708,8 @@ 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->real_commits);
> + free_commits(&s->limit_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
tog: limit feature