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

Update next to be current with master #13913

Merged
merged 97 commits into from
Dec 15, 2018
Merged

Update next to be current with master #13913

merged 97 commits into from
Dec 15, 2018

Conversation

pelotom
Copy link
Member

@pelotom pelotom commented Dec 15, 2018

No description provided.

oliviertassinari and others added 30 commits November 21, 2018 19:51
* Timeout props

We could just use the component's timeout props. Tried it myself and it worked :)

* [Grow] Condense the demo
* [docs] Fix theme menu link

* better hash url generator
* [Tooltip] Fix the property forwarding priority

* review
* Change &quote; to '

* fix all the occurrences
* [Input, InputBase] fix propTypes of value and defaultValue

* add a test
* [Dialog] Add xl maxWidth and demo component

* let's merge
…ui#13686)

* [docs] Add support for changing react version in codesandbox demos
[docs] Change styles Hook API codesandbox to use next version of react and react-dom

* [docs] Change react version for Adapting hook API codesandbox demo

* add the next flag to all the demos in case of
* ignore touchmove after touchend

* only listen to touchmove if touch enabled

* test add fireBodyMouseEvent helper
* [docs] Add notistack Snackbar live deme in complementary projects

* Fix peer dependecy issue with @material-ui/core

* let's merge

* add a comment about what next-plugin-transpile-modules is doing
)

* [Tooltip] Hide native title when disableHoverListener is true

* add a comment
Closes mui#13718 

* [styles] Add options definitions for makeStyles

* [styles] Improve makeStyles typings test coverage
* Add icon to InputBaseClassKey types

* [InputBase] Remove dead disableUnderline property
* [InputBase] Remove props capturing of disableUnderline

* [FilledInput] Add conditional logic to remove underline in filled input

* Update FilledInput.js

* Update FilledInput.js

* let's merge
@pelotom
Copy link
Member Author

pelotom commented Dec 15, 2018

I see. So I’ll just merge and push from the cli.

@pelotom pelotom merged commit 7c074a7 into mui:next Dec 15, 2018
@pelotom pelotom deleted the update-next branch December 15, 2018 17:16
@eps1lon
Copy link
Member

eps1lon commented Dec 20, 2018

So this is now a single commit in next. What happens when we release v4 and merge next into master?

@oliviertassinari
Copy link
Member

@eps1lon Last time I have checked, @pelotom merged the pull-request then push forced on next so we keep all our contribution history.

@pelotom
Copy link
Member Author

pelotom commented Dec 20, 2018

@eps1lon Last time I have checked, @pelotom merged the pull-request then push forced on next so we keep all our contribution history.

I just fast-forward merged from master on the command line and pushed, no force needed 🙂

@eps1lon
Copy link
Member

eps1lon commented Dec 20, 2018

$ git checkout next
$ git pull upstream/next
$ git merge --ff-only upstream/master
fatal: Not possible to fast-forward, aborting.

What am I missing here?

@pelotom
Copy link
Member Author

pelotom commented Dec 20, 2018

Looks like 2 days ago #13923 was merged targeting next, was that supposed to target master?

@oliviertassinari
Copy link
Member

No, it wasn't!

@pelotom
Copy link
Member Author

pelotom commented Dec 20, 2018

Ok, so next has diverged, but not due to a squashed merge from master, so it's all good right?

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 20, 2018

I'm cherry-picking the commit on master. I still think that we can push force for this time.

@pelotom
Copy link
Member Author

pelotom commented Dec 20, 2018

@oliviertassinari I'm confused, you said #13923 wasn't supposed to target master, did you mean the opposite?

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 20, 2018

@pelotom The pull request was supposed to target master. Well spotted.

@eps1lon
Copy link
Member

eps1lon commented Jan 11, 2019

@oliviertassinari Where was the change included in master? Looks like the commit is still only on next. A cherry-pick would've been the easiest solution that would've reconciled the two branches too.

@eps1lon
Copy link
Member

eps1lon commented Jan 11, 2019

Original squashed commit that was merged into next: 6759f7c
Commit in master: 91a95d3

Looks like you picked the commit from the PR branch not the squashed commit. The commit on master is missing the PR link so I would still include both commits. It's duplication but I'd prefer the PR link over no duplication.

@oliviertassinari
Copy link
Member

@eps1lon next and master should be in sync. I think that we should do:

git checkout master
git fetch --all --prune
git checkout next
git reset --h master
git push --force

@eps1lon
Copy link
Member

eps1lon commented Jan 11, 2019

@eps1lon next and master should be in sync.

You state this like it's obvious what this means. They currently have one different commit each that results in the same diff but a different commit message.

You are now proposing to remove the commit from next and therefore the link to the PR. Since you explained this in detail and not just simply said no to my proposal I just want to confirm that we're on the same page.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 11, 2019

The above command lines were a suggestion, the simplest option I can think of. My point is that we haven't yet merged any pull request intentionnaly on the next branch. So the next branch should be an anchestor of the master branch. It's not the case as we are speaking. Regarding the pull request that was merged on the wrong branch. I have cherrypick it on master at some point.

@eps1lon
Copy link
Member

eps1lon commented Jan 11, 2019

Ok so we're not on the same page. Again I direct you to #13913 (comment) where I proofed that the commit you cherry-picked is not the one from next which caused next and master to diverge. This is the current state:
mui-next-master

Now you still didn't explicitly addressed my proposal. In order to "sync" next and master I proposed that we include the duplicate commit from master into next because the commit in next has more information than the commit in master. It results in some duplication but also additional information.

Your suggestion would result in a loss of information. The commit that included the PR link in the commit message would disappear. The patch is still there because you cherry-picked a different commit but the commit message would be different. Could you acknowledge that because I want to be very clear if we remove information from our commit history.

@oliviertassinari
Copy link
Member

I proofed that the commit you cherry-picked is not the one from next which caused next and master to diverge. This is the current state:

Yes, it's not the same one. My mistake.

@eps1lon
Copy link
Member

eps1lon commented Jan 11, 2019

Yes, it's not the same one. My mistake.

It was definitely not my intention to get you to admit a mistake. Obviously git is very hard and I don't seem to know how to do it properly either. Just want to make sure that we do recognize that we will loose the link to the PR and we're willing to make that tradeoff to have a cleaner commit history.

Otherwise we can simply rebase master into next instead of resetting next to master.

@oliviertassinari
Copy link
Member

@eps1lon As you see fit, I have tried the merge from master into next. It looks fine too.

@oliviertassinari
Copy link
Member

I have tried merging next into master. It looks even better.

@oliviertassinari
Copy link
Member

We can merge next into master. Then reset hard next on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.