On 22-08-17 03:57PM, Omar Polo wrote:
> On 2022/08/17 16:40:09 +0300, Mikhail <mp39590@gmail.com> wrote:
> > This patch implements limit feature for tog log view.
> > 
> > We keep two queues of commits - one for commits matching the pattern and
> > another one for all known commits, when user wants to use the feature,
> > we substitute those queues. Because of that displaying and movement
> > functions has been touched only a little bit.
> 
> haven't tried/read the patch yet but i like the idea!  I was thinking
> about implementing something like this, I wanted it multiple times!
> So, thanks for implementing it! :)

Likewise; this will be super handy. Thanks, Mikhail!

> Will read, test and try to review later; just one minor nit I couldn't
> not spot:
> 
> > diff refs/heads/main refs/heads/limit3
> > commit - 4fcc9f7404ca2e0dd2ee085f09d6246587c6c503
> > commit + f82712fb5e22a442d08c14774fb7506d60dab20a
> > blob - cbd5233dd61d780f245c5bcb24d95b030a5a7676
> > blob + 46b73b4047ef7e83b7c97015b9138c5f2be57857
> > --- 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 L
> > +Limit commits to the subset of matching the pattern. Use 'all'
> 
> new sentence, new line :)
> 
> > +as a pattern to display all commits.
> 
> Also, I'd use the & key.  In less(1) it acts like "/" but shows only
> the matching line, which is what "L" does in tog if i've understood
> correctly.
> 
> I'd use the empty string "" to reset instead of `all'.  (again, like
> in less(1))

I also agree with op's suggestions here.

I, too, don't have the time to review just now but I did apply the patch
and had a quick play. This diff will need something like:

	if (s->first_displayed_entry == NULL)
		return NULL;

when we enter log_move_cursor_down() now too. If you try:

  $ tog
  L
  xyz
  j

it will segv as it's now possible to display a log with no commits.
This could even be added now.

But this also made me think of something else for the sake of UX: if
there are no matches, would it be better to display something like "no
matches found" in the status/headline like we do for '/' rather than
display an empty log? This could even be added later if it's indeed
worthwhile; I just thought of this as it happened. Otherwise this works
very well in my quick test and will be a useful addition.

I finish early tomorrow so will be able to review then if not already
done.

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68