-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Replace repo.namedBlob
by git.TreeEntry
.
#22898
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just asked myself if this cache type was implemented for performance reasons, but according to the method implementations, that is negligible…
Was that different previously?
Otherwise, I don't know how this could happen.
And even if it was, it was only optimizing for README files. How heavy is that really?
Thanks for the vote of confidence! |
IIRC we needed the namedBlob at some point because we were passing round git.Blobs but we're able to get the TreeEntry just as easily now so it might be needed anymore |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as off-topic.
This comment was marked as off-topic.
`namedBlob` turned out to be a poor imitation of a `TreeEntry`. Using the latter directly shorten this code.
6c2779b
to
947604d
Compare
The current CI failures are
and
These don't seem to be related to this patch. I'll restart and if it happens again I guess I'll try to reproduce locally. EDIT: passing now; all I did was merge |
size: 7028 | ||
size: 7320 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This had to change due to the test additions I did. I used the same test repo (user2/repo1.git
) that #23152 did, but I don't know why that PR didn't force changing this number.
Metas: ctx.Repo.Repository.ComposeDocumentMetas(), | ||
GitRepo: ctx.Repo.GitRepo, | ||
}, rd) | ||
if err != nil { | ||
log.Error("Render failed for %s in %-v: %v Falling back to rendering source", readmeFile.name, ctx.Repo.Repository, err) | ||
log.Error("Render failed for %s in %-v: %v Falling back to rendering source", readmeFile.Name(), ctx.Repo.Repository, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps
log.Error("Render failed for %s in %-v: %v Falling back to rendering source", readmeFile.Name(), ctx.Repo.Repository, err) | |
log.Error("Render failed for %s in %-v: %v Falling back to rendering source", path.Join(subfolder, readmeFile.Name()), ctx.Repo.Repository, err) |
?
Please update branch |
@kousu |
Of course! Here it comes around again. I really appreciate getting this in. I've had my eye on this patch for a couple of months now. Did I do something wrong btw? Did I miss clicking "Allow Edits from Maintainers" or something? |
Nope, not your fault, I believe it's a GitHub thing that doesn't let us update it because it's from an org vs a user account. |
* giteaofficial/main: [skip ci] Updated translations via Crowdin Replace `repo.namedBlob` by `git.TreeEntry`. (go-gitea#22898) Fix theme-auto loading (go-gitea#23504) Update path to docs theme file (go-gitea#23502) Use arm image for arm runner (go-gitea#23503)
namedBlob
turned out to be a poor imitation of aTreeEntry
. Using the latter directly shortens this code.This partially undoes #23152, which I found a merge conflict with, and also expands the test it added to cover the subtle README-in-a-subfolder case.