From: Mark Jamsek <mark@jamsek.com>
Subject: Re: got {diff,log,update} -c KEYWORD (cf. svn revision keywords)
To: Omar Polo <op@omarpolo.com>
Cc: Stefan Sperling <stsp@stsp.name>, gameoftrees@openbsd.org
Date: Mon, 17 Jul 2023 18:05:18 +1000

Omar Polo <op@omarpolo.com> wrote:
> On 2023/07/14 23:44:08 +1000, Mark Jamsek <mark@jamsek.com> wrote:
> > I've made a few changes that I think make it a lot nicer, and have
> > added some non-trivial and somewhat contrived tests too.
> > 
> > I nixed the hand-rolled overflow code so we use strtonum(3), and made
> > the keyword parsing strict. That lax keyword comparison was by design
> > initially but I forgot to remove it; I was using -cBASED in testing :)
> > 
> > Something I just noticed that I like about the ':' delimiters is ^W
> > removes back to the ':', which is handy. And the +/-N works great with
> > tog's count modifier. You can open tog, see which commit it is you
> > want with Nj, and use the same N count (or the headline index-1) in the
> > 'got * -c:head:-N' command! This is going to save a lot of copypasting
> > and misremembering commit ids. It's pretty neat :)
> 
> sorry for the delay.  I've been using this today quite a few times.
> Honestly I woludn't have imaged that things like got di -cmain -c main:-3
> were *so* useful in practice :D  Thanks!

I think I'm going to use it a lot!

> ok for me; minor nits below:

Thansk, op. Just one comment below where I'd really appreciate your
thoughts.

> > --- got/got.c
> > +++ got/got.c
> > @@ -32,6 +32,7 @@
> ...
> 
> > +const struct got_error *
> > +got_keyword_to_idstr(char **ret, const char *keyword,
> > +    struct got_repository *repo, struct got_worktree *wt)
> > +{
> > [...]
> > +	while (n <= kwm.n) {
> > +		err = got_commit_graph_iter_next(&iter_id, graph, repo,
> > +		    NULL, NULL);
> > +		if (err) {
> > +			if (err->code == GOT_ERR_ITER_COMPLETED)
> > +				err = NULL;
> 
> Here (in a follow-up maybe) I'd set error to something non-NULL.  I
> think it'd be better to return an error if we couldn't go as far back
> as required.  (imagine the case with `main' being a reference to a
> commit without parent and the user doing got log -c main:-1)

At the moment, this will just show the log for main. The reason I
decided to return the last reachable (i.e., most recent or oldest)
commit if the user specifies a modifier that is too large, is for
convenience and to avoid a hard error.

For example, if you want the root commit, rather than count, just give
a large N. But do you think a ":root" keyword will be a better approach?
It could represent the root commit of the current branch if in a work
tree, or the root commit of the repository's HEAD. Or we could even make
it a modifier like "main:root" or "foo:root".

I'm not against returning a hard error here, just looking for more
ideas and considering the pros and cons of them.


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