Mark Jamsek <mark@jamsek.com> wrote:
> Git allows tags to point to commits, trees, blobs, and even other tags.
> Currently, selecting a tag that points to anything but a commit causes a
> fatal error in tog's ref view. This change instead draws a message to
> the status line, and also enables selecting nested tags that resolve to
> a commit.
> 
> I've run out of time tonight to write a test case--I'll do this after
> zzz--but to test the nested tag to commit case:
> 
>   # make tag to base commit
>   $ got tag -m "test tag" testtag
>   # make tag to previous tag
>   $ git tag -a nestedtag -m "nested tag" testtag
> 
> then invoke `tog ref` without this diff and select refs/tags/nestedtag:
> 
>   tog: wrong type of object
> 
> With this diff, the log view will instead open starting traversal from
> the tagged commit in the first tag (i.e., testtag).
> 
> To test a tag to a blob or tree object, you need to use git to create
> the tag as this is not allowed by design in got:
> 
>   $ git tag tag2blob $blobid
> 
> Selecting the refs/tags/tag2blob entry in tog's ref view without this
> diff causes tog to exit with a fatal error. With this diff, a message is
> drawn to the status line: "commit reference required". This is also a
> good test case for why the wtimeout(3) fix is needed: without that fix,
> the message is not visible when selecting tag2blob after entering a
> compound key (e.g., 2j).

Please ignore the previous diff: in current HEAD, there's a leak in the
resolve_reflist_entry() error path, which the diff didn't fix. I'd like
to address that too.

As such, below are two diffs: the first plugs the object id leak that
happens when the object id does not reference a commit, and when the
call to got_object_open_as_tag() fails. The second is the previous diff
slightly modified, which adds the ability to select from the ref view
nested tags that resolve to commits, and makes selection of tags that
point to non-commit objects non-fatal.

Regress still to come :)

Relatedly, further potential improvements to consider are implementing
the ability to view tags in tog (similar to the `got tag -l` format),
and/or open a blame view when tags pointing to blobs are selected, and
if possible open an appropriate view when tags to trees are selected.

OTOH, non-commit tags are not something I intend to use and indeed Got
does not allow creating such tags so the below diff turning a fatal
error into a status message might be sufficient for now. But the tag
view in particular might be nice.


-----------------------------------------------
commit a6b90a072fa19205126fc503a5b282c558095a6e
from: Mark Jamsek <mark@jamsek.dev>
date: Thu Dec 26 04:02:29 2024 UTC
 
 tog: plug object id leak in resolve_reflist_entry error path
 
 M  tog/tog.c  |  7+  9-

1 file changed, 7 insertions(+), 9 deletions(-)

diff 41f38c7fc060f6b72df7bdfd48422e25a610cbb8 a6b90a072fa19205126fc503a5b282c558095a6e
commit - 41f38c7fc060f6b72df7bdfd48422e25a610cbb8
commit + a6b90a072fa19205126fc503a5b282c558095a6e
blob - 7dd3a24c5a2515f0073943bd56830d25bf86f45a
blob + f130bf1e4ff34d81ecd80a6bd9fa829ae6dae344
--- tog/tog.c
+++ tog/tog.c
@@ -10023,13 +10023,11 @@ resolve_reflist_entry(struct got_object_id **commit_id
 
 	switch (obj_type) {
 	case GOT_OBJ_TYPE_COMMIT:
-		*commit_id = obj_id;
 		break;
 	case GOT_OBJ_TYPE_TAG:
 		err = got_object_open_as_tag(&tag, repo, obj_id);
 		if (err)
 			goto done;
-		free(obj_id);
 		err = got_object_get_type(&obj_type, repo,
 		    got_object_tag_get_object_id(tag));
 		if (err)
@@ -10038,9 +10036,9 @@ resolve_reflist_entry(struct got_object_id **commit_id
 			err = got_error(GOT_ERR_OBJ_TYPE);
 			goto done;
 		}
-		*commit_id = got_object_id_dup(
-		    got_object_tag_get_object_id(tag));
-		if (*commit_id == NULL) {
+		free(obj_id);
+		obj_id = got_object_id_dup(got_object_tag_get_object_id(tag));
+		if (obj_id == NULL) {
 			err = got_error_from_errno("got_object_id_dup");
 			goto done;
 		}
@@ -10053,10 +10051,10 @@ resolve_reflist_entry(struct got_object_id **commit_id
 done:
 	if (tag)
 		got_object_tag_close(tag);
-	if (err) {
-		free(*commit_id);
-		*commit_id = NULL;
-	}
+	if (err == NULL)
+		*commit_id = obj_id;
+	else
+		free(obj_id);
 	return err;
 }
 

-----------------------------------------------
commit 5b1bf5e9cc99dbb9f4bd90154942e3d26fc46fd5 (main, main3)
from: Mark Jamsek <mark@jamsek.dev>
date: Thu Dec 26 04:29:51 2024 UTC
 
 tog: make ref view selection of non-commit tags non-fatal
 
 Tags can point to all git objects: commits, trees, blobs, and tags.
 Selecting a tag that points to any object other than a commit causes a
 fatal error. Instead, report a message to the status line. Similarly,
 nested tags may resolve to a commit, which currently errors. Instead,
 keep peeling till we reach the bottom and if it's a commit, use it for
 the requested view.
 
 M  tog/tog.c  |  32+  18-

1 file changed, 32 insertions(+), 18 deletions(-)

diff a6b90a072fa19205126fc503a5b282c558095a6e 5b1bf5e9cc99dbb9f4bd90154942e3d26fc46fd5
commit - a6b90a072fa19205126fc503a5b282c558095a6e
commit + 5b1bf5e9cc99dbb9f4bd90154942e3d26fc46fd5
blob - f130bf1e4ff34d81ecd80a6bd9fa829ae6dae344
blob + baee65693260da4a0588426b06e3213938d91200
--- tog/tog.c
+++ tog/tog.c
@@ -1320,8 +1320,16 @@ view_request_new(struct tog_view **requested, struct t
 		view_get_split(view, &y, &x);
 
 	err = view_dispatch_request(&new_view, view, request, y, x);
-	if (err)
-		return err;
+	if (err) {
+		/*
+		 * The ref view expects its selected entry to resolve to
+		 * a commit object id to open either a log or tree view.
+		 */
+		if (err->code != GOT_ERR_OBJ_TYPE)
+			return err;
+		view->action = "commit reference required";
+		return NULL;
+	}
 
 	if (view_is_parent_view(view) && view->mode == TOG_VIEW_SPLIT_HRZN &&
 	    request != TOG_VIEW_HELP) {
@@ -10025,23 +10033,29 @@ resolve_reflist_entry(struct got_object_id **commit_id
 	case GOT_OBJ_TYPE_COMMIT:
 		break;
 	case GOT_OBJ_TYPE_TAG:
-		err = got_object_open_as_tag(&tag, repo, obj_id);
-		if (err)
-			goto done;
-		err = got_object_get_type(&obj_type, repo,
-		    got_object_tag_get_object_id(tag));
-		if (err)
-			goto done;
-		if (obj_type != GOT_OBJ_TYPE_COMMIT) {
+		/*
+		 * Git allows nested tags that point to tags; keep peeling
+		 * till we reach the bottom, which is always a non-tag ref.
+		 */
+		do {
+			if (tag != NULL)
+				got_object_tag_close(tag);
+			err = got_object_open_as_tag(&tag, repo, obj_id);
+			if (err)
+				goto done;
+			free(obj_id);
+			obj_id = got_object_id_dup(
+			    got_object_tag_get_object_id(tag));
+			if (obj_id == NULL) {
+				err = got_error_from_errno("got_object_id_dup");
+				goto done;
+			}
+			err = got_object_get_type(&obj_type, repo, obj_id);
+			if (err)
+				goto done;
+		} while (obj_type == GOT_OBJ_TYPE_TAG);
+		if (obj_type != GOT_OBJ_TYPE_COMMIT)
 			err = got_error(GOT_ERR_OBJ_TYPE);
-			goto done;
-		}
-		free(obj_id);
-		obj_id = got_object_id_dup(got_object_tag_get_object_id(tag));
-		if (obj_id == NULL) {
-			err = got_error_from_errno("got_object_id_dup");
-			goto done;
-		}
 		break;
 	default:
 		err = got_error(GOT_ERR_OBJ_TYPE);


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