Skip to content

Conversation

@lunny
Copy link
Member

@lunny lunny commented Jan 5, 2026

Replace the Batch interface with the new ObjectInfoPool and ObjectPool interfaces. This avoids exposing the writer and limits access to a read-only interface for Git object content. In addition, this abstraction makes it possible to support alternative implementations, such as a go-git–based ObjectPool or a remote ObjectPool, in the future.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 5, 2026
@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jan 5, 2026
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Jan 5, 2026
@wxiaoguang
Copy link
Contributor

You just don't know what you are doing.

Such design is definitely wrong. You should never make an "Object" (which is also a horribly bad name) contain stateful fields.

@lunny
Copy link
Member Author

lunny commented Jan 5, 2026

You just don't know what you are doing.

Such design is definitely wrong. You should never make an "Object" (which is also a horribly bad name) contain stateful fields.

I adjusted the interface of Object which now returned 3 parameters and the reader will not be the member of ObjectInfo. The object is the official name from git. ls .git you can find an objects directory which contains tag, commit, tree. All of them are named object. Would you have a better name for it?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 6, 2026

Besides those problems, the more essential problem is: such design doesn't help the roadmap like #34651

What you does in this PR is just "rename NewBatch to NewObjectPool / NewBatchCheck to NewObjectInfoPool", there is no real improvement. You still have two "batch cat file" related structs and two processes.

If your goal is something like #34651 , you need a design for it and really does the refactor, but not just moving and renaming code.

@lunny
Copy link
Member Author

lunny commented Jan 6, 2026

Besides those problems, the more essential problem is: such design doesn't help the roadmap like #34651

What you does in this PR is just "rename NewBatch to NewObjectPool / NewBatchCheck to NewObjectInfoPool", there is no real improvement. You still have two "batch cat file" related structs and two processes.

If your goal is something like #34651 , you need a design for it and really does the refactor, but not just moving and renaming code.

Yes. This PR does not address #34651, and at the moment I don’t have a clear solution for that issue.

The new interface introduced here allows us to use the interface ObjectPool instead of struct Repository in Commit and Tree or other struct. With this change, we can support three types of implementations going forward: the existing command-based implementation, a go-git–based implementation, and a remote command–based implementation.

@wxiaoguang
Copy link
Contributor

If you don't have a clear solution for that issue, I don't see why&how you can "support three types of implementations going forward: the existing command-based implementation, a go-git–based implementation, and a remote command–based implementation". Just more rubbish.

@lafriks

This comment was marked as off-topic.

@wxiaoguang

This comment was marked as off-topic.

@wxiaoguang

This comment was marked as off-topic.

@lunny
Copy link
Member Author

lunny commented Jan 7, 2026

If you don't have a clear solution for that issue, I don't see why&how you can "support three types of implementations going forward: the existing command-based implementation, a go-git–based implementation, and a remote command–based implementation". Just more rubbish.

I sent #36313 which will replace this one and the new interface will be suitable for git batch-command. This will be closed.

@lunny lunny closed this Jan 7, 2026
@lunny lunny deleted the lunny/refactor_object_pool branch January 7, 2026 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/go Pull requests that update Go code type/refactoring Existing code has been cleaned up. There should be no new functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants