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

symbolize_unittest: make it a bit more portable #985

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

vapier
Copy link
Member

@vapier vapier commented Dec 9, 2023

No description provided.

@vapier
Copy link
Member Author

vapier commented Dec 9, 2023

rebase of #87

@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7d973f9) 63.92% compared to head (76c2c47) 64.19%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #985      +/-   ##
==========================================
+ Coverage   63.92%   64.19%   +0.27%     
==========================================
  Files          17       17              
  Lines        3326     3326              
  Branches     1125     1125              
==========================================
+ Hits         2126     2135       +9     
+ Misses        774      767       -7     
+ Partials      426      424       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sergiud
Copy link
Collaborator

sergiud commented Dec 20, 2023

Thanks for the PR.

Could you please rebase and push the commit again as I no longer seem to have access to your branch after force pushing.

@vapier
Copy link
Member Author

vapier commented Dec 20, 2023

pretty sure you can fetch the commit you clobbered from the remote

$ git fetch https://github.com/vapier/glog 233df366d56df9d3d32898451f2ba1ace7e234de
$ git cherry-pick FETCH_HEAD

@sergiud
Copy link
Collaborator

sergiud commented Dec 20, 2023

That's not the issue. I simply can't push to your master since the branch no longer contains the PR commit.

For future reference, please submit your changes from a dedicated feature branch to avoid confusion that local and remote branches with the same name can cause.

@vapier
Copy link
Member Author

vapier commented Dec 20, 2023

yeah, that's not really a thing. don't force push if you don't want to clobber/rewrite a ref.

i can rebase the original commit onto the latest branch & push to my repo again. but i can't re-open PR's in this project. i can only file new ones.

@sergiud sergiud reopened this Dec 20, 2023
@sergiud
Copy link
Collaborator

sergiud commented Dec 20, 2023

yeah, that's not really a thing. don't force push if you don't want to clobber/rewrite a ref.

What is not a thing? Force pushing is perfectly fine and necessary when rebasing which is what I did. I simply messed up the local ref when pushing.

This works now. Thanks.

@sergiud sergiud added this to the 0.7 milestone Dec 20, 2023
@vapier
Copy link
Member Author

vapier commented Dec 20, 2023

requiring people use non-default branches to generate PRs to somehow protect against bad force pushes. you force pushed the wrong state to a remote branch. giving it a diff name wouldn't have helped -- you would have typed the remote branch in order to rewrite it.

but whatever, let's just agree to disagree and move on.

@sergiud sergiud merged commit 6607b36 into google:master Dec 20, 2023
269 checks passed
@sergiud
Copy link
Collaborator

sergiud commented Dec 20, 2023

That's fine. I'm eager to learn.

giving it a diff name wouldn't have helped

Why not? Compare

git push [email protected]:vapier/glog.git master:master # whoops

vs

git push [email protected]:vapier/glog.git portable-labels

If you have a better suggestion how to avoid such a mishap, please let me know.

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

Successfully merging this pull request may close these issues.

3 participants