Git LFS lock api #2938

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

Resolve #2725

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

CreatedUnix int64 `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 744a07e Compare 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 api Nov 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

lunny Nov 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 ... ?

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 3015595 Compare 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 2988c10 Compare 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 40b3cbb Compare November 27, 2017 23:14

Settle back to maximum VARCHAR(3072)

ca344b5

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

Let place for repoid in key

ab0a3eb

sapk force-pushed the git-lfs-lock-api branch from 7798ef5 to ab0a3eb Compare 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 0180efa Compare 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:master Nov 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Git LFS lock api #2938

Git LFS lock api #2938

Uh oh!

sapk commented Nov 20, 2017

codecov-io commented Nov 22, 2017 •

edited

Loading

sapk commented Nov 22, 2017 •

edited

Loading

lunny Nov 23, 2017

lunny Nov 23, 2017

lunny Nov 23, 2017

lunny Nov 23, 2017

sapk Nov 23, 2017 •

edited

Loading

lunny Nov 23, 2017

lunny Nov 23, 2017

sapk Nov 23, 2017

lunny Nov 23, 2017

lunny Nov 26, 2017

sapk Nov 26, 2017

sapk Nov 26, 2017

sapk Nov 26, 2017

lunny Nov 27, 2017

sapk Nov 27, 2017

lunny commented Nov 28, 2017

lafriks commented Nov 28, 2017

sapk commented Nov 28, 2017

lafriks commented Nov 28, 2017

Labels

5 participants

Uh oh!

Git LFS lock api #2938

Git LFS lock api #2938

Uh oh!

Conversation

sapk commented Nov 20, 2017

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

Uh oh!

Codecov Report

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

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapk Nov 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Nov 28, 2017

lafriks commented Nov 28, 2017

sapk commented Nov 28, 2017

lafriks commented Nov 28, 2017

Labels

5 participants

codecov-io commented Nov 22, 2017 •

edited

Loading

sapk commented Nov 22, 2017 •

edited

Loading

sapk Nov 23, 2017 •

edited

Loading