Skip to content

Conversation

lunny
Copy link
Member

@lunny lunny commented Jun 8, 2025

For git version 2.36, git cat-file --batch-command was introduced which can replace git cat-file --batch and git cat-file --batch-check.

This PR implements an abstract layer for the batch commands so that both git 2.36 and lower version git can work.
If git version is lower than 2.36, it will start two subprocesses git cat-file --batch and git cat-file --batch-check.
If git version is greater than 2.36, only git cat-file --batch-command will be started.

This reduced half of child processes of git catfiles.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jun 8, 2025
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 8, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Jun 8, 2025
@lunny lunny added this to the 1.25.0 milestone Jun 8, 2025
@lunny lunny closed this Jun 12, 2025
@GiteaBot GiteaBot removed this from the 1.25.0 milestone Jun 12, 2025
@lunny lunny reopened this Jun 12, 2025
@lunny lunny force-pushed the lunny/catfile_batch_refactor branch from f0b6480 to 334eb9e Compare June 18, 2025 17:15
@lunny lunny added this to the 1.25.0 milestone Jun 18, 2025
@wxiaoguang wxiaoguang removed this from the 1.25.0 milestone Aug 29, 2025
@lunny lunny added this to the 1.26.0 milestone Sep 29, 2025
@GiteaBot GiteaBot removed the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 20, 2025
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Oct 20, 2025
Comment on lines 188 to 193
// ToTrustCmdArg converts a string (trusted as argument) to CmdArg
// In most cases, it shouldn't be used. Use AddXxx function instead
func ToTrustCmdArg(arg string) internal.CmdArg {
return internal.CmdArg(arg)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you don't need it.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed at db4b7e4

Comment on lines +155 to +159
func NewBatch(ctx context.Context, repoPath string) (Batch, error) {
if DefaultFeatures().SupportCatFileBatchCommand {
return newBatchCommandCatFile(ctx, repoPath)
}
return newBatchCatFileWithCheck(ctx, repoPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it is not the first time that I told you that you must have correct tests

Copy link
Member Author

Choose a reason for hiding this comment

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

Tests added at db4b7e4

@wxiaoguang wxiaoguang marked this pull request as draft October 20, 2025 23:33
@@ -0,0 +1 @@
ref: refs/heads/master
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why it needs a new git repo for testing purpose. There are already plenty.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for lfs test. There is no test repository for lfs under git/tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why it must be under git/tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed at 87e12a1

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

Labels

lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/go Pull requests that update Go code performance/cpu 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