Fix 500 when repo has invalid .editorconfig #59
Conversation
Current coverage is 2.18% (diff: 100%)@@ master #59 diff @@
========================================
Files 31 31
Lines 7508 7508
Methods 0 0
Messages 0 0
Branches 0 0
========================================
Hits 164 164
Misses 7327 7327
Partials 17 17
|
| ec, err := ctx.Repo.GetEditorconfig() | ||
| if err != nil && !git.IsErrNotExist(err) { | ||
| ctx.Handle(500, "ErrGettingEditorconfig", err) | ||
| setEditorconfigIfExists(ctx) |
There was a problem hiding this comment.
Shouldn't this be in the router instead of in the route?
| ec, err := ctx.Repo.GetEditorconfig() | ||
| if err != nil && !git.IsErrNotExist(err) { | ||
| ctx.Handle(500, "ErrGettingEditorconfig", err) | ||
| setEditorconfigIfExists(ctx) |
| ec, err := ctx.Repo.GetEditorconfig() | ||
| if err != nil && !git.IsErrNotExist(err) { | ||
| ctx.Handle(500, "Repo.GetEditorconfig", err) | ||
| setEditorconfigIfExists(ctx) |
| ec, err := ctx.Repo.GetEditorconfig() | ||
| if err != nil && !git.IsErrNotExist(err) { | ||
| ctx.Handle(500, "ErrGettingEditorconfig", err) | ||
| setEditorconfigIfExists(ctx) |
|
Any chance you could add a test for this ? Other than that, LGTM |
|
See the original issue gogs/gogs#3643 To add a test, we should first have a mock of the git interface, or a real git repo, since this touch git. It's something for the future. Also, how and why did you add two commits to this PR? |
|
I've added two commits by clicking "Update branch" button in github interface. I belive another button will let me rebase/squash before merge. Will try right now, as this LGTM (ok for postponing test) |
|
Uhm, it takes a second LGTM, mine was already counted |
|
Needs a rebase. |
|
Rebase can be done via github button, as long as LGTM passes |
Nope, there is no button to update, so maybe there are merge conflicts. |
|
Yup, looks like being the case, @andreynering can you manually rebase and resolve conflicts (then I guess it'll need another LGTM round ?) |
|
Merging manually shows no conflicts, should I try pushing the merge via commandline ? |
|
The branch is out of sync, that's why this can only be done by admins, but we should avoid that. |
|
I remember having seen an "Update branch" button in other PRs, when they were out of sync, did any configuration change about that ? (or maybe the branch owner had the branch protected..) |
Creating a notice instead
tboerger
added
the
lgtm/done
Creating a notice instead
Fixes gogs/gogs#3643