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

fix Issue 20893 - [REG 2.087] 32-bit arithmetic goes wrong #11230

Merged
merged 1 commit into from
Jun 5, 2020

Conversation

WalterBright
Copy link
Member

Fix refactoring mistake in #9722

It's an ugly problem, and should be a priority.

@WalterBright WalterBright added Severity:Regression PRs that fix regressions Review:stable-priority A regression fix to stable that should receive higher priority labels Jun 5, 2020
@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 5, 2020

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
20893 regression [REG 2.087] 32-bit arithmetic goes wrong

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + dmd#11230"

@thewilsonator
Copy link
Contributor

Err, stable...

@WalterBright WalterBright changed the base branch from master to stable June 5, 2020 08:24
@WalterBright
Copy link
Member Author

Err, stable...

I follow the directions here exactly. Result is the mess you see :-(

mercury~/forks/dmd fix20893> git rebase --onto upstream/stable upstream/master
First, rewinding head to replay your work on top of it...
Applying: fix Issue 20893 - [REG 2.087] 32-bit arithmetic goes wrong

Then I do:
https://github.blog/2016-08-15-change-the-base-branch-of-a-pull-request/

Result: Mess.

So I try:

git push -f origin stable

which does not help.

I figure I'm missing some obvious, but omitted, step?

@MoonlightSentinel
Copy link
Contributor

git push -f origin stable

Pushing stable should be unnecessary, your PR uses the fix20893 branch, Did you push that branch as well?

@WalterBright
Copy link
Member Author

Pushing stable should be unnecessary, your PR uses the fix20893 branch, Did you push that branch as well?

I followed the directions presented here exactly. It did not say to do anything else like push that branch.

@Geod24
Copy link
Member

Geod24 commented Jun 5, 2020

Are your remote up to date ? (git remote update -p, I run it so often I have an alias for this).
And as @MoonlightSentinel pointed, pushing to your stable won't help. The syntax is: git push [-f] $NAME_OF_REMOTE $LOCAL_REF[:$REMOTE_BRANCH].

Let me fix that for you.

@WalterBright
Copy link
Member Author

This stuff desperately needs to be added to the directions. The directions should be complete.

@WalterBright
Copy link
Member Author

Another question:

is origin my fork, or is upstream my fork?

@Geod24
Copy link
Member

Geod24 commented Jun 5, 2020

Here you go. For reference, when git rebase is uncooperative, you can also do:

$ git checkout upstream/stable
$ git branch -D fix20893 # Make sure you know the commit before, or `man git-reflog`
$ git checkout -b fix20893 # Gives you a fresh branch off `stable`
$ git cherry-pick 72f110d # The original commit ID

In my experience this is easier for people that struggle with rebase. But really, read this.

EDIT: To pick up those change: git remote update && git branch -D fix20893 && git checkout fix20893 is the easiest (basically delete your local branch, and re-create it based on the branch in origin).

@thewilsonator
Copy link
Contributor

run git remote -v it will list all the local names for external repository.

@Geod24
Copy link
Member

Geod24 commented Jun 5, 2020

@WalterBright : git branch -v (EDIT: Oops, git remote -v) will tell you which is which. I suspect origin is your fork, upstream is dlang/dmd, as it is the convention (but again, there's nothing preventing you from using different names if you wanted to).

@WalterBright
Copy link
Member Author

I don't want to use different names, I want to use the usual conventions as long as I haven't mastered git.

git branch -v does not tell me what origin and upstream are, though git remote -v does. Also, why do your instructions not include git fetch?

Now, when I do git remote -v I get:

git remote -v
origin  git@github.com:WalterBright/dmd.git (fetch)
origin  git@github.com:WalterBright/dmd.git (push)
upstream        git@github.com:D-Programming-Language/dmd (fetch)
upstream        git@github.com:D-Programming-Language/dmd (push)

Notice the first is dmd.git and the second is just dmd. Why?

@WalterBright
Copy link
Member Author

When would I do git remote update vs git fetch upstream stable?

@WalterBright
Copy link
Member Author

Anyhow, thanks for the info and fixing this PR. Please fix the "rebase to stable" directions.

@Geod24
Copy link
Member

Geod24 commented Jun 5, 2020

git branch -v does not tell me what origin and upstream are, though git remote -v does.

My bad, I don't know why I wrote branch here... It should have been remote as you realized.

Also, why do your instructions not include git fetch?

git fetch just fetches either origin or the upstream of the current branch. git fetch -a will fetch all remotes. git remote update is pretty much the same as git fetch -a. git remote update -p (--prune) will also remove references to remote branches that have been removed.

Notice the first is dmd.git and the second is just dmd. Why?

I'm not sure, but I guess Github changed their remote endpoints at some point. You can do git remote set-url upstream git@github.com:dlang/dmd.git to change it to the "latest" address.

@WalterBright
Copy link
Member Author

@Geod24 thanks!

@dlang-bot dlang-bot merged commit ec9bc54 into dlang:stable Jun 5, 2020
@WalterBright WalterBright deleted the fix20893 branch June 6, 2020 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:auto-merge Review:stable-priority A regression fix to stable that should receive higher priority Severity:Bug Fix Severity:Regression PRs that fix regressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants