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!

ok for me; minor nits below:

> --- got/got.c
> +++ got/got.c
> @@ -32,6 +32,7 @@
>  #include <sha2.h>
>  #include <signal.h>
>  #include <stdio.h>
> +#include <stddef.h>

stddef.h is not needed anymore.

>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>

> --- /dev/null
> +++ lib/keyword.c
> @@ -0,0 +1,263 @@
> +/*
> + * Copyright (c) 2023 Mark Jamsek <mark@jamsek.dev>
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include <sys/queue.h>
> +
> +#include <ctype.h>
> +#include <limits.h>
> +#include <stddef.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sha1.h>

it's not needed right now but I'd add an #include <sha2.h> too.  Some
of our headers require it being included, and as soon as the sha256
work is resumed got_object.h will require it too.

> +#include "got_reference.h"
> +#include "got_error.h"
> +#include "got_object.h"
> +#include "got_repository.h"
> +#include "got_cancel.h"
> +#include "got_worktree.h"
> +#include "got_commit_graph.h"
> +#include "got_keyword.h"

> +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)

> +			break;
> +		}
> +
> +		if (kwm.sym == GOT_KEYWORD_DESCENDANT) {
> +			/*
> +			 * We want the Nth generation descendant of KEYWORD,
> +			 * so queue all commits from HEAD to KEYWORD then we
> +			 * can walk from KEYWORD to its Nth gen descendent.
> +			 */
> +			err = got_object_qid_alloc(&qid, &iter_id);
> +			if (err)
> +				goto done;
> +			STAILQ_INSERT_HEAD(&commits, qid, entry);
> +
> +			if (got_object_id_cmp(&iter_id, kwid) == 0)
> +				break;
> +			continue;
> +		}
> +		++n;
> +	}
> +
> +	if (kwm.sym == GOT_KEYWORD_DESCENDANT) {
> +		n = 0;
> +
> +		STAILQ_FOREACH(qid, &commits, entry) {
> +			if (qid == STAILQ_LAST(&commits, got_object_qid, entry)
> +			    || n == kwm.n)
> +				break;
> +			++n;
> +		}
> +
> +		memcpy(&iter_id, &qid->id, sizeof(iter_id));
> +

nit: extra empty line

> +	}