jj
jj copied to clipboard
jj-vcs
`jj` commands should throw a warning when the fileset does not exist
Closes https://github.com/jj-vcs/jj/issues/6556.
Checklist
If applicable:
- [x] I have updated
CHANGELOG.md - [x] I have updated the documentation (
README.md,docs/,demos/) - [x] I have updated the config schema (
cli/src/config-schema.json) - [x] I have added/updated tests to cover my changes
IMO, you should merge the revision that changes the command and the revision that adds the test for each command. It would be easier to review. And it's a bit strange, given how the revisions are organized, that the number of revisions in the PR is odd :)
IMO, you should merge the revision that changes the command and the revision that adds the test for each command. It would be easier to review.
I want to make sure that the warning is shown in a consistent manner across all commands. That's why the commands and tests are one after another. I would be happy to split the PR in whatever way is more convenient for the reviewer when ready :)
And it's a bit strange, given how the revisions are organized, that the number of revisions in the PR is odd :)
I didn't have to add tests for all the commands. Some of them already had tests capturing this behaviour, for example, split.
nit: Could you squash the tests into the commit they're testing? See https://jj-vcs.github.io/jj/latest/contributing/#commit-guidelines
Thanks again for working on this. We have a release scheduled for Wednesday. I think it's better to get this merged after the release so we get a chance to test it a bit before it's released. I'm not particularly worried about bugs, but I would like to see that there is no significant performance impact (including at Google) and that we feel that we print the warnings at the right time relative to other output so we don't release it on Wednesday and then change the behavior in some way in the very next release.
... I would like to see that there is no significant performance impact (including at Google)...
Sounds reasonable.
that we feel that we print the warnings at the right time relative to other output so we don't release it on Wednesday and then change the behavior in some way in the very next release.
I've ensured the warning about unmatched paths is always displayed first, before any other warning. I've kept this ordering consistent across all the commands. Does this address your concern about the warning placement?
Update: I like the idea of software (especially, dev tooling) failing fast. If something is not right, I would much rather know about it as early as possible.
I've ensured the warning about unmatched paths is always displayed first, before any other warning. I've kept this ordering consistent across all the commands. Does this address your concern about the warning placement?
My guess was actually that it's better to display it quite late (see some of the comments I left). I'm not sure which is better, but the fact that we have different intuition for it seems suggest that we should test it on ourselves before releasing it to users :)
In case you missed it, you're free to merge this when it's ready. If there are any questions for me or other reviewers, let us know.