Skip to content

Conversation

@nlisker
Copy link
Collaborator

@nlisker nlisker commented Aug 18, 2022

Fixes the mistakes in the JBS ticket and some additional minor corrections.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/877/head:pull/877
$ git checkout pull/877

Update a local copy of the PR:
$ git checkout pull/877
$ git pull https://git.openjdk.org/jfx pull/877/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 877

View PR using the GUI difftool:
$ git pr show -t 877

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/877.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 18, 2022

👋 Welcome back nlisker! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@nlisker nlisker changed the base branch from master to jfx19 August 18, 2022 19:11
@openjdk openjdk bot added the rfr Ready for review label Aug 18, 2022
@mlbridge
Copy link

mlbridge bot commented Aug 18, 2022

Webrevs

@andy-goryachev-oracle
Copy link
Contributor

Nir, have you sync'ed your branch with the latest master? There are 5000+ files changed listed.

@nlisker
Copy link
Collaborator Author

nlisker commented Aug 18, 2022

It's a PR against the javafx19 branch. I think I forked the master branch though, which is why it is showing all the commits. I might need to rebase, but if the changes are approved it should integrate without issues.

@andy-goryachev-oracle
Copy link
Contributor

you probably need to rebase. impossible to review.

@kevinrushforth
Copy link
Member

kevinrushforth commented Aug 18, 2022

It's a PR against the javafx19 branch. I think I forked the master branch though, which is why it is showing all the commits.

Yeah, this is a fairly easy mistake to make. It's also very obvious when it happens.

I might need to rebase, but if the changes are approved it should integrate without issues.

You will definitely need to rebase and force push before this can be reviewed. No, it otherwise won't integrate without issues.

/reviewers 2

@openjdk
Copy link

openjdk bot commented Aug 18, 2022

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

@kevinrushforth
Copy link
Member

I'll review it.

@arapte or @aghaisas Can one of you be the second reviewer?

kevinrushforth and others added 16 commits August 19, 2022 02:06
8288449: Update attribution in glib.md file

Reviewed-by: kcr, angorya
…m a Scene/SubScene

Reviewed-by: jhendrikx, aghaisas, mhanl
…omparison. A condition expression should not be reduced to an assignment

Reviewed-by: kcr
@nlisker nlisker force-pushed the 8286678_Fix_mistakes_in_FX_API_docs branch from c4b2bcd to 263061e Compare August 18, 2022 23:12
@nlisker
Copy link
Collaborator Author

nlisker commented Aug 18, 2022

Looks like the rebase missed a few commits. Not sure why.

@kevinrushforth
Copy link
Member

How did you do the rebase? It should be something like:

git fetch upstream
git rebase -i upstream/master
<remove all commits that came from the master branch>

Alternatively, you can git reset --hard upstream/jfx19 to reset your branch, and then cherry pick all of the commits (or apply the aggregate diffs as one commit).

@kevinrushforth
Copy link
Member

Oh, there is only a single unique commit in your branch (or rather there was before you rebased some of the commits from master), so the following should work:

git reset --hard upstream/jfx19
git cherry-pick 263061e366e5c7b4d0928b33d7bc4da90b9fa519

Check that there is only the one commit, and that the diffs look good:

git log upstream/jfx19..
git diff upstream/jfx19..

then force push.

@nlisker nlisker force-pushed the 8286678_Fix_mistakes_in_FX_API_docs branch 2 times, most recently from 5d0fbde to 263061e Compare August 18, 2022 23:45
@nlisker
Copy link
Collaborator Author

nlisker commented Aug 18, 2022

git reset --hard upstream/jfx19
That gives an error

fatal: ambiguous argument 'upstream/jfx19': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

I will close this PR and resubmit.

@nlisker nlisker closed this Aug 18, 2022
@nlisker nlisker deleted the 8286678_Fix_mistakes_in_FX_API_docs branch August 18, 2022 23:50
@nlisker
Copy link
Collaborator Author

nlisker commented Aug 18, 2022

#880 is the new PR.

@kevinrushforth
Copy link
Member

fatal: ambiguous argument 'upstream/jfx19': unknown revision or path not in the working tree.

I should have said "presuming you have a remote configured with the name upstream". Anyway, what you did was good.

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

Labels

rfr Ready for review

Development

Successfully merging this pull request may close these issues.

4 participants