Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue Template Uses Incorrect Path For Ref (Commit Instead Of Branch) #20456

Closed
Deadmano opened this issue Jul 22, 2022 · 9 comments · Fixed by #20461
Closed

Issue Template Uses Incorrect Path For Ref (Commit Instead Of Branch) #20456

Deadmano opened this issue Jul 22, 2022 · 9 comments · Fixed by #20461
Labels
topic/ui Change the appearance of the Gitea UI type/bug

Comments

@Deadmano
Copy link

Deadmano commented Jul 22, 2022

Description

When adding an issue from a template, the icon that links to the ref incorrectly wants to reference a commit instead of the branch, despite the branch existing with that same name. See the attached screenshot.

The template.md file contains:

---

name: "New Bug"
about: "Log a new bug!"
title: "Bug: "
ref: "main"
labels:

- bug

---

Expected path: /user/repo/src/branch/main
Actual path: /user/repo/src/commit/main

The actual path, which points to commit does not exist and thus results in a 404.

My understanding is that the link as per the screenshot will take you to the branch/tag as if you manually selected it when creating an issue and picking a branch/tag and the "ref" portion of the template would do exactly that. The documentation on issue templates was vague and doesn't mention what "ref" actually refers to.

Screenshots

Repo_Issue_5

Gitea Version

1.17.0-rc2

Can you reproduce the bug on the Gitea demo site?

No

Operating System

Windows 10

Browser Version

Firefox 102

@Deadmano Deadmano added type/bug topic/ui Change the appearance of the Gitea UI labels Jul 22, 2022
@Deadmano
Copy link
Author

@6543 & @lunny tagging you both as I believe I saw some things related to changes made in the latest RC that may be relevant? I could have sworn it was working prior to updating to v1.17.0-rc2, link pointing to the actual branch. I went back to 1.17.0+rc1-34-g4c1f4ee84 and the issue now still persists. Any feedback would be greatly appreciated!

@zeripath
Copy link
Contributor

Have you tried?

---

name: "New Bug"
about: "Log a new bug!"
title: "Bug: "
ref: "refs/heads/main"
labels:

- bug

---

@zeripath
Copy link
Contributor

If this represents a change in behaviour from 1.16 I think we should change:

gitea/modules/context/repo.go

Lines 1081 to 1088 in 599ae09

var it api.IssueTemplate
content, err := markdown.ExtractMetadata(string(data), &it)
if err != nil {
log.Debug("ExtractMetadata: %v", err)
continue
}
it.Content = content
it.FileName = entry.Name()

to prepend the refs/heads/ to the Ref if it does not match a SHA or does not already start with refs/

@Deadmano
Copy link
Author

Deadmano commented Jul 22, 2022

@zeripath that did it; you are a wonderful soul! Now what I'd love to know is why it was working prior, and updating to the latest RC seemingly broke it? I tried multiple repos, initialised both via CLI, GUI, and via the Web Interface, and it made no change to the template issue.

Is there something specific I'm meant to do when upgrading from an older install? I just shut down the instance, replaced the previous executable with the newer one, and then fired it up again.

Perhaps the documentation can be updated to explain how Ref works? I had no idea it takes a literal Git ref since the wording in the example makes it seem as if it'd point to a default main branch.

Would it then be refs/tags/{version} for tags?

@zeripath
Copy link
Contributor

Was it definitely doing this other behaviour in 1.16? If so that is likely to mean that it's an unintended change of behaviour and we should implement the changes I've suggested above

@Deadmano
Copy link
Author

Deadmano commented Jul 22, 2022

It was working fine under 1.17.0+rc1-34-g4c1f4ee84 which I installed the binary gitea-1.17-windows-4.0-amd64.exe from the 1.17 downloads page. This is the first release I ever downloaded and started with.

I tried upgrading to v1.17.0-rc2 from GitHub directly using the gitea-1.17.0-rc2-windows-4.0-amd64.exe file which is when I noticed this starting to happen with my templates.

@Deadmano

This comment was marked as off-topic.

@zeripath

This comment was marked as off-topic.

@zeripath
Copy link
Contributor

OK Issue templates was in 1.16.0 so there has been an unintended change here and we should change the code above to prepend the refs/heads/ if it doesn't start with refs/.

An alternative option is to try to detect which ref this could be referring to as I suspect that people might actually want tags etc to be detected nicely.

I'm not certain.

zeripath added a commit to zeripath/gitea that referenced this issue Jul 23, 2022
Fix go-gitea#20456

At some point during the 1.17 cycle abbreviated refs to issue branches
started breaking. This is likely due serious inconsistencies in our
management of refs throughout Gitea - which is a bug needing to be
addressed in a different PR. (Likely more than one)

We should try to use non-abbreviated refs as much as possible. That is
where a user has inputted a abbreviated ref we should add refs/heads/ if
it is branch etc. I know people keep writing and merging PRs that remove
prefixes from stored content but it is just wrong and it keeps causing
problems like this. We should only remove the prefix at the time of
presentation as the prefix is the only way of knowing umambiguously and
permanently if the ref is referring to a branch, tag or commit. We need
to make it so that every ref has the appropriate prefix, and probably
also need to come up with some definitely unambiguous way of storing
SHAs if they're used in a ref field. We must not store potentially
ambiguous refs. (Especially tagnames - there is no reason why users cannot
create a branch with the same short name as a tag and vice versa and any
attempt to prevent this will fail. You can even create a branch and a
tag that matches a SHA1 pattern.)

To that end in order to fix this bug, when parsing issue templates check
the provided Ref, if it does not start with refs/ add the BranchPrefix
to it. This allows people to make their templates refer to a tag.

Next we need to handle the issue links that are already written. The
links here are created with `git.RefURL`

Here we see there is a bug introduced in go-gitea#17551 whereby the provided Ref
can be double-escaped so we remove the incorrect external escape.
(The escape added in go-gitea#17551 is in the right place - unfortunately it
missed that the calling function was doing the wrong thing.)

Then within RefURL we check if the unprefixed ref could actually be a
SHA before defaulting that an unprefixed ref is actually a commit - if not
it is assumed to be a branch. This will handle most of the problem cases
excepting the very unusual cases where someone has deliberately written
a branch to look like a SHA1.

But please if something is called a `ref` or interpreted as a `ref` make
it a full-ref before storing or using it. By all means if something is a
`branch` assume the prefix is removed but always add it back in if you
are using it as a `ref`. Stop storing abbreviated branch names and tag
names as refs. It will keep on causing problems like this.

Fix go-gitea#20456

Signed-off-by: Andrew Thornton <[email protected]>
lunny added a commit that referenced this issue Nov 22, 2022
Fix #20456

At some point during the 1.17 cycle abbreviated refishs to issue
branches started breaking. This is likely due serious inconsistencies in
our management of refs throughout Gitea - which is a bug needing to be
addressed in a different PR. (Likely more than one)

We should try to use non-abbreviated `fullref`s as much as possible.
That is where a user has inputted a abbreviated `refish` we should add
`refs/heads/` if it is `branch` etc. I know people keep writing and
merging PRs that remove prefixes from stored content but it is just
wrong and it keeps causing problems like this. We should only remove the
prefix at the time of
presentation as the prefix is the only way of knowing umambiguously and
permanently if the `ref` is referring to a `branch`, `tag` or `commit` /
`SHA`. We need to make it so that every ref has the appropriate prefix,
and probably also need to come up with some definitely unambiguous way
of storing `SHA`s if they're used in a `ref` or `refish` field. We must
not store a potentially
ambiguous `refish` as a `ref`. (Especially when referring a `tag` -
there is no reason why users cannot create a `branch` with the same
short name as a `tag` and vice versa and any attempt to prevent this
will fail. You can even create a `branch` and a
`tag` that matches the `SHA` pattern.)

To that end in order to fix this bug, when parsing issue templates check
the provided `Ref` (here a `refish` because almost all users do not know
or understand the subtly), if it does not start with `refs/` add the
`BranchPrefix` to it. This allows people to make their templates refer
to a `tag` but not to a `SHA` directly. (I don't think that is
particularly unreasonable but if people disagree I can make the `refish`
be checked to see if it matches the `SHA` pattern.)

Next we need to handle the issue links that are already written. The
links here are created with `git.RefURL`

Here we see there is a bug introduced in #17551 whereby the provided
`ref` argument can be double-escaped so we remove the incorrect external
escape. (The escape added in #17551 is in the right place -
unfortunately I missed that the calling function was doing the wrong
thing.)

Then within `RefURL()` we check if an unprefixed `ref` (therefore
potentially a `refish`) matches the `SHA` pattern before assuming that
is actually a `commit` - otherwise is assumed to be a `branch`. This
will handle most of the problem cases excepting the very unusual cases
where someone has deliberately written a `branch` to look like a `SHA1`.

But please if something is called a `ref` or interpreted as a `ref` make
it a full-ref before storing or using it. By all means if something is a
`branch` assume the prefix is removed but always add it back in if you
are using it as a `ref`. Stop storing abbreviated `branch` names and
`tag` names - which are `refish` as a `ref`. It will keep on causing
problems like this.

Fix #20456

Signed-off-by: Andrew Thornton <[email protected]>
Co-authored-by: Lauris BH <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
fsologureng pushed a commit to fsologureng/gitea that referenced this issue Nov 22, 2022
Fix go-gitea#20456

At some point during the 1.17 cycle abbreviated refishs to issue
branches started breaking. This is likely due serious inconsistencies in
our management of refs throughout Gitea - which is a bug needing to be
addressed in a different PR. (Likely more than one)

We should try to use non-abbreviated `fullref`s as much as possible.
That is where a user has inputted a abbreviated `refish` we should add
`refs/heads/` if it is `branch` etc. I know people keep writing and
merging PRs that remove prefixes from stored content but it is just
wrong and it keeps causing problems like this. We should only remove the
prefix at the time of
presentation as the prefix is the only way of knowing umambiguously and
permanently if the `ref` is referring to a `branch`, `tag` or `commit` /
`SHA`. We need to make it so that every ref has the appropriate prefix,
and probably also need to come up with some definitely unambiguous way
of storing `SHA`s if they're used in a `ref` or `refish` field. We must
not store a potentially
ambiguous `refish` as a `ref`. (Especially when referring a `tag` -
there is no reason why users cannot create a `branch` with the same
short name as a `tag` and vice versa and any attempt to prevent this
will fail. You can even create a `branch` and a
`tag` that matches the `SHA` pattern.)

To that end in order to fix this bug, when parsing issue templates check
the provided `Ref` (here a `refish` because almost all users do not know
or understand the subtly), if it does not start with `refs/` add the
`BranchPrefix` to it. This allows people to make their templates refer
to a `tag` but not to a `SHA` directly. (I don't think that is
particularly unreasonable but if people disagree I can make the `refish`
be checked to see if it matches the `SHA` pattern.)

Next we need to handle the issue links that are already written. The
links here are created with `git.RefURL`

Here we see there is a bug introduced in go-gitea#17551 whereby the provided
`ref` argument can be double-escaped so we remove the incorrect external
escape. (The escape added in go-gitea#17551 is in the right place -
unfortunately I missed that the calling function was doing the wrong
thing.)

Then within `RefURL()` we check if an unprefixed `ref` (therefore
potentially a `refish`) matches the `SHA` pattern before assuming that
is actually a `commit` - otherwise is assumed to be a `branch`. This
will handle most of the problem cases excepting the very unusual cases
where someone has deliberately written a `branch` to look like a `SHA1`.

But please if something is called a `ref` or interpreted as a `ref` make
it a full-ref before storing or using it. By all means if something is a
`branch` assume the prefix is removed but always add it back in if you
are using it as a `ref`. Stop storing abbreviated `branch` names and
`tag` names - which are `refish` as a `ref`. It will keep on causing
problems like this.

Fix go-gitea#20456

Signed-off-by: Andrew Thornton <[email protected]>
Co-authored-by: Lauris BH <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
zeripath added a commit to zeripath/gitea that referenced this issue Jan 13, 2023
Backport go-gitea#20461

Fix go-gitea#20456

At some point during the 1.17 cycle abbreviated refishs to issue
branches started breaking. This is likely due serious inconsistencies in
our management of refs throughout Gitea - which is a bug needing to be
addressed in a different PR. (Likely more than one)

We should try to use non-abbreviated `fullref`s as much as possible.
That is where a user has inputted a abbreviated `refish` we should add
`refs/heads/` if it is `branch` etc. I know people keep writing and
merging PRs that remove prefixes from stored content but it is just
wrong and it keeps causing problems like this. We should only remove the
prefix at the time of
presentation as the prefix is the only way of knowing umambiguously and
permanently if the `ref` is referring to a `branch`, `tag` or `commit` /
`SHA`. We need to make it so that every ref has the appropriate prefix,
and probably also need to come up with some definitely unambiguous way
of storing `SHA`s if they're used in a `ref` or `refish` field. We must
not store a potentially
ambiguous `refish` as a `ref`. (Especially when referring a `tag` -
there is no reason why users cannot create a `branch` with the same
short name as a `tag` and vice versa and any attempt to prevent this
will fail. You can even create a `branch` and a
`tag` that matches the `SHA` pattern.)

To that end in order to fix this bug, when parsing issue templates check
the provided `Ref` (here a `refish` because almost all users do not know
or understand the subtly), if it does not start with `refs/` add the
`BranchPrefix` to it. This allows people to make their templates refer
to a `tag` but not to a `SHA` directly. (I don't think that is
particularly unreasonable but if people disagree I can make the `refish`
be checked to see if it matches the `SHA` pattern.)

Next we need to handle the issue links that are already written. The
links here are created with `git.RefURL`

Here we see there is a bug introduced in go-gitea#17551 whereby the provided
`ref` argument can be double-escaped so we remove the incorrect external
escape. (The escape added in go-gitea#17551 is in the right place -
unfortunately I missed that the calling function was doing the wrong
thing.)

Then within `RefURL()` we check if an unprefixed `ref` (therefore
potentially a `refish`) matches the `SHA` pattern before assuming that
is actually a `commit` - otherwise is assumed to be a `branch`. This
will handle most of the problem cases excepting the very unusual cases
where someone has deliberately written a `branch` to look like a `SHA1`.

But please if something is called a `ref` or interpreted as a `ref` make
it a full-ref before storing or using it. By all means if something is a
`branch` assume the prefix is removed but always add it back in if you
are using it as a `ref`. Stop storing abbreviated `branch` names and
`tag` names - which are `refish` as a `ref`. It will keep on causing
problems like this.

Fix go-gitea#20456

Signed-off-by: Andrew Thornton <[email protected]>
Co-authored-by: Lauris BH <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
topic/ui Change the appearance of the Gitea UI type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants