[PATCH] Handle tags outside of refs/tags gracefully.

[PATCH] Handle tags outside of refs/tags gracefully.

[Ulrich Spörlein]

Big conversions from Subversion to Git might keep track of several
"internal" tags under namespaces different from refs/tags. If a full
refs specification was given, don't prepend refs/tags again.

This fixes a "Bad tag reference" for the FreeBSD project URLs like so:
https://cgit.freebsd.org/src/tag/?h=refs/backups/projects/mips@202126

(fun fact: the log handler properly handles
log/?h=backups/projects/mips@202126 though it is an annotated tag under
refs/backups.

This fixes issue https://github.com/freebsd/git_conv/issues/24
---
 ui-tag.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ui-tag.c b/ui-tag.c
index 424bbcc..cc86b31 100644
--- a/ui-tag.c
+++ b/ui-tag.c
@@ -47,7 +47,11 @@ void cgit_print_tag(char *revname)
 	if (!revname)
 		revname = ctx.qry.head;
 
-	strbuf_addf(&fullref, "refs/tags/%s", revname);
+	if (!strncmp(revname, "refs/", 5)) {
+		strbuf_addstr(&fullref, revname);
+	} else {
+		strbuf_addf(&fullref, "refs/tags/%s", revname);
+	}
 	if (get_oid(fullref.buf, &oid)) {
 		cgit_print_error_page(404, "Not found",
 			"Bad tag reference: %s", revname);
-- 
2.24.0

Re: [PATCH] Handle tags outside of refs/tags gracefully.

[Jason A. Donenfeld]

This is for the tag UI, though. Aren't tags supposed to live in
refs/tags/ by definition?

Special casing "refs/" also winds up breaking repositories that
accidentally push tags literally named 'refs/tags/something'. I've
seen this happen quite a few times in the real world.

Re: [PATCH] Handle tags outside of refs/tags gracefully.

[Ulrich Spörlein]

On Tue, 2020-12-29 at 12:37:43 +0100, Jason A. Donenfeld wrote:
>This is for the tag UI, though. Aren't tags supposed to live in
>refs/tags/ by definition?
>
>Special casing "refs/" also winds up breaking repositories that
>accidentally push tags literally named 'refs/tags/something'. I've
>seen this happen quite a few times in the real world.

Nothing is really enforced by git, so you can put tags anywhere (and 
suffer the consequences). We decided to have these tags not be pulled by 
default and stashed them under refs/backups/foo so they are not directly 
user visible after a default clone.

If I ask for the tag refs/backups/foo, I expect it to look for that, and 
not for refs/tags/refs/backups/foo. That is what I feel most of the git 
commands do, that need to look up a ref. It was just striking how the 
log handler does handle this just fine (as does the git CLI).

It's unfortunate that there are repos out there with 
refs/tags/refs/tags/bar (what the ... ?). So I guess there's no easy fix 
here then.

Cheers
Uli

Re: [PATCH] Handle tags outside of refs/tags gracefully.

[Jason A. Donenfeld]

On Tue, Dec 29, 2020 at 7:22 PM Ulrich Spörlein <uqs@freebsd.org> wrote:
>
> On Tue, 2020-12-29 at 12:37:43 +0100, Jason A. Donenfeld wrote:
> >This is for the tag UI, though. Aren't tags supposed to live in
> >refs/tags/ by definition?
> >
> >Special casing "refs/" also winds up breaking repositories that
> >accidentally push tags literally named 'refs/tags/something'. I've
> >seen this happen quite a few times in the real world.
>
> Nothing is really enforced by git, so you can put tags anywhere (and
> suffer the consequences). We decided to have these tags not be pulled by
> default and stashed them under refs/backups/foo so they are not directly
> user visible after a default clone.
>
> If I ask for the tag refs/backups/foo, I expect it to look for that, and
> not for refs/tags/refs/backups/foo. That is what I feel most of the git
> commands do, that need to look up a ref. It was just striking how the
> log handler does handle this just fine (as does the git CLI).

The fact that the git CLI handles it properly actually is a compelling
reason to mimic its logic. How does it handle this scenario? I assume
it looks up both possibilities and returns the first one that matches?
In which order does it check?

Jason

Re: [PATCH] Handle tags outside of refs/tags gracefully.

[Gianni Ceccarelli]

On Tue, 2020-12-29 at 12:37:43 +0100, Jason A. Donenfeld wrote:  
> This is for the tag UI, though. Aren't tags supposed to live in
> refs/tags/ by definition?

Yes. The "git tag" manpage says:

>> Add a tag reference in refs/tags/, unless -d/-l/-v is given to
>> delete, list or verify tags

On 2020-12-29 "Jason A. Donenfeld" <Jason@zx2c4.com> wrote:
> The fact that the git CLI handles it properly actually is a compelling
> reason to mimic its logic. How does it handle this scenario? I assume
> it looks up both possibilities and returns the first one that matches?
> In which order does it check?

Depends which sub-command we're talking about::

  $ mkdir /tmp/a
  $ cd /tmp/a
  $ git init
  $ touch foo && git add foo && git commit -m first
  $ mkdir .git/refs/weird && git rev-parse HEAD > .git/refs/weird/thing
  $ git log --decorate
  commit 4081ad6720f3f60f5f11a77f1d932517496e33ba (HEAD -> master, refs/weird/thing)
  Author: dakkar <dakkar@thenautilus.net>
  Date:   2020-12-29 20:10:18 +0000

    first

Now, ``git checkout weird/thing`` works, as does ``git log
weird/thing``, or anything that uses ``rev-parse`` internally.

``git tag`` will *not* show ``weird/thing`` because it's not a tag,
it's a non-tag ref::

  $ git tag -d weird/thing
  error: tag 'weird/thing' not found.

As far as CGit is concerned, these work:

* https://www.thenautilus.net/cgit/example/log/?h=weird/thing
* https://www.thenautilus.net/cgit/example/commit/?h=weird/thing
* https://www.thenautilus.net/cgit/example/tree/?h=weird/thing
* https://www.thenautilus.net/cgit/example/diff/?h=weird/thing

-- 
	Dakkar - <Mobilis in mobile>
	GPG public key fingerprint = A071 E618 DD2C 5901 9574
	                             6FE2 40EA 9883 7519 3F88
	                    key id = 0x75193F88

Re: [PATCH] Handle tags outside of refs/tags gracefully.

[Gianni Ceccarelli]

I have found an annoying case…

In the repository created as per my previous message, I did::

  $ git tag -a foo
  $ git rev-parse refs/tags/foo > .git/refs/weird/annotated
  $ git push origin refs/weird/*:refs/weird/*

This creates an "annotated tag", which is an object in the store, not
just a reference.

Now CGit shows the ``refs/weird/annotated`` as a tag in
https://www.thenautilus.net/cgit/example/ and several other views, but
following that link goes to
https://www.thenautilus.net/cgit/example/tag/?h=refs/weird/annotated
which says "bad tag reference"

I hope you'll concur this is not the best behaviour.

I see several ways to "fix" this:

* in all the various views, don't show links to annotated tags whose
  ref is outside of ``refs/tags/``
* in ``/tag``, try ``refs/tags/$h`` first, and if that doesn't work,
  try ``$h``, and show that only if it's an annotated tag (not just a
  reference)
* create another endpoint (``/atag``?) to show tag objects, and link
  to that one for annotated tags whose ref is outside of
  ``refs/tags/``

-- 
	Dakkar - <Mobilis in mobile>
	GPG public key fingerprint = A071 E618 DD2C 5901 9574
	                             6FE2 40EA 9883 7519 3F88
	                    key id = 0x75193F88

Re: [PATCH] Handle tags outside of refs/tags gracefully.

[John Keeping]

On Tue, Jan 05, 2021 at 11:53:03AM +0000, Gianni Ceccarelli wrote:
> I have found an annoying case…
> 
> In the repository created as per my previous message, I did::
> 
>   $ git tag -a foo
>   $ git rev-parse refs/tags/foo > .git/refs/weird/annotated
>   $ git push origin refs/weird/*:refs/weird/*
> 
> This creates an "annotated tag", which is an object in the store, not
> just a reference.
> 
> Now CGit shows the ``refs/weird/annotated`` as a tag in
> https://www.thenautilus.net/cgit/example/ and several other views, but
> following that link goes to
> https://www.thenautilus.net/cgit/example/tag/?h=refs/weird/annotated
> which says "bad tag reference"
> 
> I hope you'll concur this is not the best behaviour.
> 
> I see several ways to "fix" this:
> 
> * in all the various views, don't show links to annotated tags whose
>   ref is outside of ``refs/tags/``
> * in ``/tag``, try ``refs/tags/$h`` first, and if that doesn't work,
>   try ``$h``, and show that only if it's an annotated tag (not just a
>   reference)
> * create another endpoint (``/atag``?) to show tag objects, and link
>   to that one for annotated tags whose ref is outside of
>   ``refs/tags/``

I think something like option 2 is the right answer here, since that
brings us closer to Git's behaviour.  Does the patch below help?

-- >8 --
Subject: [PATCH] ui-tag: accept tags not under refs/tags/

In log views, we decorate commits with tags under any ref hierarchy,
which generates links to ui-tag with, for example, refs/weird/tag.  By
forcing a refs/tags/ prefix onto this, a 404 not found error is
guaranteed.

Taking inspiration from Git's ref_rev_parse_rules, let's try to resolve
the given tag name raw and relative to refs/ before trying refs/tags/.

Reported-by: Gianni Ceccarelli <dakkar@thenautilus.net>
Signed-off-by: John Keeping <john@metanate.com>
---
 ui-tag.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/ui-tag.c b/ui-tag.c
index 846d5b1..c1f81d1 100644
--- a/ui-tag.c
+++ b/ui-tag.c
@@ -38,17 +38,32 @@ static void print_download_links(char *revname)
 	html("</td></tr>");
 }
 
+static const char *tag_patterns[] = {
+	"%s",
+	"refs/%s",
+	"refs/tags/%s",
+	NULL
+};
+
 void cgit_print_tag(char *revname)
 {
 	struct strbuf fullref = STRBUF_INIT;
 	struct object_id oid;
 	struct object *obj;
+	const char **pattern;
 
 	if (!revname)
 		revname = ctx.qry.head;
 
-	strbuf_addf(&fullref, "refs/tags/%s", revname);
-	if (get_oid(fullref.buf, &oid)) {
+	for (pattern = tag_patterns; *pattern; pattern++) {
+		strbuf_reset(&fullref);
+		strbuf_addf(&fullref, *pattern, revname);
+
+		if (!get_oid(fullref.buf, &oid))
+			break;
+	}
+
+	if (!*pattern) {
 		cgit_print_error_page(404, "Not found",
 			"Bad tag reference: %s", revname);
 		goto cleanup;
-- 
2.30.0

Re: [PATCH] Handle tags outside of refs/tags gracefully.

[Gianni Ceccarelli]

On 2021-01-05 John Keeping <john@keeping.me.uk> wrote:
> I think something like option 2 is the right answer here, since that
> brings us closer to Git's behaviour.

``git tag`` never looks anywhere but under ``refs/tags/``. Not even
``git tag --contains`` will show annotated tags that are only
referenced outside that path.

OTOH (on my example repo)::

  $ git show HEAD
  commit 4081ad6720f3f60f5f11a77f1d932517496e33ba (HEAD -> master, refs/weird/thing, tag: refs/weird/annotated, tag: foo, origin/master, origin/HEAD)
  Author: dakkar <dakkar@thenautilus.net>
  Date:   2020-12-29 20:10:18 +0000

      first

  $ git tag -v refs/weird/annotated
  error: tag 'refs/weird/annotated' not found.

So maybe CGit is already behaving very closely to git.
  
> Does the patch below help?
>
> [snip]
>  
> +static const char *tag_patterns[] = {
> +	"%s",
> +	"refs/%s",
> +	"refs/tags/%s",
> +	NULL
> +};

I feel like those strings should be in the reverse order: we've been
asked to show a tag, let's try to resolve the ref as an actual tag
before guessing.

-- 
	Dakkar - <Mobilis in mobile>
	GPG public key fingerprint = A071 E618 DD2C 5901 9574
	                             6FE2 40EA 9883 7519 3F88
	                    key id = 0x75193F88

Re: [PATCH] Handle tags outside of refs/tags gracefully.

[Gianni Ceccarelli]

On 2021-01-05 Ulrich Spörlein <uqs@freebsd.org> wrote:
> Yes, but `git log` will show all of them, it doesn't care :) (and so
> does cgits log handler ... but then you get 404s when clicking them).

Thank you for confirming. That's exactly what I wrote two messages
ago:

>> Now CGit shows the ``refs/weird/annotated`` as a tag in
>> https://www.thenautilus.net/cgit/example/ and several other views,
>> but
>> following that link goes to
>> https://www.thenautilus.net/cgit/example/tag/?h=refs/weird/annotated
>> which says "bad tag reference"

-- 
	Dakkar - <Mobilis in mobile>
	GPG public key fingerprint = A071 E618 DD2C 5901 9574
	                             6FE2 40EA 9883 7519 3F88
	                    key id = 0x75193F88