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

Git LFS lock api #2938

Merged
merged 30 commits into from Nov 28, 2017

Conversation

5 participants
@sapk
Member

sapk commented Nov 20, 2017

Resolve #2725

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

@lafriks lafriks added the status/wip label Nov 20, 2017

@sapk sapk force-pushed the sapk-fork:git-lfs-lock-api branch 2 times, most recently from 9458fea to 744a07e Nov 21, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Nov 22, 2017

Codecov Report

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

Impacted file tree graph

@@            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 sapk changed the title from [WIP] Git LFS lock api to Git LFS lock api Nov 22, 2017

@sapk

This comment has been minimized.

Member

sapk commented Nov 22, 2017

It is ready for review.

@lunny lunny removed the status/wip label Nov 22, 2017

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

@@ -0,0 +1,173 @@
// Copyright 2014 The Gogs Authors. All rights reserved.

This comment has been minimized.

@lunny

lunny Nov 23, 2017

Member

... 2017 The Gitea ... ?

package models
import (
api "code.gitea.io/sdk/gitea"

This comment has been minimized.

@lunny

lunny Nov 23, 2017

Member

wrong order

"time"
)
// LFSLock represents a git lfs lfock of repository.

This comment has been minimized.

@lunny

lunny Nov 23, 2017

Member

lfock -> lock

Owner *User `xorm:"-"`
Path string
Created time.Time `xorm:"-"`
CreatedUnix int64 `xorm:"INDEX"`

This comment has been minimized.

@lunny

lunny Nov 23, 2017

Member

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

This comment has been minimized.

@sapk

sapk Nov 23, 2017

Member

I base myself on other models like :

s.CreatedUnix = time.Now().Unix()

After review more of those, I might be needed adding xorm:"INDEX created" on created like :
CreatedUnix int64 `xorm:"INDEX created"`

After adding that, does I need ?
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.

// IsLFSLockExist returns true if lock with given path already exists.
func IsLFSLockExist(repoID int64, path string) (bool, error) {
return x.Get(&LFSLock{RepoID: repoID, Path: path}) //TODO Define if path should needed to be lower for windows compat ?

This comment has been minimized.

@lunny

lunny Nov 23, 2017

Member

maybe store a lower column?

if err != nil {
return nil, err
} else if isExist {
l, err := GetLFSLock(lock.RepoID, lock.Path)

This comment has been minimized.

@lunny

lunny Nov 23, 2017

Member

IsLFSLockExist has been invoked on GetLFSLock.

This comment has been minimized.

@sapk

sapk Nov 23, 2017

Member

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

// GetLFSLockByRepoID returns a list of locks of repository.
func GetLFSLockByRepoID(repoID int64) (locks []*LFSLock, err error) {
err = x.Where("repo_id = ?", repoID).Find(&locks)
return locks, err

This comment has been minimized.

@lunny

lunny Nov 23, 2017

Member

return

sapk added some commits Nov 18, 2017

@sapk sapk force-pushed the sapk-fork:git-lfs-lock-api branch from 964e7ee to 3015595 Nov 24, 2017

sapk added some commits Nov 24, 2017

Remove repo link since only id is needed.
Save a little of memory and cpu time.
RepoID int64 `xorm:"INDEX"`
Owner *User `xorm:"-"`
OwnerID int64 `xorm:"INDEX"`
Path string `xorm:"TEXT"`

This comment has been minimized.

@lunny

lunny Nov 26, 2017

Member

And maybe Path should be unique with RepoID ?

This comment has been minimized.

@sapk

sapk Nov 26, 2017

Member

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

This comment has been minimized.

@sapk

sapk Nov 26, 2017

Member

Done + add not null constraint

This comment has been minimized.

@sapk

sapk Nov 26, 2017

Member

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.

This comment has been minimized.

@lunny

lunny Nov 27, 2017

Member

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

This comment has been minimized.

@sapk

sapk Nov 27, 2017

Member

after looking at it linux limits path length to 4096

sapk added some commits Nov 26, 2017

@sapk sapk force-pushed the sapk-fork:git-lfs-lock-api branch from 29d0c3a to 2988c10 Nov 26, 2017

sapk added some commits Nov 27, 2017

@sapk sapk force-pushed the sapk-fork:git-lfs-lock-api branch from 1af481d to 40b3cbb Nov 27, 2017

@sapk sapk force-pushed the sapk-fork:git-lfs-lock-api branch from b543065 to ca344b5 Nov 27, 2017

@sapk sapk force-pushed the sapk-fork:git-lfs-lock-api branch from 7798ef5 to ab0a3eb Nov 27, 2017

sapk added some commits Nov 27, 2017

@sapk sapk force-pushed the sapk-fork:git-lfs-lock-api branch from d74a2ed to 0180efa Nov 27, 2017

@lunny

This comment has been minimized.

Member

lunny commented Nov 28, 2017

LGTM

@tboerger tboerger added lgtm/need 1 and removed lgtm/need 2 labels Nov 28, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Nov 28, 2017

@sapk please resolve vendor conflict

@sapk

This comment has been minimized.

Member

sapk commented Nov 28, 2017

@lafriks done

@lafriks

This comment has been minimized.

Member

lafriks commented Nov 28, 2017

LGTM

@tboerger tboerger added lgtm/done and removed lgtm/need 1 labels Nov 28, 2017

@lafriks lafriks merged commit d99f4ab into go-gitea:master Nov 28, 2017

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details

@sapk sapk referenced this pull request Nov 28, 2017

Merged

Fix git lfs path #3016

@sapk sapk deleted the sapk-fork:git-lfs-lock-api branch Dec 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment