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.

Sign up for GitHub

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

Jump to bottom

Set default lfs content path to data/lfs #2521

Merged
merged 3 commits into from
Oct 19, 2017
Merged

Set default lfs content path to data/lfs #2521

merged 3 commits into from
Oct 19, 2017

Conversation

cez81
Copy link
Contributor

@cez81 cez81 commented Sep 16, 2017

As title.

gitea/conf/app.ini

Line 161 in d766d0c

LFS_CONTENT_PATH = data/lfs
it's already correct in app.ini

Fix #2508

@codecov-io
Copy link

codecov-io commented Sep 16, 2017 •
edited
Loading

Codecov Report

Merging #2521 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2521   +/-   ##
=======================================
  Coverage   26.91%   26.91%           
=======================================
  Files          87       87           
  Lines       17286    17286           
=======================================
  Hits         4652     4652           
  Misses      11955    11955           
  Partials      679      679

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2222448...6b567e3. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 16, 2017
lafriks
lafriks requested changes Sep 16, 2017
View reviewed changes
modules/setting/setting.go Outdated
@@ -737,6 +737,9 @@ func NewContext() {
}

if LFS.StartServer {
if LFS.ContentPath == "" {
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs something like:

	sec = Cfg.Section("lfs")
	LFS.ContentPath = sec.Key("CONTENT_PATH").MustString(path.Join(AppDataPath, "lfs"))
	if !filepath.IsAbs(LFS.ContentPath) {
		LFS.ContentPath = filepath.Join(workDir, LFS.ContentPath)
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. Need to check if absolute path. Think the first bit is ok tho?

Copy link
Member

Choose a reason for hiding this comment

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

To be more consistent with other code I think it should be something like I gave example. Only thing I also have wrong for FS paths better is to use filepath methods not path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough 👍 I have updated accordingly :)

Copy link
Member

Choose a reason for hiding this comment

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

@cez81 did you forgot to do git push?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to commit... :/

@cez81 cez81 force-pushed the lfs_path branch 3 times, most recently from fc70763 to b3253b0 Compare September 16, 2017 17:22
lafriks
lafriks requested changes Sep 16, 2017
View reviewed changes
modules/setting/setting.go
log.Fatal(4, "Failed to map LFS settings: %v", err)
}
LFS.ContentPath = sec.Key("LFS_CONTENT_PATH").MustString(filepath.Join(AppDataPath, "lfs"))
Copy link
Member

Choose a reason for hiding this comment

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

either move this line after if LFS.StartServer { or move if ...IsAbs(... after this line so that both these lines are together

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

Set it like this instead: https://github.com/cez81/gitea/blob/d766d0c4e064bf7f66098123f39d15c2dc67e415/modules/setting/setting.go#L116

}{
  ContentPath: "data/lfs"
}

As seen for example here: https://github.com/cez81/gitea/blob/d766d0c4e064bf7f66098123f39d15c2dc67e415/modules/setting/setting.go#L214

Copy link
Member

Choose a reason for hiding this comment

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

we really need to decide how to specify default value in code. We have at least two different ways to do that that is not very good :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think setting it like this is better. Don't want to hard code the data folder, right?

@lafriks
Copy link
Member

lafriks commented Sep 16, 2017

LGTM

@lafriks
Copy link
Member

lafriks commented Sep 16, 2017

Make LG-TM work

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 16, 2017
@lunny lunny added this to the 1.x.x milestone Sep 19, 2017
@lunny lunny added the type/enhancement An improvement of existing functionality label Sep 19, 2017
@cez81 cez81 force-pushed the lfs_path branch 2 times, most recently from b8a1feb to 0bbe2a4 Compare October 17, 2017 21:04
@lunny
Copy link
Member

lunny commented Oct 19, 2017

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 19, 2017
@lunny lunny modified the milestones: 1.x.x, 1.3.0 Oct 19, 2017
@lafriks lafriks merged commit d4c9399 into go-gitea:master Oct 19, 2017
@cez81 cez81 deleted the lfs_path branch October 19, 2017 06:54
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lafriks lafriks lafriks approved these changes

@bkcsoft bkcsoft bkcsoft left review comments

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.

LFS_CONTENT_PATH defaults to ''
6 participants
@cez81 @codecov-io @lafriks @lunny @bkcsoft @tboerger