Stefan Sperling <stsp@stsp.name> wrote:
> On Thu, Aug 08, 2024 at 05:21:36PM +1000, Mark Jamsek wrote:
> > Mark Jamsek <mark@jamsek.com> wrote:
> > > The proposed change enables "tagging" the commits with the 't' keymap
> > > instead. So the user can scroll to the first commit, enter 't', and
> > > then scroll to the next commit and enter 't' to open a diff view of the
> > > changes between the tagged commits. Entering 't' on an already tagged
> > > commit untags it. Entering 'return' on any commit untags tagged commits.
> > > 
> > > I've attached two variants of this feature because I can't decide which
> > > I prefer.
> > > 
> > > topmost diff:
> > >   When the second commit is tagged, the diff view opens and the tagged
> > >   commits automatically become untagged.
> > > 
> > > bottom diff:
> > >   When the second commit is tagged, the diff view opens but both tags
> > >   remain active. J/K keymaps automatically untag tagged commits.
> > 
> > The more I think about it, I prefer the second approach of keeping tags
> > active--but I'm not married to it so am happy to go with the consensus.
> > 
> > I've attached an updated diff that marks tagged commits with a '>' in
> > one of the columns padding the author field (similar to how we mark the
> > base commit when in a work tree) instead of highlighting the entire row.
> > I think it's less confusing than having multiple rows highlighted albeit
> > much more subtle.
> > 
> > While I prefer this style, I'm happy to keep the row highlight if that's
> > otherwise favoured.
> 
> I like this feature. I am wondering about one UI tweak:
> 
> Pressing 't' a second time to see the diff feels awkward because
> I would usually type Enter to see a diff.
> 
> Have you considered letting the Enter key open the diff view instead of
> the second t, such that any commit in the log view can be diffed against
> the _most recently_ tagged commit (while restricting the amount of
> commits the user can tag to 1, for now).

No, I hadn't considered using 'Enter' instead of 't' but I like it!
Let's give it a go :)

> I suspect it would be better if 't' only did the tag/untag action.
> We might eventually want to use tagging of commits for other purposes.
> Keeping the second 't' key free of the additional action of opening
> the diff view might give us more flexibility in the future.

You make a good point of having 't' do the pair of related actions and
reusing 'return' for what it already does. Thanks!

> Later, we could then let users tag as many commits as they want if that
> becomes useful for some reason, and perhaps even tag commits based on a
> search expression if that becomes useful for some reason. (One feature
> idea: write out the individual diffs of all tagged commits to a patch file.)

I didn't have any future ambitions for multiple tags but there could
be further use cases for sure. Incidentally, my next plan was to add a
keymap to write a patch file. Along the same lines as the motivation for
this feature, I often run tog log to get the hashes or indexes of the
desired commits, then run `got diff -c:base:-n -c:base-n' > patch.diff`.
With the keymap, this could be reduced to a key stroke :)


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