search

charmbracelet / git-lfs-transfer

Server-side implementation of the Git LFS pure-SSH protocol
MIT License
52 stars 6 forks source link

Refactor backend interface #47

Closed ConcurrentCrab closed 3 months ago

ConcurrentCrab commented 3 months ago

Hi charm.sh,

I was implementing the lfs-transfer protocol for gitea (https://github.com/go-gitea/gitea/issues/17554), and in the process I vendored your excellent transfer library from this repo. I made some changes for it to work better with Gitea's internal API, but I think those are improvements in the general sense too, hence I'm opening this PR.

All tests pass with the internal local backend in this repo.

Most of these changes are pretty self explanatory I hope, only the last commit is sort of contentious. In its defence, the old 2-stage Upload API was frankly horrific with the UploadState and whatnot. This works around that by using the same clever trick as HashingReader: a VerifyingReader. This makes it much simpler for backends, as handling corrupt input is just the same as handling errors from the reader. Also makes it very simple for backends that verify input themselves to just omit wrapping the reader.

ConcurrentCrab commented 3 months ago

Also please tag, the last one is from ~2 years ago.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 65.30612% with 17 lines in your changes missing coverage. Please review.

Project coverage is 57.36%. Comparing base (be41c53) to head (575c161). Report is 19 commits behind head on master.

Files Patch % Lines
internal/local/backend.go 53.57% 8 Missing and 5 partials :warning:
transfer/hash.go 84.61% 1 Missing and 1 partial :warning:
transfer/processor.go 75.00% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #47 +/- ## ========================================== + Coverage 50.19% 57.36% +7.16% ========================================== Files 14 14 Lines 1018 849 -169 ========================================== - Hits 511 487 -24 + Misses 392 249 -143 + Partials 115 113 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ConcurrentCrab commented 3 months ago

@aymanbagabas not the exact change you suggested but I think this looks good as well.

aymanbagabas commented 3 months ago

@aymanbagabas not the exact change you suggested but I think this looks good as well.

Even better, thank you!

aymanbagabas commented 3 months ago

@ConcurrentCrab could you please rebase the commits to use conventional commits?

ConcurrentCrab commented 3 months ago

@aymanbagabas sure but... I'm not sure what else that would entail? FAICT all commits are in the format

<scope>: <title>

<body>
aymanbagabas commented 3 months ago

@ConcurrentCrab basically this

[feat|fix|refactor](<scope>): <title>

<body>

https://www.conventionalcommits.org/en/v1.0.0/

ConcurrentCrab commented 3 months ago

@aymanbagabas reworded the commit messages.