lunny/gitea
Archived
1
1
Fork 1
Code Issues 1.7k Pull Requests 154 Projects Releases 128 Wiki Activity

fix: not able to update local created non-urlencoded wiki pages #16139

Merged
lunny merged 9 commits from fix-local-wiki-update into main 2021-07-07 23:23:09 +00:00
Conversation 0 Commits 9 Files Changed 1 +43 -13
blumia commented 2021-06-12 08:58:59 +00:00
Contributor
Copy Link

Related to issue #16122, this patch intended to resolve part of the bug mentioned in https://github.com/go-gitea/gitea/issues/16122#issuecomment-859570030

If we clone the wiki repo locally and create some markdown file without urlencoded its filename, the file can be displayed correctly if we then push back it to the remote repo, but if we attempt to update these pages, it will create a new urlencoded file with updated content and the old non-urlencoded file is still here with content unchanged. This PR intended to resolve that by doing:

  1. When trying to create/update a wiki page, check if there is a non-urlencoded page that already exists.
  2. If such a page not exists, use urlencoded file name instead (keep the original behavior).

By the way, since resolving #16122 completely still need some other patches, there is something we still need to discuss:

  1. To resolve that issue, do we need to switch to use non-urlencoded files by default?
  2. We probably need to keep compatible with the old behavior, right?

If necessary, we can discuss these in #16122.

Related to issue #16122, this patch intended to resolve part of the bug mentioned in https://github.com/go-gitea/gitea/issues/16122#issuecomment-859570030 If we clone the wiki repo locally and create some markdown file without urlencoded its filename, the file can be displayed correctly if we then push back it to the remote repo, but if we attempt to update these pages, it will create a new urlencoded file with updated content and the old non-urlencoded file is still here with content unchanged. This PR intended to resolve that by doing: 1. When trying to create/update a wiki page, check if there is a non-urlencoded page that already exists. 2. If such a page not exists, use urlencoded file name instead (keep the original behavior). ---------------- By the way, since resolving #16122 completely still need some other patches, there is something we still need to discuss: 1. To resolve that issue, do we need to switch to use non-urlencoded files by default? 2. We probably need to keep compatible with the old behavior, right? If necessary, we can discuss these in #16122.
👍 2 ❤️ 1
zeripath reviewed 2021-06-21 18:07:55 +00:00
services/wiki/wiki.go
@ -83,1 +83,4 @@
// prepareWikiFileName try to find a suitable file path with file name by the given raw wiki name.
// return: existence, prepared file path with name, error
func prepareWikiFileName(gitRepo *git.Repository, wikiName string) (bool, string, error) {
zeripath commented 2021-06-21 18:07:55 +00:00
Contributor

repeatedly running this is very expensive. gitRepo.LsFiles(...) is not free.

Why don't we do somethings else:

  • Pass in the raw unchanged name
  • Add ".md" suffix if not there.
  • Look for this raw suffixed name within the stringslice - that works return true, rawSuffixed, nil
  • If not normalize and look for that - if that works returns true, normalized path, nil
  • If not return false, raw suffixed, nil
repeatedly running this is very expensive. gitRepo.LsFiles(...) is not free. Why don't we do somethings else: * Pass in the raw unchanged name * Add ".md" suffix if not there. * Look for this raw suffixed name within the stringslice - that works return true, rawSuffixed, nil * If not normalize and look for that - if that works returns true, normalized path, nil * If not return false, raw suffixed, nil
blumia reviewed 2021-06-23 05:39:16 +00:00
services/wiki/wiki.go
@ -83,1 +83,4 @@
// prepareWikiFileName try to find a suitable file path with file name by the given raw wiki name.
// return: existence, prepared file path with name, error
func prepareWikiFileName(gitRepo *git.Repository, wikiName string) (bool, string, error) {
blumia commented 2021-06-23 05:39:16 +00:00
Author
Contributor

Hi and I updated my patch, added a prepareWikiFileName() function that did what you described, but I'm not sure if I understood you correctly.

Yeah gitRepo.LsFiles(...) is not free, but adding that function seems could only make the code looks cleaner, it still requires two LsFiles() calls to look for the file inside that function, or is there another method that better suitable for this use case?

Hi and I updated my patch, added a `prepareWikiFileName()` function that did what you described, but I'm not sure if I understood you correctly. Yeah `gitRepo.LsFiles(...)` is not free, but adding that function seems could only make the code looks cleaner, it still requires two `LsFiles()` calls to look for the file inside that function, or is there another method that better suitable for this use case?
zeripath reviewed 2021-06-25 18:10:45 +00:00
services/wiki/wiki.go
@ -83,1 +83,4 @@
// prepareWikiFileName try to find a suitable file path with file name by the given raw wiki name.
// return: existence, prepared file path with name, error
func prepareWikiFileName(gitRepo *git.Repository, wikiName string) (bool, string, error) {
zeripath commented 2021-06-25 18:10:45 +00:00
Contributor

No, this wasn't what I was meaning.

You look for two filenames: The one passed in and the normalized path.

func checkFileExistence(gitRepo *git.Repository, filePath, alternate string) (bool, string, error) {
filesInIndex, err := gitRepo.LsFiles(filePath)
	if err != nil {
		log.Error("%v", err)
		return false, err
	}
	if util.IsStringInSlice(filePath, filesInIndex) {
		return true, filePath, nil
	}
	if util.IsStringInSlice(alternate, filesInIndex) {
		return true, alternate, nil
	}
  return false, "", nil
}

Although you should be a bit more efficient.

No, this wasn't what I was meaning. You look for two filenames: The one passed in and the normalized path. ```go func checkFileExistence(gitRepo *git.Repository, filePath, alternate string) (bool, string, error) { filesInIndex, err := gitRepo.LsFiles(filePath) if err != nil { log.Error("%v", err) return false, err } if util.IsStringInSlice(filePath, filesInIndex) { return true, filePath, nil } if util.IsStringInSlice(alternate, filesInIndex) { return true, alternate, nil } return false, "", nil } ``` Although you should be a bit more efficient.
zeripath approved these changes 2021-06-26 00:23:56 +00:00
blumia reviewed 2021-06-26 04:33:44 +00:00
services/wiki/wiki.go
@ -83,1 +83,4 @@
// prepareWikiFileName try to find a suitable file path with file name by the given raw wiki name.
// return: existence, prepared file path with name, error
func prepareWikiFileName(gitRepo *git.Repository, wikiName string) (bool, string, error) {
blumia commented 2021-06-26 04:33:44 +00:00
Author
Contributor

Oh, I thought LsFiles() only check if the passed file exists. Sorry I don't code in Go too much and I should read the docs first XD

Thanks for your guidance!

Oh, I thought `LsFiles()` only check if the passed file exists. Sorry I don't code in Go too much and I should read the docs first XD Thanks for your guidance!
blumia reviewed 2021-06-26 11:37:04 +00:00
services/wiki/wiki.go
blumia commented 2021-06-26 11:37:04 +00:00
Author
Contributor

Do we still need to do this (replace space to dash) for unescaped filename?

Do we still need to do this (replace space to dash) for unescaped filename?
zeripath reviewed 2021-06-26 12:50:36 +00:00
services/wiki/wiki.go
zeripath commented 2021-06-26 12:50:17 +00:00
Contributor

I'd drop it.

I'd drop it. ```suggestion ```
zeripath reviewed 2021-06-26 12:53:10 +00:00
zeripath left a comment
Contributor
Ignore me I'm getting confused

We need to make:


	isWikiExist, newWikiPath, err := prepareWikiFileName(gitRepo, newWikiName)
	if err != nil {
		return err
	}

	if isNew {
		if isWikiExist {
			return models.ErrWikiAlreadyExist{
				Title: newWikiPath,
			}
		}
	} else {
		// avoid check existence again if wiki name is not changed since gitRepo.LsFiles(...) is not free.
		isOldWikiExist := true
		oldWikiPath := newWikiPath
		if oldWikiName != newWikiName {
			isOldWikiExist, oldWikiPath, err = prepareWikiFileName(gitRepo, oldWikiName)
			if err != nil {
				return err
			}
		}

		if isOldWikiExist {
			err := gitRepo.RemoveFilesFromIndex(oldWikiPath)
			if err != nil {
				log.Error("%v", err)
				return err
			}
		}
	}

make sense with the changes in prepareWikiFileName

<details> <summary> Ignore me I'm getting confused</summary> We need to make: ``` isWikiExist, newWikiPath, err := prepareWikiFileName(gitRepo, newWikiName) if err != nil { return err } if isNew { if isWikiExist { return models.ErrWikiAlreadyExist{ Title: newWikiPath, } } } else { // avoid check existence again if wiki name is not changed since gitRepo.LsFiles(...) is not free. isOldWikiExist := true oldWikiPath := newWikiPath if oldWikiName != newWikiName { isOldWikiExist, oldWikiPath, err = prepareWikiFileName(gitRepo, oldWikiName) if err != nil { return err } } if isOldWikiExist { err := gitRepo.RemoveFilesFromIndex(oldWikiPath) if err != nil { log.Error("%v", err) return err } } } ``` make sense with the changes in prepareWikiFileName </details>
zeripath approved these changes 2021-06-26 15:33:12 +00:00
zeripath left a comment
Contributor

This is fine

This is fine
6543 (Migrated from github.com) reviewed 2021-06-30 18:13:30 +00:00
services/wiki/wiki.go
6543 (Migrated from github.com) commented 2021-06-30 18:13:30 +00:00

// Pass in the raw unchanged name
// Add ".md" suffix if not there.

where is the check if it end with .md ?

// Pass in the raw unchanged name // Add ".md" suffix if not there. where is the check if it end with .md ?
6543 (Migrated from github.com) reviewed 2021-06-30 18:13:48 +00:00
services/wiki/wiki.go
@ -84,0 +88,4 @@
escaped := NameToFilename(wikiName)
// Look for both files
filesInIndex, err := gitRepo.LsFiles(unescaped, escaped)
6543 (Migrated from github.com) commented 2021-06-30 18:13:47 +00:00

a smal unit test would be nice :D

a smal unit test would be nice :D
blumia reviewed 2021-07-01 05:00:47 +00:00
services/wiki/wiki.go
blumia commented 2021-07-01 05:00:47 +00:00
Author
Contributor

That's the recommended step from this inline review. I notice that the original NameToFilename() didn't do that either, so do I need to add that logic to both NameToUnescapedFilename() and NameToFilename() or just remove that comment?

That's the recommended step from [this inline review](https://github.com/go-gitea/gitea/pull/16139#discussion_r655600166). I notice that the original `NameToFilename()` didn't do that either, so do I need to add that logic to both `NameToUnescapedFilename()` and `NameToFilename()` or just remove that comment?
blumia reviewed 2021-07-01 05:04:51 +00:00
services/wiki/wiki.go
@ -84,0 +88,4 @@
escaped := NameToFilename(wikiName)
// Look for both files
filesInIndex, err := gitRepo.LsFiles(unescaped, escaped)
blumia commented 2021-07-01 05:04:50 +00:00
Author
Contributor

I'd like to but since I'm also not very familiar with the whole Gitea codebase and how these tests works, I think it's better to not add a test case for this PR.

I'd like to but since I'm also not very familiar with the whole Gitea codebase and how these tests works, [I think it's better to not add a test case for this PR](https://github.com/go-gitea/gitea/pull/16139#issuecomment-860194111).
6543 (Migrated from github.com) reviewed 2021-07-01 10:53:54 +00:00
services/wiki/wiki.go
@ -84,0 +88,4 @@
escaped := NameToFilename(wikiName)
// Look for both files
filesInIndex, err := gitRepo.LsFiles(unescaped, escaped)
6543 (Migrated from github.com) commented 2021-07-01 10:53:54 +00:00

Hmm ok. If i get time I'll write one, or at least a frame :)

Hmm ok. If i get time I'll write one, or at least a frame :)
6543 (Migrated from github.com) reviewed 2021-07-01 10:54:42 +00:00
services/wiki/wiki.go
6543 (Migrated from github.com) commented 2021-07-01 10:54:42 +00:00

The function then seems bit useles and we should remove it then

The function then seems bit useles and we should remove it then
blumia reviewed 2021-07-01 11:04:54 +00:00
services/wiki/wiki.go
@ -82,2 +82,4 @@
}
// prepareWikiFileName try to find a suitable file path with file name by the given raw wiki name.
// return: existence, prepared file path with name, error
blumia commented 2021-07-01 11:04:53 +00:00
Author
Contributor

I'd prefer to remove this comment and don't do the check since Markdown is the only supported markup language in Wiki. User doesn't need to know or manually specify the file extension here.

```suggestion ``` I'd prefer to remove this comment and don't do the check since Markdown is the only supported markup language in Wiki. User doesn't need to know or manually specify the file extension here.
blumia reviewed 2021-07-01 11:10:18 +00:00
services/wiki/wiki.go
blumia commented 2021-07-01 11:10:18 +00:00
Author
Contributor

I prefer to remove the comment and added a commit to remove these two lines of comment, let me know if you think I should add the check instead of remove it, thanks!

I prefer to remove the comment and added a commit to remove these two lines of comment, let me know if you think I should add the check instead of remove it, thanks!
blumia reviewed 2021-07-01 15:56:17 +00:00
services/wiki/wiki.go
blumia commented 2021-07-01 15:56:16 +00:00
Author
Contributor

The function then seems bit useles and we should remove it then

Oh! Sorry I missed this comment. A new commit has been added to remove that function now.

> The function then seems bit useles and we should remove it then Oh! Sorry I missed this comment. A new commit has been added to remove that function now.
techknowlogick approved these changes 2021-07-07 22:47:10 +00:00
techknowlogick left a comment
Contributor

Thanks :)

Thanks :)
6543 (Migrated from github.com) reviewed 2021-07-19 16:28:00 +00:00
services/wiki/wiki.go
@ -84,0 +88,4 @@
escaped := NameToFilename(wikiName)
// Look for both files
filesInIndex, err := gitRepo.LsFiles(unescaped, escaped)
6543 (Migrated from github.com) commented 2021-07-19 16:28:00 +00:00

-> https://github.com/go-gitea/gitea/pull/16487

-> https://github.com/go-gitea/gitea/pull/16487
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
zeripath
techknowlogick
Labels
Clear labels   
backport/done

   
backport/v1.0

   
backport/v1.1

   
backport/v1.10

   
backport/v1.11

   
backport/v1.12

   
backport/v1.13

   
backport/v1.14

   
backport/v1.15

   
backport/v1.2

   
backport/v1.3

   
backport/v1.4

   
backport/v1.5

   
backport/v1.6

   
backport/v1.7

   
backport/v1.8

   
backport/v1.9

   
bounty

   
changelog

   
dependencies

Pull requests that update a dependency file

  
frontport/done

   
frontport/main

   
good first issue

Likely to be an easy fix

  
Hacktoberfest

   
hacktoberfest-accepted

   
in progress

   
kind/api

   
kind/breaking

   
kind/bug

   
kind/build

   
kind/deployment

   
kind/deprecated

   
kind/docs

   
kind/enhancement

   
kind/feature

   
kind/lint

   
kind/misc

   
kind/moderation

features for moderateing spam, abuse, ...

  
kind/package

   
kind/proposal

   
kind/question

   
kind/refactor

   
kind/regression

   
kind/security

   
kind/summary

   
kind/testing

   
kind/translation

   
kind/ui

   
kind/upstream-related

An issue that is caused by an upstream dependency

  
kind/usability

   
kind/ux

   
lgtm/done

   
lgtm/need 1

   
lgtm/need 2

   
performance/bigrepo

Performance Issues affecting Big Repositories

  
performance/cpu

   
performance/memory

Performance issues affecting memory use

  
performance/speed

performance issues with slow downs

  
priority/critical

   
priority/low

   
priority/maybe

   
priority/medium

   
proposal/rejected

   
reviewed/confirmed

Issue has been reviewed and confirmed to be present or accepted to be implemented

  
reviewed/duplicate

   
reviewed/fixed

A fix for this issue is already in the attached milestone

  
reviewed/invalid

   
reviewed/not-a-bug

The reported issue is the intended behavior

  
reviewed/wontfix

   
skip-changelog

   
stale

   
status/blocked

   
status/needs-feedback

   
status/wip

   
theme/2fa

   
theme/authentication

   
theme/avatar

   
theme/backup-restore

   
theme/docker

Docker themed issued

  
theme/federation

   
theme/issues

   
theme/kanban

   
theme/markdown

   
theme/migration

Migration issues

  
theme/mobile

   
theme/pr

   
theme/signing

   
theme/sqlite

   
theme/timetracker

   
theme/webhook

   
theme/wiki
No Label
backport/done
backport/v1.0
backport/v1.1
backport/v1.10
backport/v1.11
backport/v1.12
backport/v1.13
backport/v1.14
backport/v1.15
backport/v1.2
backport/v1.3
backport/v1.4
backport/v1.5
backport/v1.6
backport/v1.7
backport/v1.8
backport/v1.9
bounty
changelog
dependencies
frontport/done
frontport/main
good first issue
Hacktoberfest
hacktoberfest-accepted
in progress
kind/api
kind/breaking
kind/bug
kind/build
kind/deployment
kind/deprecated
kind/docs
kind/enhancement
kind/feature
kind/lint
kind/misc
kind/moderation
kind/package
kind/proposal
kind/question
kind/refactor
kind/regression
kind/security
kind/summary
kind/testing
kind/translation
kind/ui
kind/upstream-related
kind/usability
kind/ux
lgtm/done
lgtm/need 1
lgtm/need 2
performance/bigrepo
performance/cpu
performance/memory
performance/speed
priority/critical
priority/low
priority/maybe
priority/medium
proposal/rejected
reviewed/confirmed
reviewed/duplicate
reviewed/fixed
reviewed/invalid
reviewed/not-a-bug
reviewed/wontfix
skip-changelog
stale
status/blocked
status/needs-feedback
status/wip
theme/2fa
theme/authentication
theme/avatar
theme/backup-restore
theme/docker
theme/federation
theme/issues
theme/kanban
theme/markdown
theme/migration
theme/mobile
theme/pr
theme/signing
theme/sqlite
theme/timetracker
theme/webhook
theme/wiki
Milestone
Clear milestone
No items
No Milestone
1.15.0
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
4 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: lunny/gitea#16139
No description provided.
Delete Branch "fix-local-wiki-update"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?