Lucas Gabriel Vuotto <lucas@sexy.is> wrote:
> Hi Mark,
> 
> thanks for the review! Indeed, the part I doubted the most moving out
> print_tag was the bad_sigs bit. I believe your suggestion still isn't
> 100% compatible with the previous version: originally, err could be
> set to something on bad_sigs case (last iter + NULL commit). I decided
> to instead check for GOT_ERR_BAD_TAG_SIGNATURE in order to break.

np!

It is compatible with the previous behaviour because we carry the
BAD_TAG_SIGNATURE error case in the bad_sigs out parameter. As such,
even on the last iteration and a NULL commit case, if err is NULL but
bad_sigs is set, list_tags() sets err to GOT_ERR_BAD_TAG_SIGNATURE.
And bad_sigs is persistent--once set it's not cleared. Now we are
duplicating the check: we check for BAD_TAG_SIGNATURE in print_tag() and
set bad_sigs to carry this error state but still return the error to the
caller to check it again at the end of each iteration and ignore it,
then after the ref loop at the end of list_tags() we check bad_sigs for
the error state and set the error accordingly.

I think it is cleaner and more consistent to clear err in print_tag()
when we set bad_sigs and don't add the extra BAD_TAG_SIGNATURE check at
the end of each loop iteration in list_tags().

But I'm happy as long as the behaviour is preserved so it's up to you :)

> I adopted all your suggestion. The strcspn one is particulary nice.
> 
> As for dealing with a potentially NULL sig_msg, I decided to print "bad
> signature" in that case.

Nice, that's much clearer. Thanks!

> I also switched to -S signer and -s for short. I adopted the object type
> label, but went with 12-characters hash prefix and decided against
> adding refs/tags/. If there is a function to get a short and unambiguous
> hash prefix, I can use that instead.

Good call on all fronts! I agree with eliding the full tag reference.
As mentioned on IRC, I think this information is implicit in the
`got tag -ls` command so provided stsp is ok with it, I am too.

I would be happy with a slightly shorter prefix--I think 10 is currently
long enough for unique prefixes in src.git--but 12 has a higher chance
of being unique in super large repositories. And by omitting the full
tag reference name, we have the screen estate to spare a few extra
characters for a more likely unique prefix.

Thanks for addressing everything so quick! I haven't inspected the diff
yet but will later tonight; I'd like to make sure stsp is ok with the
short tag name too.


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