Skip to content
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

Adding locking_notes.md #5079

Merged
merged 4 commits into from Aug 8, 2022
Merged

Adding locking_notes.md #5079

merged 4 commits into from Aug 8, 2022

Conversation

pratap043
Copy link
Contributor

This is a PR with the proposal for enhancing the Git LFS REST API to capture additional notes on the locks.

This is a PR with the proposal for enhancing the Git LFS REST API.
Copy link
Member

@bk2204 bk2204 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey,

I think this is a good start. In addition to a more generic description, I'd like to see a proposed API body and response, much like in the actual API documentation.

docs/proposals/locking_notes.md Outdated Show resolved Hide resolved
Updated the proposal document by adding the API's expected Request and Response Json data.
@pratap043
Copy link
Contributor Author

Thanks Brian (@bk2204). I have updated the document with the changes as suggested. Kindly check once and let me know for further modifications if any.

@pratap043
Copy link
Contributor Author

Hi Brian (@bk2204 ). I have updated the document by removing the specific use case to make it generic, thank you.

Copy link
Member

@bk2204 bk2204 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thanks.

@bk2204
Copy link
Member

bk2204 commented Jul 25, 2022

I'm going to accept this as a proposal. That means that we'll consider it for possible implementation in a future version, but due to implementation constraints, it's possible that it will change before it's shipped as we discover additional requirements.

It doesn't mean that we'll be implementing it in a particular timeframe or that we'll immediately accept a PR implementing it, since we typically will want to see at least one major implementer commit to implement it at some point.

I'm also going to ping @git-lfs/implementers since this is a thing they'll be interested in.

@pratap043
Copy link
Contributor Author

Thanks so much Brian @bk2204.

< }
```

### Request (with out notes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually want this? Such feature (answering without notes after implementation adds notes support) will actually require noticeable amounts of code. Both git-lfs and https://github.com/git-as-svn/git-lfs-java simply call JSON serializer on a Lock structure.

git-lfs: https://github.com/git-lfs/git-lfs/blob/v3.2.0/locking/locks.go#L200-L211
git-lfs-java: https://github.com/git-as-svn/git-lfs-java/blob/v0.19.0/gitlfs-common/src/main/kotlin/ru/bozaro/gitlfs/common/data/Lock.kt#L6-L30

And it is nontrivial to conditionally exclude a field from serialization.

The only reason to omit notes that I can come up with is network traffic.

Except this thing, I give my whole +1 to this proposal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine for the server to simply return any notes that are present, whether or not any were requested. If no notes are requested on the client side, we'll just not emit the field, and then the server can either return them if appropriate or not.

@bk2204 bk2204 merged commit 7001116 into git-lfs:main Aug 8, 2022
@pratap043 pratap043 deleted the git-lfs-notes branch August 17, 2022 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants