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

Better end user visibility for failures in agit PRs #32965

Open
TheFox0x7 opened this issue Dec 23, 2024 · 12 comments · May be fixed by #33012
Open

Better end user visibility for failures in agit PRs #32965

TheFox0x7 opened this issue Dec 23, 2024 · 12 comments · May be fixed by #33012
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@TheFox0x7
Copy link
Contributor

Feature Description

The error messages in agit PRs aren't very visible to the end user. I think that adding an error label for fails would make them pop out more (if one has color enabled). See screenshots for full options.

I encountered this while trying to submit an agit PR to docs, which uncovered a missing UserMsg, which would've been helpful:

log.Error(err.Error())
ctx.JSON(http.StatusInternalServerError, private.Response{
Err: err.Error(),
})

and I've glanced over it before, so I think having a color before it would be helpful.

Similarly an instruction for setting up branch tracking would be a good improvement - no idea how to set it up though.

Screenshots

image
Possible options:
image

@TheFox0x7 TheFox0x7 added the type/proposal The new feature has not been accepted yet but needs to be discussed first. label Dec 23, 2024
@lunny
Copy link
Member

lunny commented Dec 23, 2024

I resolve the problems via resync of all git hooks because there are no proc-receive hooks under this repository. I think there is no migration for git hooks when introducing the agit feature.

@TheFox0x7
Copy link
Contributor Author

I can reproduce this (or similar) fault locally - lack of collaborator status prevents user from submitting agit PR, which is not forwarded to them:

remote: Gitea: Internal Server Error (no message for end users)
remote: error: proc-receive exited abnormally

I in the original text I meant that the error should be made more visible (and descriptive), which I figured is more of a enhancement that an a bug. Some errors - like the lack of collaborator status - are worth forwarding to user in my opinion.
Apologies if I worded it in a confusing way originally.

@wxiaoguang
Copy link
Contributor

If I understand correctly , only the "Gitea:" prefixed line is outputed by Gitea.

Other lines are from git, which are not controllable

@hiifong
Copy link
Member

hiifong commented Dec 26, 2024

If I understand correctly , only the "Gitea:" prefixed line is outputed by Gitea.

Other lines are from git, which are not controllable

In fact, there is a lot of information without the Gitea prefix, which is also returned to the client by Gitea.

example:
image
image

@TheFox0x7
Copy link
Contributor Author

That's on success though, which could also make use of the highlighted strings like:
hint: Create a new pull request for or same with visit.

My point here was that Gitea: Internal Server Error (no message for end users) is for one - missing an error which is informative to the end user, secondly not very obvious that is has an error - prefixing it with error too would I think help in this.

I can submit a PR for this once we agree on the scope - where to put highlighted strings and which errors should be passed down to user (or which should not, whichever is easier).

@hiifong
Copy link
Member

hiifong commented Dec 26, 2024

However, we cannot modify the git buildin information, such as the error: proc-receive exited abnormally shown in your screenshot.
https://github.com/git/git/blob/996f0c583b36aa5d6c6308285aea1421eb7efae7/builtin/receive-pack.c#L998

@wxiaoguang
Copy link
Contributor

My point here was that Gitea: Internal Server Error (no message for end users) is for one - missing an error which is informative to the end user, secondly not very obvious that is has an error - prefixing it with error too would I think help in this.

Agree to improve these error message. Actually it was even worse long time ago 🤣 .... I have managed to do some improvements (maybe there might be something interesting, FYI: Refactor internal API for git commands, use meaningful messages instead of "Internal Server Error" (#23687)). And yes, I still didn't make it good enough at that moment, so there are still a lot of things to do.

@hiifong
Copy link
Member

hiifong commented Dec 26, 2024

As far as the current code is concerned, only the error level information is easier to implement, most of these are calls to the fail method,But there is no fixed way to return data for other levels of information.

image

@hiifong

This comment was marked as off-topic.

@wxiaoguang

This comment was marked as off-topic.

@hiifong
Copy link
Member

hiifong commented Dec 26, 2024

Sorry, forgive my poor English

@TheFox0x7 TheFox0x7 linked a pull request Dec 27, 2024 that will close this issue
@TheFox0x7
Copy link
Contributor Author

I've looked through (probably) most errors and I think that the ErrMustCollaborator and ErrBlockedUser are only ones which would benefit from being visible to the end user same as with routers/web/repo.

Which also led me to discovering: #33013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants