[core] Don't force a remote when listing prettier changes

[Janpot]

My master branch is tracking upstream/master, where upstream points to https://github.com/mui-org/material-ui.git
My origin/master is so far behind it breaks the yarn prettier command for me (stdout overflow).
I propose to remove explicitly specifying the remote.
Like in the script this one is based on: https://github.com/facebook/react/blob/b87aabdfe1b7461e7331abb3601d9e6bb27544bc/scripts/shared/listChangedFiles.js#L29

• I have followed (at least) the PR section of the contributing guide.

[mui-pr-bot]

No bundle size changes comparing ad6fe1b...4821a88

Generated by 🚫 dangerJS against 4821a88

[oliviertassinari]

I have the same git configuration as you. It performs better this way, thank you for the fix!

[Janpot]

@oliviertassinari Are there any guidelines written down around which labels to use like [core]?

[oliviertassinari]

@Janpot There isn't. From my perspective, core is about internal changes, that won't directly benefit users.

[eps1lon]

This won't work for CI. In CI branches are usually hard reset which means it can't pick up the proper merge base.

At least this was the case when we first introduced it. Either we change it to upstream/master or we debug this first e.g. log the changed files and see if the CI picks this file up properly.

My origin/master is so far behind it breaks the yarn prettier

Why not update it? You should do this either way so your branch is as close to master as possible. The farther behind your branch the more likely you encounter merge conflicts when opening a PR. I usually update my origin once or twice a week:

$ git checkout master
$ git pull upstream master
$ git push origin master

Since I run prettier in a pre-push hook I quickly notice when my local master is too far behin.

[Janpot]

@eps1lon

This won't work for CI. In CI branches are usually hard reset which means it can't pick up the proper merge base.

Why don't we verify this? I think we can just verify the output? I don't have access to it, but it seems to have run successfully. It also runs successfully in react CI. I'm also not aware of hard reset messing with tracking branches.

Why not update it?

Because I don't use it, since my master branch is tracking upstream/master and not origin/master, I'm always perfectly up to date. My origin/master is a year and a half behind. keeping origin/master in sync is just unnecessary ceremony, I will never use it for anything.

[oliviertassinari]

In my local env, it looks like this (4 months old 🤷‍♂️)

$ git show origin/master

commit 783b693 (origin/master)
Author: el que m'est 14348482+elquimista@users.noreply.github.com
Date: Sat Jul 13 03:20:13 2019 -0700

[Janpot]

$ git show origin/master
  commit edde6ad19ecc59a7166ec2ce756a84b691df9aac
  Author: Olivier Tassinari <...>
  Date:   Sat May 4 00:17:47 2019 +0200

Not a year and a half, I seem to have synced it somewhere in the meantime 🙂

[eps1lon]

Why don't we verify this?

What should we verify?

but it seems to have run successfully.

If the list of files is empty prettier can never complain.

[Janpot]

What should we verify?

The build output of the prettier command in Circle CI. It should be something like

Checking scripts/listChangedFiles.js

Only, I don't have access to your Circle CI 🤷‍♂.

In the meantime I removed a semicolon somewhere so we can verify whether it breaks the build or not. There isn't much more I can do without access to Circle CI.

[Janpot]

@eps1lon How do you feel about detecting CI and picking a branch based on that? Maybe ci-info could be used?

[Janpot]

Maybe I should add a unit test for listChangedFiles, so that we get notified when/if this breaks in CI?

[eps1lon]

@eps1lon How do you feel about detecting CI and picking a branch based on that? Maybe ci-info could be used?

I'm open to any improvement.

I think doing a full check on master and changed-only in a PR should be good enough to detect breakage.

Only, I don't have access to your Circle CI 🤷‍♂.

What elevated permissions do you need and why? https://circleci.com/gh/mui-org/material-ui/131034 should be readable by anyone.

Looking at the output the new behavior is not correct. It runs on unrelated files and not the ones introduced in this PR.

[Janpot]

Oh ok, I was clicking the details links in the github checks ui => https://app.circleci.com/jobs/github/mui-org/material-ui/131034
They seem to be broken. seems to be only happening on desktop. clearing cache/cookies and logging out/in again seems to have solved it now.

[Janpot]

Looking at the output the new behavior is not correct. It runs on unrelated files and not the ones introduced in this PR.

@eps1lon Looks like it was just a bit behind on master, after merging master in this branch, the expected files are checked: https://circleci.com/gh/mui-org/material-ui/131312
I added a test to makes sure things won't break in CI while still working locally.

[eps1lon]

Looks like it was just a bit behind on master

This shouldn't be required. Otherwise maintainers would need to check if the branch is behind. It should just run on the files listed in "files changed".

[Janpot]

This shouldn't be required. Otherwise maintainers would need to check if the branch is behind. It should just run on the files listed in "files changed".

It's not required, it's just a few unchanged files that are checked as well on top of the changed ones. That is not a problem, and it already behaves like that without my PR. If anything, I'd say keeping your branch up to date with master is good practice in most cases.

Specifically, it's this commit #18780 that was merged in master after the commit where I started this branch.

[eps1lon]

It's not required, it's just a few unchanged files that are checked as well on top of the changed ones.

Let's see if this causes confusion later.

[eps1lon]

@Janpot Thanks for working on this!