From: Mark Jamsek Subject: Re: Introduce one-line mode for got tag -l To: Lucas Gabriel Vuotto Cc: gameoftrees@openbsd.org Date: Tue, 24 Dec 2024 00:23:17 +1100 Lucas Gabriel Vuotto wrote: > As the subject reads. There are 2 commits in here: The refactor is compatible with the previous behaviour except for one discrepancy I've annotated inline along with some other minor fixes. And there's an already existing bit of undefined behaviour that we can also address with this change, which I've noted below. > ----------------------------------------------- > commit 8daa7bca200234e24a0e77d03e589d5ff53d698b > from: Lucas Gabriel Vuotto > date: Sun Dec 22 18:29:15 2024 UTC > > got tag: move tag printing into its own function > > in prepartion for a oneline output mode > > M got/got.c > > diff 589b5832ea0e12318af1af49fb76e2887629e2b6 8daa7bca200234e24a0e77d03e589d5ff53d698b > commit - 589b5832ea0e12318af1af49fb76e2887629e2b6 > commit + 8daa7bca200234e24a0e77d03e589d5ff53d698b > blob - 36655f1f322184d063373730e92b7dbf08b373a4 > blob + a24af714d90145206eec64eed5b494e9d1436a82 > --- got/got.c > +++ got/got.c > @@ -7554,6 +7554,77 @@ get_tag_refname(char **refname, const char *tag_name) > } > > static const struct got_error * > +print_tag(struct got_tag_object *tag, struct got_commit_object *commit, > + const char *refname, const char *refstr, const char *tagger, > + time_t tagger_time, const char *id_str, const char *ssh_sig, > + const char *allowed_signers, const char *revoked_signers, int verbosity, > + int *bad_sigs) > +{ > + static const struct got_error *err = NULL; > + char datebuf[26]; > + char *sig_msg = NULL, *tagmsg0 = NULL, *tagmsg, *line, *datestr; > + > + printf("%stag %s %s\n", GOT_COMMIT_SEP_STR, refname, refstr); > + printf("from: %s\n", tagger); > + datestr = get_datestr(&tagger_time, datebuf); > + if (datestr) > + printf("date: %s UTC\n", datestr); > + if (commit) > + printf("object: %s %s\n", GOT_OBJ_LABEL_COMMIT, id_str); > + else { > + switch (got_object_tag_get_object_type(tag)) { > + case GOT_OBJ_TYPE_BLOB: > + printf("object: %s %s\n", GOT_OBJ_LABEL_BLOB, id_str); > + break; > + case GOT_OBJ_TYPE_TREE: > + printf("object: %s %s\n", GOT_OBJ_LABEL_TREE, id_str); > + break; > + case GOT_OBJ_TYPE_COMMIT: > + printf("object: %s %s\n", GOT_OBJ_LABEL_COMMIT, id_str); > + break; > + case GOT_OBJ_TYPE_TAG: > + printf("object: %s %s\n", GOT_OBJ_LABEL_TAG, id_str); > + break; > + default: > + break; > + } > + } > + > + if (ssh_sig) { > + err = got_sigs_verify_tag_ssh(&sig_msg, tag, ssh_sig, > + allowed_signers, revoked_signers, verbosity); > + if (err && err->code == GOT_ERR_BAD_TAG_SIGNATURE) > + *bad_sigs = 1; > + else if (err) > + goto out; We need to explicitly clear err in the BAD_TAG_SIGNATURE case otherwise we can return the error to the caller if commit is NULL (and the tagmsg0 strdup(3) doesn't fail), which will cause a premature break out of the caller's tag list loop; this is not the same behaviour as before this refactor where we would set the bad_sigs flag and continue iterating the tag list in the BAD_TAG_SIGNATURE case: if (err != NULL) { if (err->code != GOT_ERR_BAD_TAG_SIGNATURE) goto done; *bad_sigs = 1; err = NULL; } > + printf("signature: %s", sig_msg); > + free(sig_msg); Relatedly, this problem existed before this refactor but we may as well correct it now: in the BAD_TAG_SIGNATURE case, sig_msg /may/ be NULL as got_sigs_verify_tag_ssh() does not set sig_msg in one of its BAD_TAG_SIGNATURE cases but it does in another, yet we unconditionally use sig_msg as an argument for the printf(3) %s conversion specifier, which is undefined behaviour. We can either elide the "signature:" line altogether or print something else in its place if sig_msg is NULL: printf("signature: %s", sig_msg != NULL ? sig_msg : "n/a"); > + } > + > + if (commit) { > + err = got_object_commit_get_logmsg(&tagmsg0, commit); > + if (err) > + goto out; for consistency's sake, please use the `done` label in this routine > + got_object_commit_close(commit); The caller owns commit, this routine is only borrowing it so should not be responsible for closing it; please move its closure to the caller. > + } else { > + tagmsg0 = strdup(got_object_tag_get_message(tag)); > + got_object_tag_close(tag); Likewise, the caller also owns tag so this routine should not close it; please move the close to the caller. > + if (tagmsg0 == NULL) { > + err = got_error_from_errno("strdup"); > + goto out; > + } > + } > + > + tagmsg = tagmsg0; > + while ((line = strsep(&tagmsg, "\n")) != NULL) > + printf(" %s\n", line); > + > + out: as mentioned above, please use the `done` naming convention > + free(tagmsg0); > + return err; > +} > + > +static const struct got_error * > list_tags(struct got_repository *repo, const char *tag_name, int verify_tags, > const char *allowed_signers, const char *revoked_signers, int verbosity) > { > @@ -7583,10 +7654,8 @@ list_tags(struct got_repository *repo, const char *tag > > TAILQ_FOREACH(re, &refs, entry) { > const char *refname; > - char *refstr, *tagmsg0, *tagmsg, *line, *id_str, *datestr; > - char datebuf[26]; > + char *refstr, *id_str; > const char *tagger, *ssh_sig = NULL; > - char *sig_msg = NULL; > time_t tagger_time; > struct got_object_id *id; > struct got_tag_object *tag; > @@ -7648,71 +7717,15 @@ list_tags(struct got_repository *repo, const char *tag > } > } > > - printf("%stag %s %s\n", GOT_COMMIT_SEP_STR, refname, refstr); > - free(refstr); > - printf("from: %s\n", tagger); > - datestr = get_datestr(&tagger_time, datebuf); > - if (datestr) > - printf("date: %s UTC\n", datestr); > - if (commit) > - printf("object: %s %s\n", GOT_OBJ_LABEL_COMMIT, id_str); > - else { > - switch (got_object_tag_get_object_type(tag)) { > - case GOT_OBJ_TYPE_BLOB: > - printf("object: %s %s\n", GOT_OBJ_LABEL_BLOB, > - id_str); > - break; > - case GOT_OBJ_TYPE_TREE: > - printf("object: %s %s\n", GOT_OBJ_LABEL_TREE, > - id_str); > - break; > - case GOT_OBJ_TYPE_COMMIT: > - printf("object: %s %s\n", GOT_OBJ_LABEL_COMMIT, > - id_str); > - break; > - case GOT_OBJ_TYPE_TAG: > - printf("object: %s %s\n", GOT_OBJ_LABEL_TAG, > - id_str); > - break; > - default: > - break; > - } > - } > + err = print_tag(tag, commit, refname, refstr, tagger, > + tagger_time, id_str, ssh_sig, allowed_signers, > + revoked_signers, verbosity, &bad_sigs); > + > free(id_str); > + free(refstr); > > - if (ssh_sig) { > - err = got_sigs_verify_tag_ssh(&sig_msg, tag, ssh_sig, > - allowed_signers, revoked_signers, verbosity); > - if (err && err->code == GOT_ERR_BAD_TAG_SIGNATURE) > - bad_sigs = 1; > - else if (err) > - break; > - printf("signature: %s", sig_msg); > - free(sig_msg); > - sig_msg = NULL; > - } > - > - if (commit) { > - err = got_object_commit_get_logmsg(&tagmsg0, commit); > - if (err) > - break; > - got_object_commit_close(commit); > - } else { > - tagmsg0 = strdup(got_object_tag_get_message(tag)); > - got_object_tag_close(tag); > - if (tagmsg0 == NULL) { > - err = got_error_from_errno("strdup"); > - break; > - } > - } > - > - tagmsg = tagmsg0; > - do { > - line = strsep(&tagmsg, "\n"); > - if (line) > - printf(" %s\n", line); > - } while (line); > - free(tagmsg0); > + if (err != NULL) > + break; > } > done: > got_ref_list_free(&refs); > > ----------------------------------------------- > commit 3ce9f89cf4b06724a377239d761d90a1f3aee83e (tag-short) > from: Lucas Gabriel Vuotto > date: Sun Dec 22 18:29:15 2024 UTC > > got tag: provide one-line output mode > > M got/got.1 > M got/got.c > M regress/cmdline/tag.sh > > diff 8daa7bca200234e24a0e77d03e589d5ff53d698b 3ce9f89cf4b06724a377239d761d90a1f3aee83e > commit - 8daa7bca200234e24a0e77d03e589d5ff53d698b > commit + 3ce9f89cf4b06724a377239d761d90a1f3aee83e > blob - e653af513ca5618a8e7dcf9459524bbb2f483e8d > blob + b0e1bb7bcc396fdb25eabf4910c07d7bdb08d85f > --- got/got.1 > +++ got/got.1 > @@ -1725,7 +1725,7 @@ option to be used as well. > .El > .It Xo > .Cm tag > -.Op Fl lVv > +.Op Fl lSVv > .Op Fl c Ar commit > .Op Fl m Ar message > .Op Fl r Ar repository-path > @@ -1821,6 +1821,12 @@ working directory. > If this directory is a > .Nm > work tree, use the repository path associated with this work tree. > +.It Fl S > +Display a short one-line summary of each tag, instead of the default > +history format. > +Can only be used with the > +.Fl l > +option. > .It Fl s Ar signer-id > While creating a new tag, sign this tag with the identity given in > .Ar signer-id . > blob - a24af714d90145206eec64eed5b494e9d1436a82 > blob + 421c4578e51ad094ff09fb718aecf10be5d5d7b0 > --- got/got.c > +++ got/got.c > @@ -7473,7 +7473,7 @@ done: > __dead static void > usage_tag(void) > { > - fprintf(stderr, "usage: %s tag [-lVv] [-c commit] [-m message] " > + fprintf(stderr, "usage: %s tag [-lSVv] [-c commit] [-m message] " > "[-r repository-path] [-s signer-id] name\n", getprogname()); > exit(1); > } > @@ -7554,6 +7554,46 @@ get_tag_refname(char **refname, const char *tag_name) > } > > static const struct got_error * > +print_tag_oneline(struct got_tag_object *tag, struct got_commit_object *commit, > + const char *refname, const char *refstr, time_t tagger_time) > +{ > + static const struct got_error *err = NULL; > + struct tm tm; > + char *tagmsg0 = NULL, *tagmsg, *nl; > + char datebuf[11]; > + > + if (gmtime_r(&tagger_time, &tm) == NULL) > + return got_error_from_errno("gmtime_r"); > + if (strftime(datebuf, sizeof(datebuf), "%F", &tm) == 0) > + return got_error(GOT_ERR_NO_SPACE); > + > + if (commit) { > + err = got_object_commit_get_logmsg(&tagmsg0, commit); > + if (err) > + return err; > + got_object_commit_close(commit); like the other routine, please leave commit closure for the caller/owner > + } else { > + tagmsg0 = strdup(got_object_tag_get_message(tag)); > + got_object_tag_close(tag); as above, please leave tag closure for the caller/owner > + if (tagmsg0 == NULL) > + return got_error_from_errno("strdup"); > + } > + > + tagmsg = tagmsg0; > + while (isspace((unsigned char)*tagmsg)) > + tagmsg++; > + nl = strchr(tagmsg, '\n'); > + if (nl) > + *nl = '\0'; This is really subjective and only mentioned in case you also prefer it as I know this is lifted from its log -s counterpart routine so please feel free to disregard, but I'm partial to strcspn(3) in these cases (then we can drop nl too): tagmsg[strcspn(tagmsg, "\n")] = '\0'; > + > + printf("%s %s %.7s %s\n", datebuf, refname, refstr, tagmsg); > + > + free(tagmsg0); > + > + return err; > +} > + > +static const struct got_error * > print_tag(struct got_tag_object *tag, struct got_commit_object *commit, > const char *refname, const char *refstr, const char *tagger, > time_t tagger_time, const char *id_str, const char *ssh_sig, > @@ -7626,7 +7666,8 @@ print_tag(struct got_tag_object *tag, struct got_commi > > static const struct got_error * > list_tags(struct got_repository *repo, const char *tag_name, int verify_tags, > - const char *allowed_signers, const char *revoked_signers, int verbosity) > + const char *allowed_signers, const char *revoked_signers, int verbosity, > + int oneline) > { > static const struct got_error *err = NULL; > struct got_reflist_head refs; > @@ -7717,9 +7758,13 @@ list_tags(struct got_repository *repo, const char *tag > } > } > > - err = print_tag(tag, commit, refname, refstr, tagger, > - tagger_time, id_str, ssh_sig, allowed_signers, > - revoked_signers, verbosity, &bad_sigs); > + if (oneline) > + err = print_tag_oneline(tag, commit, refname, refstr, > + tagger_time); > + else > + err = print_tag(tag, commit, refname, refstr, tagger, > + tagger_time, id_str, ssh_sig, allowed_signers, > + revoked_signers, verbosity, &bad_sigs); > > free(id_str); > free(refstr); > @@ -7908,7 +7953,7 @@ cmd_tag(int argc, char *argv[]) > char *allowed_signers = NULL, *revoked_signers = NULL, *editor = NULL; > const char *signer_id = NULL; > const char *tag_name = NULL, *commit_id_arg = NULL, *tagmsg = NULL; > - int ch, do_list = 0, verify_tags = 0, verbosity = 0; > + int ch, do_list = 0, oneline = 0, verify_tags = 0, verbosity = 0; > int *pack_fds = NULL; > > #ifndef PROFILE > @@ -7917,7 +7962,7 @@ cmd_tag(int argc, char *argv[]) > err(1, "pledge"); > #endif > > - while ((ch = getopt(argc, argv, "c:lm:r:s:Vv")) != -1) { > + while ((ch = getopt(argc, argv, "c:lm:r:Ss:Vv")) != -1) { > switch (ch) { > case 'c': > commit_id_arg = optarg; > @@ -7937,6 +7982,9 @@ cmd_tag(int argc, char *argv[]) > } > got_path_strip_trailing_slashes(repo_path); > break; > + case 'S': > + oneline = 1; > + break; > case 's': > signer_id = optarg; > break; > @@ -7958,7 +8006,9 @@ cmd_tag(int argc, char *argv[]) > argc -= optind; > argv += optind; > > - if (do_list || verify_tags) { > + if (oneline && !do_list) > + errx(1, "-S option con only be used when listing tags"); > + else if (do_list || verify_tags) { > if (commit_id_arg != NULL) > errx(1, > "-c option can only be used when creating a tag"); > @@ -8048,7 +8098,7 @@ cmd_tag(int argc, char *argv[]) > if (error) > goto done; > error = list_tags(repo, tag_name, verify_tags, allowed_signers, > - revoked_signers, verbosity); > + revoked_signers, verbosity, oneline); > } else { > error = get_gitconfig_path(&gitconfig_path); > if (error) > blob - 005a6304020f8cb434ecb6fb450d1b44d9bcc66e > blob + 8b1676b2da3337a7015d4d611342f241a374b52a > --- regress/cmdline/tag.sh > +++ regress/cmdline/tag.sh > @@ -223,6 +223,60 @@ test_tag_list() { > test_done "$testroot" "$ret" > } > > +test_tag_list_oneline() { > + local testroot=`test_init tag_list` > + local commit_id=`git_show_head $testroot/repo` > + local tag=1.0.0 > + local tag2=2.0.0 > + > + # create tag with Git > + git -C $testroot/repo tag -a -m 'test' $tag > + # create tag with Got > + (cd $testroot/repo && got tag -m 'test' $tag2 > /dev/null) > + > + tag_id=`got ref -r $testroot/repo -l \ > + | grep "^refs/tags/$tag" | tr -d ' ' | cut -d: -f2` > + local tagger_time=`git_show_tagger_time $testroot/repo $tag` > + d1=`date -u -r $tagger_time +"%F"` > + tag_id2=`got ref -r $testroot/repo -l \ > + | grep "^refs/tags/$tag2" | tr -d ' ' | cut -d: -f2` > + local tagger_time2=`git_show_tagger_time $testroot/repo $tag2` > + d2=`date -u -r $tagger_time2 +"%F"` > + > + got tag -r $testroot/repo -lS > $testroot/stdout > + > + echo "$d2 $tag2 $(trim_obj_id 7 "$tag_id2") test" > $testroot/stdout.expected > + echo "$d1 $tag $(trim_obj_id 7 "$tag_id") test" >> $testroot/stdout.expected > + cmp -s $testroot/stdout $testroot/stdout.expected > + ret=$? > + if [ $ret -ne 0 ]; then > + diff -u $testroot/stdout.expected $testroot/stdout > + test_done "$testroot" "$ret" > + return 1 > + fi > + > + got tag -r $testroot/repo -lS $tag > $testroot/stdout > + > + echo "$d1 $tag $(trim_obj_id 7 "$tag_id") test" > $testroot/stdout.expected > + cmp -s $testroot/stdout $testroot/stdout.expected > + ret=$? > + if [ $ret -ne 0 ]; then > + diff -u $testroot/stdout.expected $testroot/stdout > + test_done "$testroot" "$ret" > + return 1 > + fi > + > + got tag -r $testroot/repo -lS $tag2 > $testroot/stdout > + > + echo "$d2 $tag2 $(trim_obj_id 7 "$tag_id2") test" > $testroot/stdout.expected > + cmp -s $testroot/stdout $testroot/stdout.expected > + ret=$? > + if [ $ret -ne 0 ]; then > + diff -u $testroot/stdout.expected $testroot/stdout > + fi > + test_done "$testroot" "$ret" > +} > + > test_tag_list_lightweight() { > local testroot=`test_init tag_list_lightweight` > local commit_id=`git_show_head $testroot/repo` > @@ -573,6 +627,7 @@ test_parseargs "$@" > run_test test_tag_create > run_test test_tag_list > run_test test_tag_list_lightweight > +run_test test_tag_list_oneline > run_test test_tag_create_ssh_signed > run_test test_tag_create_ssh_signed_missing_key > run_test test_tag_commit_keywords -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68