Handle non-existent upstream master branches in `x fmt` #106415

Noratrieb · 2023-01-03T17:57:48Z

People who do have a remote for rust-lang/rust but don't have the master branch checked out there used to get this error when running x fmt:

fatal: ambiguous argument 'rust/master': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git [...] -- [...]'
rust/master

Which is not exactly helpful.

Now, we fall back to origin/master (hoping that at least that remote exists) for that case. If there is still some error, we just fall back to x fmt . and print a warning.

r? @jyn514

Mark-Simulacrum · 2023-01-03T18:04:00Z

I don't think we should fallback on an arbitrary remote, if your origin is way behind the current HEAD we'll do a bunch of work trying to figure out what the right delta is, but really just formatting all files in that case seems better to me.

I don't think we should allow for our heuristics being wrong by using arbitrary remotes.

jyn514 · 2023-01-03T18:29:32Z

@Mark-Simulacrum we already hardcode origin for download-ci-llvm, which IMO is a lot more important to get correct. I'm not sure why we would hold formatting to a higher standard (especially since it's already going to be caught in CI by x fmt --check).

Mark-Simulacrum · 2023-01-03T18:39:12Z

Hm, that seems like a problem there - I would probably not do that. But anyway, will leave it up to you.

jyn514

Hm, that seems like a problem there - I would probably not do that. But anyway, will leave it up to you.

Yeah it's a little unfortunate. But people have been using it for 2+ years and no one has complained, so I think it's ok in practice.

r=me with the nit fixed

src/tools/build_helper/src/git.rs

Noratrieb · 2023-01-03T18:45:25Z

I agree that hardcoding remotes isn't a great idea, but I think it's fine here. This path will only be hit on fairly nonstandard setups that don't have the upstream master fetched, and also if it fails it will just be a warning and it will work just fine (albeit a little slower).

Noratrieb · 2023-01-06T20:01:24Z

@bors r=jyn514

bors · 2023-01-06T20:01:26Z

📌 Commit d5e5762 has been approved by jyn514

It is now in the queue for this repository.

bors · 2023-01-06T21:51:48Z

⌛ Testing commit d5e5762 with merge 7ac9572...

bors · 2023-01-07T00:53:14Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 7ac9572 to master...

rust-timer · 2023-01-07T02:12:30Z

Finished benchmarking commit (7ac9572): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.6%	[0.6%, 0.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.4%	[-1.4%, -1.4%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.2%	[-1.5%, -1.0%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.2%	[-1.5%, -1.0%]	3

RalfJung · 2023-09-01T06:57:13Z

src/bootstrap/format.rs

+                            Falling back to formatting all files."
+                        );
+                        // Something went wrong when getting the version. Just format all the files.
+                        paths.push(".".into());


I've stared at this for quite a while now, but I don't think I get it... why does this push . to the path list? We have other error cases below ("could not find usable git"), and they don't do that. Why is this different? I think for ./x.py fmt it is unnecessary since we're anyway searching everything, and for ./x.py fmt compiler/ this s actively buggy since it formats not just the files in the compiler dir. Am I missing something?

you're probably right. i don't remember the details of this change though, but this does look sus

…ulacrum bootstrap/format: remove unnecessary paths.push Cc rust-lang#106415 (review) I verified that this still formats all fileds when `get_modified_rs_files` is made to return an error. r? `@Nilstrieb`

…ulacrum bootstrap/format: remove unnecessary paths.push Cc rust-lang#106415 (review) I verified that this still formats all fileds when `get_modified_rs_files` is made to return an error. r? ``@Nilstrieb``

Rollup merge of rust-lang#115440 - RalfJung:bootstrap-fmt, r=Mark-Simulacrum bootstrap/format: remove unnecessary paths.push Cc rust-lang#106415 (review) I verified that this still formats all fileds when `get_modified_rs_files` is made to return an error. r? ``@Nilstrieb``

Add build_helper crate to share code between tidy and bootstrap

25c1531

rustbot assigned jyn514 Jan 3, 2023

rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 3, 2023

jyn514 reviewed Jan 3, 2023

View reviewed changes

src/tools/build_helper/src/git.rs Outdated Show resolved Hide resolved

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 3, 2023

Handle non-existant upstream master branches in x fmt

d5e5762

Noratrieb force-pushed the where-is-my-master-branch branch from 21947c9 to d5e5762 Compare January 6, 2023 19:19

Noratrieb changed the title ~~Handle non-existant upstream master branches in x fmt~~ Handle non-existent upstream master branches in x fmt Jan 6, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 6, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 7, 2023

bors merged commit 7ac9572 into rust-lang:master Jan 7, 2023

rustbot added this to the 1.68.0 milestone Jan 7, 2023

bors mentioned this pull request Jan 7, 2023

Create "suggested tests" tool in rustbuild #106249

Merged

Noratrieb deleted the where-is-my-master-branch branch January 7, 2023 08:17

RalfJung reviewed Sep 1, 2023

View reviewed changes

RalfJung mentioned this pull request Sep 1, 2023

bootstrap/format: remove unnecessary paths.push #115440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle non-existent upstream master branches in `x fmt` #106415

Handle non-existent upstream master branches in `x fmt` #106415

Noratrieb commented Jan 3, 2023

Mark-Simulacrum commented Jan 3, 2023

jyn514 commented Jan 3, 2023 •

edited

Loading

Mark-Simulacrum commented Jan 3, 2023

jyn514 left a comment

Noratrieb commented Jan 3, 2023

Noratrieb commented Jan 6, 2023

bors commented Jan 6, 2023

bors commented Jan 6, 2023

bors commented Jan 7, 2023

rust-timer commented Jan 7, 2023

RalfJung Sep 1, 2023 •

edited

Loading

Noratrieb Sep 1, 2023

Handle non-existent upstream master branches in x fmt #106415

Handle non-existent upstream master branches in x fmt #106415

Conversation

Noratrieb commented Jan 3, 2023

Mark-Simulacrum commented Jan 3, 2023

jyn514 commented Jan 3, 2023 • edited Loading

Mark-Simulacrum commented Jan 3, 2023

jyn514 left a comment

Choose a reason for hiding this comment

Noratrieb commented Jan 3, 2023

Noratrieb commented Jan 6, 2023

bors commented Jan 6, 2023

bors commented Jan 6, 2023

bors commented Jan 7, 2023

rust-timer commented Jan 7, 2023

Overall result: ❌✅ regressions and improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

RalfJung Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Noratrieb Sep 1, 2023

Choose a reason for hiding this comment

Handle non-existent upstream master branches in `x fmt` #106415

Handle non-existent upstream master branches in `x fmt` #106415

jyn514 commented Jan 3, 2023 •

edited

Loading

RalfJung Sep 1, 2023 •

edited

Loading