run: Teach run to resolve revsets and about jobs. #2486

PhilipMetzger · 2023-10-31T21:44:21Z

This also adds jobs, the argument reading the thread count to use and shell_command.
While we're at it, make execute a no-op and teach run to resolve the passed revsets.
I also fixed my misunderstanding of Clap which makes
jj run 'echo hello world' -r 'mine() & ~origin@remote' --jobs 4 parse correctly.

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added tests to cover my changes

martinvonz

It would be nice to have the move separated from the other changes, so I didn't have to scroll back and forth to visually compared the old and new version. This is fine for this time, however.

cli/src/commands/mod.rs

arxanas · 2023-10-31T22:42:15Z

It's best to separate code moves from code changes where possible, because then source control (and the human readers) can track the code moves better.

cli/src/commands/run.rs

PhilipMetzger · 2023-11-02T19:15:51Z

Sorry for the many pushes on this branch, it seems that I couldn't write good commit messages yesterday.

ilyagr · 2023-11-03T03:13:54Z

Sorry for the many pushes on this branch, it seems that I couldn't write good commit messages yesterday.

This is every day for me, or at least every day I have a PR :)

I hope it is OK; I'm certainly having trouble avoiding numerous pushes.

cli/src/commands/run.rs

This also adds `jobs`, the argument reading the thread count to use and `shell_command`. While we're at it, make `execute` a no-op and teach `run` to resolve the passed revsets. I also fixed my misunderstanding of `Clap` which makes `jj run 'echo hello world' -r 'mine() & ~origin@remote' --jobs 4` parse correctly. Also contains a small fix in the `pre-commit` example for it.

arxanas

Sorry, failed to comment in time.

cli/src/commands/run.rs

Adress the post-merge comments from #2486, which found a doc comment inaccurate and to not blindly ignore `-j 0` which would've worked until now. I've also reduced the default `jobs` size to two, as most cheap consumer laptops don't have four cores. Thanks to @necaqua the controversial `unsafe` usage was already removed. I've omitted to change `revisions` from an Vec to a RevisonArg for the moment, as I will keep working on the file anyway.

@necauqua

Adress the post-merge comments from #2486, which found a doc comment inaccurate and to not blindly ignore `-j 0` which would've worked until now. I've also reduced the default `jobs` size to two, as most cheap consumer laptops don't have four cores. Thanks to @necauqua the controversial `unsafe` usage was already removed. I've omitted to change `revisions` from an Vec to a RevisonArg for the moment, as I will keep working on the file anyway.

@necauqua

Adress the post-merge comments from #2486, which found a doc comment inaccurate and to not blindly ignore `-j 0` which would've worked until now. I've also reduced the default `jobs` size to two, as most cheap consumer laptops don't have four cores. Thanks to @necauqua the controversial `unsafe` usage was already removed. I've omitted to change `revisions` from an Vec to a RevisonArg for the moment, as I will keep working on the file anyway.

@necauqua

Adress the post-merge comments from #2486, which found a doc comment inaccurate and to not blindly ignore `-j 0` which would've worked until now. I've also reduced the default `jobs` size to one, as it's user-visible configuration which determines how many processes should run. Thanks to @necauqua the controversial `unsafe` usage was already removed. I've omitted to change `revisions` from an Vec to a RevisonArg for the moment, as I will keep working on the file anyway.

@necauqua

Adress the post-merge comments from #2486, which found a doc comment inaccurate and to not blindly ignore `-j 0` which would've worked until now. I've also reduced the default `jobs` size to one, as it's user-visible configuration which determines how many processes should run. Thanks to @necauqua the controversial `unsafe` usage was already removed. I've omitted to change `revisions` from an Vec to a RevisonArg for the moment, as I will keep working on the file anyway.

@necauqua

Adress the post-merge comments from #2486, which found a doc comment inaccurate and to not blindly ignore `-j 0` which would've worked until now. I've also reduced the default `jobs` size to one, as it's user-visible configuration which determines how many processes should run. Thanks to @necauqua the controversial `unsafe` usage was already removed. I've omitted to change `revisions` from an Vec to a RevisonArg for the moment, as I will keep working on the file anyway.

@necauqua

Adress the post-merge comments from #2486, which found a doc comment inaccurate and to not blindly ignore `-j 0` which would've worked until now. I've also reduced the default `jobs` size to one, as it's user-visible configuration which determines how many processes should run. Thanks to @necauqua the controversial `unsafe` usage was already removed. I've omitted to change `revisions` from an Vec to a RevisonArg for the moment, as I will keep working on the file anyway.

@necauqua

Adress the post-merge comments from #2486, which found a doc comment inaccurate and to not blindly ignore `-j 0` which would've worked until now. I've also reduced the default `jobs` size to one, as it's user-visible configuration which determines how many processes should run. Thanks to @necauqua the controversial `unsafe` usage was already removed. I've omitted to change `revisions` from an Vec to a RevisonArg for the moment, as I will keep working on the file anyway.

@necauqua

Adress the post-merge comments from #2486, which found a doc comment inaccurate and to not blindly ignore `-j 0` which would've worked until now. I've also reduced the default `jobs` size to one, as it's user-visible configuration which determines how many processes should run. Thanks to @necauqua the controversial `unsafe` usage was already removed. I've omitted to change `revisions` from an Vec to a RevisonArg for the moment, as I will keep working on the file anyway.

@necauqua

Adress the post-merge comments from #2486, which found a doc comment inaccurate and to not blindly ignore `-j 0` which would've worked until now. I've also reduced the default `jobs` size to one, as it's user-visible configuration which determines how many processes should run. Thanks to @necauqua the controversial `unsafe` usage was already removed. I've omitted to change `revisions` from an Vec to a RevisonArg for the moment, as I will keep working on the file anyway.

PhilipMetzger force-pushed the push-mrzzusvxlqlt branch 2 times, most recently from 6e1977e to e7425b1 Compare October 31, 2023 21:57

martinvonz reviewed Oct 31, 2023

View reviewed changes

cli/src/commands/mod.rs Outdated Show resolved Hide resolved

PhilipMetzger force-pushed the push-mrzzusvxlqlt branch 2 times, most recently from 8e0a4a0 to cf16ca9 Compare November 1, 2023 21:36

PhilipMetzger changed the title ~~commands: move cmd_run into run.rs.~~ commands: move run into run.rs. Nov 1, 2023

PhilipMetzger force-pushed the push-mrzzusvxlqlt branch from cf16ca9 to 6bc1d81 Compare November 1, 2023 22:00

martinvonz approved these changes Nov 1, 2023

View reviewed changes

cli/src/commands/run.rs Outdated Show resolved Hide resolved

cli/src/commands/run.rs Outdated Show resolved Hide resolved

PhilipMetzger force-pushed the push-mrzzusvxlqlt branch 2 times, most recently from a50df7b to faabaee Compare November 2, 2023 19:15

ilyagr mentioned this pull request Nov 3, 2023

Move commands in separate files #2513

Merged

4 tasks

PhilipMetzger force-pushed the push-mrzzusvxlqlt branch from faabaee to 28d90df Compare November 3, 2023 18:33

martinvonz reviewed Nov 3, 2023

View reviewed changes

cli/src/commands/run.rs Show resolved Hide resolved

PhilipMetzger force-pushed the push-mrzzusvxlqlt branch from 28d90df to f7da7c8 Compare November 3, 2023 18:49

PhilipMetzger changed the title ~~commands: move run into run.rs.~~ run: Teach run to resolve revsets and about jobs. Nov 3, 2023

martinvonz approved these changes Nov 4, 2023

View reviewed changes

PhilipMetzger force-pushed the push-mrzzusvxlqlt branch from f7da7c8 to da2f14b Compare November 4, 2023 15:14

PhilipMetzger merged commit 8e4b6df into main Nov 4, 2023
14 checks passed

PhilipMetzger deleted the push-mrzzusvxlqlt branch November 4, 2023 15:29

arxanas reviewed Nov 4, 2023

View reviewed changes

cli/src/commands/run.rs Show resolved Hide resolved

cli/src/commands/run.rs Show resolved Hide resolved

PhilipMetzger mentioned this pull request Nov 8, 2023

run: Fix up various things. #2553

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

run: Teach run to resolve revsets and about jobs. #2486

run: Teach run to resolve revsets and about jobs. #2486

PhilipMetzger commented Oct 31, 2023 •

edited

Loading

martinvonz left a comment •

edited

Loading

arxanas commented Oct 31, 2023

PhilipMetzger commented Nov 2, 2023

ilyagr commented Nov 3, 2023 •

edited

Loading

arxanas left a comment

run: Teach run to resolve revsets and about jobs. #2486

run: Teach run to resolve revsets and about jobs. #2486

Conversation

PhilipMetzger commented Oct 31, 2023 • edited Loading

Checklist

martinvonz left a comment • edited Loading

Choose a reason for hiding this comment

arxanas commented Oct 31, 2023

PhilipMetzger commented Nov 2, 2023

ilyagr commented Nov 3, 2023 • edited Loading

arxanas left a comment

Choose a reason for hiding this comment

PhilipMetzger commented Oct 31, 2023 •

edited

Loading

martinvonz left a comment •

edited

Loading

ilyagr commented Nov 3, 2023 •

edited

Loading