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

directory support for wiki #8891

Closed
wants to merge 5 commits into from
Closed

directory support for wiki #8891

wants to merge 5 commits into from

Conversation

lucastucious
Copy link

@lucastucious lucastucious commented Nov 9, 2019

forked at PR #7390

This PR

  • add support for directories inside wiki.
  • Create/update/delete pages works well. If an old page is updated, the file will be moved to subdirectory.
  • allows to read files with nearly all letters, as it is possible to push all via wiki gitrepo
    files can exists in old and new structure.
    files are selected by link, so it is possible to access and view both
    writing to one of them will delete both, and create new one in directory structure
    delete will delete the selected file
    support images relative to directory path
    fix preview on preview rendering
    fix 500 error if wiki contains pages with invalid uri encoding (e.g. filename: badescaping%%.md)
    ui fixes
    add button 'Abort' to wiki edit and new page

Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some comments about the code (only the first couple of files).

The problem I see here (if I understand your intention correctly) is that existing wikis might become invalid after this code is implemented. And a migration process will be extremely difficult, since wikis contain links/references inside the pages and they must match the names of the documents (and page history will become useless).

Perhaps this could be implemented as optional for new repos, though, as those will contain no history.

);
}, 0);

let $toolbar = $(preview).closest('.CodeMirror-wrap').prev();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let $toolbar = $(preview).closest('.CodeMirror-wrap').prev();
const $toolbar = $(preview).closest('.CodeMirror-wrap').prev();

Suggestion by as per linter error.

func WikiNameToSubURL(name string) string {
return url.QueryEscape(strings.Replace(name, " ", "-", -1))
// remove path up
re1 := regexp.MustCompile(`(\.\.\/)`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be simpler:

name = strings.ReplaceAll(name, "/../", "/")

(And TrimSuffix/TrimPrefix for /.. and ../)

But strings starting with ../, ending with /.. or containing /../ should be rejected as invalid instead of sanitized, IMHO.

name = strings.Trim(name, "\n\r\t /")
name = url.QueryEscape(name)
//restore spaces
re3 := regexp.MustCompile(`(?m)(%20|\+)`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regexps that are not dynamic should be stored outside of the function to avoid compiling them multiple times. That also makes it easier to make a unit test for them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, return strings.ReplaceAll(name, "+", "%20") should do the trick as well.

}
for _, kv := range restore {
loopRe := regexp.MustCompile(kv[0])
name = loopRe.ReplaceAllString(name, kv[1])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other comments above.

}

// FilenameToPathFilename converts a wiki filename to filename with filepath.
func FilenameToPathFilename(name string) string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import "net/url"

And you have net.PathEscape(), net.PathUnescape().

@@ -136,6 +198,9 @@ func (repo *Repository) updateWikiPage(doer *User, oldWikiName, newWikiName, con
}

gitRepo, err := git.OpenRepository(basePath)

fmt.Println(reflect.TypeOf(gitRepo))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be here

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 10, 2019
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Nov 11, 2019
@Cherrg
Copy link
Contributor

Cherrg commented Nov 16, 2019

The problem I see here (if I understand your intention correctly) is that existing wikis might become invalid after this code is implemented. And a migration process will be extremely difficult, since wikis contain links/references inside the pages and they must match the names of the documents (and page history will become useless).

Yeah my intention was to support both, old links and new ones (if i remember correctly) -> so old links and paths should still be working. (I hope i don't missed too much cases)
Previously there was a bug with relative paths too. May some one with an existing wiki could test this PR.

I will fix my own branch in the next few days so it will compile again. (if @LucasDLK isn't faster)

@stale
Copy link

stale bot commented Jan 15, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Jan 15, 2020
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Jan 15, 2020
@stale stale bot removed the issue/stale label Jan 15, 2020
@techknowlogick techknowlogick deleted the branch go-gitea:master May 4, 2021 16:17
@go-gitea go-gitea locked and limited conversation to collaborators Jun 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants