Fix #732: Add LFS objects to base repository on merging #7082
Conversation
GiteaBot
added
the
lgtm/need 2
Codecov Report
@@ Coverage Diff @@
## master #7082 +/- ##
==========================================
+ Coverage 41.53% 41.73% +0.19%
==========================================
Files 449 451 +2
Lines 61344 61531 +187
==========================================
+ Hits 25478 25678 +200
+ Misses 32511 32471 -40
- Partials 3355 3382 +27
Continue to review full report at Codecov.
|
zeripath
changed the title
This switches from relying on having git-lfs installed on the server, (and in fact .gitattributes being correctly installed.) Instead on merge we walk the merge history and ensure that all lfs objects pointed to in the history are added to the base repository.
zeripath
force-pushed
the
route-lfs-internally-fix-#732
branch
from
4ae0753 to
02a0c26
Compare
|
I've expanded git_test to include showing that on merging a fork the lfs objects remain available. |
|
I've also expanded the git_test to delete the repository before checking if the lfs objects are available in the merged repository. |
modules/merge/lfs.go
Outdated
| @@ -0,0 +1,226 @@ | |||
| // Copyright 2019 The Gitea Authors. | |||
There was a problem hiding this comment.
How about use modules/pull but not modules/merge so that we could move more pull related codes to this package.
There was a problem hiding this comment.
Well we could just rename this when that's done but if you prefer I can do that now.
A few of the pipelines used in the lfs code are reusable, e.g. in #7199 - so may be some of those should be moved to git (or migrated to go-git code over time.)
We need to read the file plus the terminal newline that cat-file sends - hence 1024 + 1.
Codecov Report
@@ Coverage Diff @@
## master #7082 +/- ##
=========================================
+ Coverage 40.98% 41.18% +0.2%
=========================================
Files 462 464 +2
Lines 62584 62771 +187
=========================================
+ Hits 25648 25852 +204
+ Misses 33574 33530 -44
- Partials 3362 3389 +27
Continue to review full report at Codecov.
|
GiteaBot
added
lgtm/need 1
| } | ||
|
|
||
| func doRevListObjects(revListWriter *io.PipeWriter, wg *sync.WaitGroup, tmpBasePath, headSHA, baseSHA string, errChan chan<- error) { | ||
| defer wg.Done() |
There was a problem hiding this comment.
How about
defer func() {
wg.Done()
revListWriter.Close()
}()There was a problem hiding this comment.
Hmm AFAIU defers run in spite of panics. If wg.Done() panics then revListWriter will not close if we do this suggestion.
There was a problem hiding this comment.
(not that that particularly matters as the panic will be unrecovered in the goroutine and thus bring down gitea)
|
Otherwise LGTM |
GiteaBot
added
lgtm/done
…itea#7082) On merge we walk the merge history and ensure that all lfs objects pointed to in the history are added to the base repository. This switches from relying on having git-lfs installed on the server, (and in fact .gitattributes being correctly installed.)
This PR Fixes #732.
It first disables the standard git-lfs filters including the missing process, (filter.lfs.process, filter.lfs.clean, filter.lfs.smudge, filter.lfs.required.)
Then just before merging it walks the merging history ensuring that all files in the history to be merged, that are <1k, and match LFS pointer files - actually representing lfs objects in the content store are then added to the base repositories lfs objects.
This PR attempts to Fix #732 (Yes this bug is that old) whereby trying to merge a pull request containing LFS objects fails. This is substantially better than simply ignoring the missed filter.lfs.process which is an alternative fix as that technique likely leads to the loss of lfs objects on forked repo deletion.The current implementation just routes the requests to the internal url for the gitea server - however this may not work if Gitea is running over a unix pipe and therefore likely requires more thought.It does however provide a fix.