`jj` commands should throw a warning when the fileset does not exist #7693

whereistejas · 2025-10-10T14:07:27Z

Closes #6556.

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/updated tests to cover my changes

github-actions

The following commits do not follow our format for subject lines:

a98a670: Use print_unmatched_explicit_paths in cmd_status.
14e7bbd: Update CHANGELOG

Commits should have a subject line following the format <topic>: <description>. Please review the commit guidelines for more information.

All commits are now correctly formatted. Thank you for your contribution!

CHANGELOG.md

cli/src/commands/status.rs

glehmann · 2025-10-22T15:32:42Z

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 :)

whereistejas · 2025-10-22T16:03:31Z

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.

martinvonz · 2025-10-22T16:12:21Z

nit: Could you squash the tests into the commit they're testing? See https://jj-vcs.github.io/jj/latest/contributing/#commit-guidelines

martinvonz

Thanks!

martinvonz · 2025-10-29T16:14:50Z

cli/src/cli_util.rs

+    // If there are no explicit paths to show then return early.
+    if explicit_paths.is_empty() {
+        return Ok(());
+    }


What's the benefit of this check? That should ideally be mentioned in the commit description (if it's not obvious, and it's not obvious to me at least).

Is this just an optimization? But if explicit_paths is empty, then explicit_paths.retain() will be very cheap anyway. So I don't think it's a worthwhile optimization.

Or is it about correctness when explicit_paths is empty and trees is also empty? It makes sense to not print the warning in such cases. Are you going to add callers that may pass an empty list of trees in a later commit? Could you update the commit description to explain that that's the reason if it is?

Actually, if the second thing is what it's about, I would suggest just moving the early return out of the loop. As mentioned above, I don't think it's useful as an optimization anyway. If we move it after the loop, then it will handle the case of an empty list of trees. If you do that, I would suggest actually not making it an early return but instead make the output conditional on the list being non-empty.

It's the latter. Some scenarios when a fileset_expression may evaluate to valid explicit paths but we may not have any trees to compare them against are:

Squashing a commit into itself.

Running jj log -r "none()"

I will add an explanation to the commit message.

cli/src/cli_util.rs

cli/src/commands/status.rs

cli/src/commands/squash.rs

cli/src/commands/split.rs

cli/src/commands/interdiff.rs

cli/src/commands/fix.rs

CHANGELOG.md

cli/src/commands/absorb.rs

We should not display any warning if `explicit_paths` is empty. Previously, we would by-pass this check if `trees` was empty (which was incorrect behaviour).

We inline `print_unmatched_explicit_paths` when displaying the log as graph for performance reasons.

martinvonz · 2025-10-31T16:04:17Z

cli/src/cli_util.rs

-        }
    }
+
+    if explicit_paths.is_empty() {


optional: I don't think an early return is worth it when it's skipping just a few lines and the lines are no overly long already, so I slightly prefer this:

if !explicit_paths.is_empty() { let ui_paths = explicit_paths .iter() .map(|&path| workspace_command.format_file_path(path)) .join(", "); writeln!( ui.warning_default(), "No matching entries for paths: {ui_paths}" )?; } Ok(())

martinvonz · 2025-10-31T16:34:01Z

cli/src/commands/split.rs

+            workspace_command,
+            &fileset_expression,
+            [
+                // We check the parent commit to account for delete files.


nit: "delete" -> "deleted"

martinvonz · 2025-10-31T16:55:30Z

cli/src/commands/log.rs

+            let commits: Vec<Commit> = iter.commits(store).try_collect()?;
+
+            let trees: Vec<MergedTree> =
+                commits.iter().map(|commit| commit.tree()).try_collect()?;
+
+            print_unmatched_explicit_paths(ui, &workspace_command, &fileset_expression, &trees)?;


nit: We should be able to share the inlined version of print_unmatched_explicit_paths() instead and not have to collect all commits and tress to vectors

martinvonz · 2025-10-31T17:05:39Z

cli/src/commands/log.rs

                let commit = store.get_commit(&key.0)?;
+                let tree = commit.tree()?;
+                // TODO: propagate errors
+                explicit_paths.retain(|&path| tree.path_value(path).unwrap().is_absent());


Hmm, we should also remove paths that are in the parent tree. Otherwise jj log <deleted file> -r <revision where file was deleted> will print the warning. However, it's expensive to calculate the parent tree for merge commits. Perhaps we can remove paths that are in any parent tree instead. That's slightly incorrect because if a file was removed on one of the parent branches of a merge commit then we won't print the warning for the merge commit even though the file wasn't changed or present in the commit. That's probably fine because the point of the warning is to detect typos and such, and if the file did exist at some point, then it's probably not a typo.

It might be good to have tests for these two cases too. To clarify, here's the second case:

$ jj log D (empty) |\ C| (empty) | | | B delete foo |/ A add file foo $ jj log -r D foo # Since foo isn't present or modified in D, we should ideally print a warning but I don't think we can because it's slow

Btw, jj log is also an exception because the user can do either jj log <fileset> or jj log -r 'files(<fileset>)' (or something more complex). We will only show the warning if the fileset was passed as a positional argument, not if it was part of the revset. Not asking for any changes here, just noting this.

martinvonz · 2025-10-31T17:08:44Z

cli/src/commands/interdiff.rs

+        ui,
+        &workspace_command,
+        &fileset_expression,
+        // We check the parent commits to account for delete files.


nit: "delete" -> "deleted"

martinvonz · 2025-10-31T17:10:40Z

cli/tests/test_split_command.rs

    [EOF]
    ");
+
+    let _ = work_dir.run_jj(["new"]).success();


nit: You should be able to remove the let _ = (same in some other commits in this PR)

martinvonz · 2025-10-31T17:12:03Z

cli/src/commands/fix.rs

+
+    let repo = workspace_command.repo();
+
+    let root_commits: Vec<Commit> = target_expr


nit: "root_commits" -> "commits" since they're not just the roots

martinvonz · 2025-10-31T17:14:27Z

cli/src/commands/fix.rs

-        .parse_file_patterns(ui, &args.paths)?
-        .to_matcher();
+
+    let (root_commits, root_trees): (Vec<_>, Vec<_>) = root_commits


nit: "root_commits" -> "commit_ids" since they're the IDs

martinvonz · 2025-10-31T17:29:52Z

cli/src/commands/fix.rs

+    let (root_commits, root_trees): (Vec<_>, Vec<_>) = root_commits
+        .into_iter()
+        .map(|commit| (commit.id().clone(), commit.tree()))
+        .unzip();
+    let root_trees: Vec<MergedTree> = root_trees.into_iter().try_collect()?;


nit: probably clearer as two separate iterators:

let commit_ids = commits.iter().map(|commit| commit.id().clone()).collect_vec(); let trees: Vec<_> = commits.iter().map(|commit| commit.tree()).try_collect()?;

whereistejas requested a review from a team as a code owner October 10, 2025 14:07

github-actions bot previously requested changes Oct 10, 2025

View reviewed changes

whereistejas changed the title ~~jj status should throw an error when the fileset does not exist~~ jj status should throw a warning when the fileset does not exist Oct 10, 2025

whereistejas force-pushed the warn-when-fileset-is-missing branch from 14e7bbd to 84fb867 Compare October 10, 2025 14:11

whereistejas force-pushed the warn-when-fileset-is-missing branch from 84fb867 to bea6920 Compare October 10, 2025 14:15

martinvonz reviewed Oct 10, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

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

whereistejas force-pushed the warn-when-fileset-is-missing branch 3 times, most recently from fb17003 to d6a7b86 Compare October 13, 2025 16:30

whereistejas changed the title ~~jj status should throw a warning when the fileset does not exist~~ jj commands should throw a warning when the fileset does not exist Oct 13, 2025

whereistejas force-pushed the warn-when-fileset-is-missing branch 3 times, most recently from 0eeeaac to 0c2a608 Compare October 13, 2025 17:30

whereistejas force-pushed the warn-when-fileset-is-missing branch 3 times, most recently from 5e3fd26 to 55ab0d8 Compare October 21, 2025 15:46

whereistejas force-pushed the warn-when-fileset-is-missing branch 4 times, most recently from ccb9689 to 0d8298b Compare October 29, 2025 16:10

martinvonz reviewed Oct 29, 2025

View reviewed changes

whereistejas force-pushed the warn-when-fileset-is-missing branch 2 times, most recently from d1ffa00 to 5ffbff5 Compare October 31, 2025 15:43

whereistejas added 2 commits October 31, 2025 15:46

cli: always check if explicit_paths is empty

480ea56

We should not display any warning if `explicit_paths` is empty. Previously, we would by-pass this check if `trees` was empty (which was incorrect behaviour).

cli: jj status warns when fileset doesn't exist

11b2f6e

whereistejas force-pushed the warn-when-fileset-is-missing branch from 5ffbff5 to bf2283f Compare October 31, 2025 15:54

cli: jj squash warns when fileset doesn't exist

6deadac

whereistejas force-pushed the warn-when-fileset-is-missing branch from bf2283f to ddc8109 Compare October 31, 2025 15:56

whereistejas added 7 commits October 31, 2025 17:02

cli: jj split warns when fileset doesn't exist

04bbcde

cli: jj restore warns when fileset doesn't exist

2b390d9

cli: jj resolve warns when fileset doesn't exist

2823e3b

cli: jj log warns when fileset doesn't exist

6809c9d

We inline `print_unmatched_explicit_paths` when displaying the log as graph for performance reasons.

cli: jj interdiff warns when fileset doesn't exist

c2f5c02

cli: jj fix warns when fileset doesn't exist

bdd6019

cli: jj absorb warns when fileset doesn't exist

484bd5f

whereistejas force-pushed the warn-when-fileset-is-missing branch from ddc8109 to 484bd5f Compare October 31, 2025 17:02

whereistejas requested a review from martinvonz October 31, 2025 17:24

martinvonz reviewed Oct 31, 2025

View reviewed changes


		let repo = workspace_command.repo();

		let root_commits: Vec<Commit> = target_expr

jj commands should throw a warning when the fileset does not exist #7693

Are you sure you want to change the base?

jj commands should throw a warning when the fileset does not exist #7693

Conversation

whereistejas commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

github-actions bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glehmann commented Oct 22, 2025

Uh oh!

whereistejas commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinvonz commented Oct 22, 2025

Uh oh!

martinvonz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whereistejas Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

`jj` commands should throw a warning when the fileset does not exist #7693

`jj` commands should throw a warning when the fileset does not exist #7693

whereistejas commented Oct 10, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading

whereistejas commented Oct 22, 2025 •

edited

Loading

whereistejas Oct 31, 2025 •

edited

Loading