Git LFS lock api#2938

sapk · 2017-11-20T00:49:16Z

codecov-io · 2017-11-22T00:16:01Z

Codecov Report

Merging #2938 into master will increase coverage by 0.3%.
The diff coverage is 56.61%.

@@ Coverage Diff @@## master #2938 +/- ## ========================================= + Coverage 32.73% 33.03% +0.3%  ========================================= Files 267 269 +2 Lines 39189 39483 +294 ========================================= + Hits 12828 13044 +216 - Misses 24539 24593 +54 - Partials 1822 1846 +24

Impacted Files	Coverage Δ
routers/routes/routes.go	`86.99% <100%> (+0.64%)`	⬆️
models/models.go	`49.33% <100%> (+0.22%)`	⬆️
modules/lfs/locks.go	`47.59% <47.59%> (ø)`
models/lfs_lock.go	`69.76% <69.76%> (ø)`
models/error.go	`30.98% <73.33%> (+2.04%)`	⬆️
models/repo_indexer.go	`49% <0%> (-2.98%)`	⬇️
modules/indexer/repo.go	`60.86% <0%> (-2.61%)`	⬇️
models/repo.go	`38% <0%> (+0.18%)`	⬆️
... and 5 more

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 6ad4990...a987dda. Read the comment docs.

sapk · 2017-11-22T00:23:34Z

It is ready for review.

lunny · 2017-11-23T12:25:25Z

models/lfs_lock.go

... 2017 The Gitea ... ?

lunny · 2017-11-23T12:25:37Z

models/lfs_lock.go

wrong order

lunny · 2017-11-23T12:25:55Z

models/lfs_lock.go

lfock -> lock

lunny · 2017-11-23T12:27:02Z

models/lfs_lock.go

Use created is enough, then line 28 -> 30 and line 36 could be removed.

I base myself on other models like :
gitea/models/issue_stopwatch.go
Line 24 in a8717e5
s.CreatedUnix=time.Now().Unix()

After review more of those, I might be needed adding xorm:"INDEX created" on created like :
gitea/models/token.go
Line 23 in aa962de
CreatedUnixint64`xorm:"INDEX created"`

After adding that, does I need ?
gitea/models/token.go
Line 32 in aa962de
t.Created=time.Unix(t.CreatedUnix, 0).Local()

I will totally follow you on xorm since you will know more about it and maybe (in another PR) we should clean other models with such pattern.

lunny · 2017-11-23T12:30:30Z

models/lfs_lock.go

maybe store a lower column?

lunny · 2017-11-23T12:37:14Z

models/lfs_lock.go

IsLFSLockExist has been invoked on GetLFSLock.

@lunny To be sure, I would better use directly GetLFSLock and check if !IsErrLFSLockNotExist(err) ?

lunny · 2017-11-23T12:37:49Z

models/lfs_lock.go

…e testing

Save a little of memory and cpu time.

lunny · 2017-11-26T01:49:54Z

models/lfs_lock.go

+ RepoID int64 `xorm:"INDEX"`
+ Owner *User `xorm:"-"`
+ OwnerID int64 `xorm:"INDEX"`
+ Path string `xorm:"TEXT"`


And maybe Path should be unique with RepoID ?

Yes and that make me think that I should also use filepath.Clean before adding a entry.

Done + add not null constraint

Revert back the uniqueness since mysql need a fixed size to index uniquess on field and path is a TEXT field. Uniqueness is preserve by code.

Path could be xorm:"varchar(1024)".

after looking at it linux limits path length to 4096

lunny · 2017-11-28T02:19:34Z

LGTM

lafriks · 2017-11-28T17:16:11Z

@sapk please resolve vendor conflict

sapk · 2017-11-28T20:42:36Z

@lafriks done

lafriks · 2017-11-28T20:49:02Z

LGTM

lunny added the type/enhancement An improvement of existing functionality label Nov 20, 2017

lunny added this to the 1.x.x milestone Nov 20, 2017

lafriks added the pr/wip This PR is not ready for review label Nov 20, 2017

sapk force-pushed the git-lfs-lock-api branch 2 times, most recently from 9458fea to 744a07eCompare November 21, 2017 22:05

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 22, 2017

sapk changed the title ~~[WIP] Git LFS lock api~~Git LFS lock apiNov 22, 2017

lunny removed the pr/wip This PR is not ready for review label Nov 22, 2017

lunny modified the milestones: 1.x.x, 1.4.0 Nov 22, 2017

lunny reviewed Nov 23, 2017
View reviewed changes

models/lfs_lock.go Outdated
Copy link
Member
lunnyNov 23, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... 2017 The Gitea ... ?
sapk reacted with thumbs up emoji

lunny requested changes Nov 23, 2017
View reviewed changes

sapk added 13 commits November 24, 2017 23:25

Implement routes
55f5424

move to api/sdk and create model
80b3ce0

Implement add + list
144ab60

List return 200 empty list no 404
3ef7315

Add verify lfs lock api
8ac2219

Add delete and start implementing auth control
b3ad366

Revert to code.gitea.io/sdk/gitea vendor
bcd3b39

Apply needed check for all lfs locks route
74a04bc

Add simple tests
82bab1b

fix lint
cc66370

Improve tests
18eed10

Add delete test + fix
61606fc

Add lfs ascii header
3015595

sapk force-pushed the git-lfs-lock-api branch from 964e7ee to 3015595Compare November 24, 2017 22:26

sapk added 2 commits November 25, 2017 00:19

Various fixes from review + remove useless code + add more corner cas…
5b3b301
…e testing

Remove repo link since only id is needed.
b37282e
Save a little of memory and cpu time.

lunny reviewed Nov 26, 2017
View reviewed changes

sapk added 3 commits November 26, 2017 18:45

Clean path
bd3e9e7

Improve DB format
caf92f9

Revert uniquess repoid+path
2988c10

sapk force-pushed the git-lfs-lock-api branch from 29d0c3a to 2988c10Compare November 26, 2017 23:58

sapk added 2 commits November 27, 2017 03:29

(Re)-setup uniqueness + max path length
b6acea8

Fixed TEXT in place of VARCHAR
40b3cbb

sapk force-pushed the git-lfs-lock-api branch from 1af481d to 40b3cbbCompare November 27, 2017 23:14

Settle back to maximum VARCHAR(3072)
ca344b5

sapk force-pushed the git-lfs-lock-api branch from b543065 to ca344b5Compare November 27, 2017 23:27

Let place for repoid in key
ab0a3eb

sapk force-pushed the git-lfs-lock-api branch from 7798ef5 to ab0a3ebCompare November 27, 2017 23:37

sapk added 3 commits November 28, 2017 00:40

Let place for repoid in key
d13c73e

Let place for repoid in key
59565f4

Revert back
0180efa

sapk force-pushed the git-lfs-lock-api branch from d74a2ed to 0180efaCompare November 27, 2017 23:52

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 Nov 28, 2017

Merge branch 'master' into git-lfs-lock-api
a987dda

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 Nov 28, 2017

lafriks merged commit d99f4ab into go-gitea:masterNov 28, 2017

sapk mentioned this pull request Nov 28, 2017
Fix git lfs path #3016
Merged

sapk deleted the git-lfs-lock-api branch December 9, 2017 21:47

lafriks mentioned this pull request Jan 12, 2018
Add draft blog post for 1.4.0 release go-gitea/blog#54
Merged

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Uh oh!

Git LFS lock api#2938

Git LFS lock api #2938

Uh oh!

Conversation

sapk commented Nov 20, 2017

Uh oh!

codecov-io commented Nov 22, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sapk commented Nov 22, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sapkNov 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lunny commented Nov 28, 2017

Uh oh!

lafriks commented Nov 28, 2017

Uh oh!

sapk commented Nov 28, 2017

Uh oh!

lafriks commented Nov 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Nov 22, 2017•
edited
Loading

sapk commented Nov 22, 2017•
edited
Loading

sapkNov 23, 2017•
edited
Loading