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

Skip the content when object is unexpected in git.Repository.getTree(no go-git) #29253

Closed
wants to merge 1 commit into from

Conversation

yp05327
Copy link
Contributor

@yp05327 yp05327 commented Feb 19, 2024

Fix #29101

cat-file --batch output format:

<sha> SP <type> SP <size> LF\n<content>

When object type is expected, the content will be handled, but when it is not expected, the content will not be handled, and will be read in the next turn.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 19, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 19, 2024
@yp05327 yp05327 changed the title Skip blob content in getTree Skip the content when object is not a tree Feb 19, 2024
@yp05327 yp05327 requested a review from KN4CK3R February 19, 2024 02:25
@lunny
Copy link
Member

lunny commented Feb 19, 2024

Can you add a test?

@yp05327
Copy link
Contributor Author

yp05327 commented Feb 19, 2024

Can you add a test?

I will try, but it occurs randomly, need to consider how to test it correctly. 🤔
Have no good idea now.

@yp05327 yp05327 changed the title Skip the content when object is not a tree Skip the content when object is unexpected in git.Repository.getTree(no go-git) Feb 19, 2024
@yp05327
Copy link
Contributor Author

yp05327 commented Feb 19, 2024

The test got random results. Maybe we need to dig deeper to find out the real reason. Or it is hard to add such a test for this issue.
image
image

@KN4CK3R
Copy link
Member

KN4CK3R commented Feb 19, 2024

My fix from #29104 prevented the error too but doesn't fix the real problem. Should be the same for this fix. Gave up at that point then. I may have a look at this again later.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 19, 2024
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Mark as "request changes":

  • The root cause should be figured out
  • It needs tests and comments

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 19, 2024
@KN4CK3R
Copy link
Member

KN4CK3R commented Feb 19, 2024

@yp05327 Are you fine with it if I would create a new PR containing your commit because there are other places too which should be changed. Otherwise we can merge your PR and I create a follow up PR.

@yp05327
Copy link
Contributor Author

yp05327 commented Feb 20, 2024

@yp05327 Are you fine with it if I would create a new PR containing your commit because there are other places too which should be changed. Otherwise we can merge your PR and I create a follow up PR.

Never mind. I will close this one.

@yp05327 yp05327 closed this Feb 20, 2024
Copy link

Automatically locked because of our CONTRIBUTING guidelines

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 28, 2024
@yp05327 yp05327 deleted the fix-29101 branch May 27, 2024 00:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid output in batch_reader.go
6 participants