Commands which warn "The argument XX is being interpreted as a path" (eg, squash) should decline to take action if no paths match

[mhammond]

I just did jj squash xw and the output was:

warning: The argument "xw" is being interpreted as a path. To specify a revset, pass -r "xw" instead.
Rebased 14 descendant commits

The warning was correct - I did mean to specify -r. However, it still performed a squash.

Given xw didn't actually match any paths, jj should IMO have declined to do anything.

[Kintaro]

I don't seem to be able to replicate it.

➜ jj git init
Initialized repo in "."
➜ jj describe -m "first"
Working copy now at: mynoyzzw e7859c26 (empty) first
Parent commit      : zzzzzzzz 00000000 (empty) (no description set)
➜ echo "foo" > foo
➜ jj log
@  mynoyzzw me@example.com 2024-03-28 15:56:48.000 +09:00 5bd2957b
│  first
◉  zzzzzzzz root() 00000000
➜ jj new
Working copy now at: rmvzttzn e5e243f2 (empty) (no description set)
Parent commit      : mynoyzzw 5bd2957b first
➜ jj describe -m "second"
Working copy now at: rmvzttzn c6b7467f (empty) second
Parent commit      : mynoyzzw 5bd2957b first
➜ echo "bar" > bar
➜ jj log
@  rmvzttzn me@example.com 2024-03-28 15:57:24.000 +09:00 472cd8f0
│  second
◉  mynoyzzw me@example.com 2024-03-28 15:56:48.000 +09:00 5bd2957b
│  first
◉  zzzzzzzz root() 00000000
➜ jj squash r
warning: The argument "r" is being interpreted as a path. To specify a revset, pass -r "r" instead.
Working copy now at: rmvzttzn aac9f1e6 second
Parent commit      : mynoyzzw 8ce69cc9 first
➜ jj log
@  rmvzttzn me@example.com 2024-03-28 15:57:29.000 +09:00 aac9f1e6
│  second
◉  mynoyzzw me@example.com 2024-03-28 15:57:29.000 +09:00 8ce69cc9
│  first
◉  zzzzzzzz root() 00000000

I don't see the squash being executed.

[mhammond]

Thanks, I did mean to come back to this as it occurred to me later that the squash did indeed not happen - but jj took some action here, right? While the change IDs are the same, the commit IDs are different as shown both in the log and the working copy. That implies something changed, right?

I failed to reproduce exactly what I saw, but the Rebased 14 descendant commits message in particular I found confusing - there's a clear implication it changed things. So I was confused about the new state of the repo, so re-actively did jj undo but the entire process left me a little confused about exactly what happened and what state I was now in.

[Kintaro]

The changes to the repo you talk about are probably due to the operations log. Squash is a mutational command, so it probably still changes some internals along the way, before it figures out the path doesn't exist and a rev was probably meant.

By looking at the code (still new to jj myself though), I see that it does build diffs etc to figure out what to squash. My guess is that the changes in git commit ids stem from there?

[martinvonz]

Yes, that's correct. The commits get rewritten and the timestamp changes, but the content stays the same.

[Kintaro]

In that case, I'd vote for closing the issue. The only improvements I see are:

• discard the transaction when no diff to be found, but that might contradict the model
• the warning only triggers if it's a single path, so we might improve it by checking every path if it actually exists. And it only triggers if the diff is empty at the moment. If one were to out in 2 paths, the warning doesn't trigger and it still doesn't really do anything. So the best improvement I'd see here is more of a guidance warning.

The latter case should be revisited. If more than one path is given it is clear that revisions are not intended, but there might be an argument for if a given path doesn't exist, that a warning should be given because the intention of the squash wasn't there.

[martinvonz]

I think this is similar to #3367, which made jj rebase a no-op if the commit to rebase was already in place. I think it's similar in that the old behavior was more consistent but probably not what the user wanted. That's what this request is about as well - making jj squash behave differently when squashing an empty set of paths.

It's very tempting to say that we should not have these special behaviors and instead print a warning and let the user undo if they didn't want to make the change. That's what we currently do. I think @arxanas was arguing for keeping that behavior. In addition to being simpler, it also also means we don't have to worry about finding an override if the user actually did want to make the no-op change.

By the way, we have had special handling in jj describe for a very long time to make it skip rewriting the commit if the message was unchanged.

If we're going to add more of these special cases, maybe we can at least find some generic way of implementing it, so it's less code and more consistent behavior.

[Kintaro]

I guess the transaction could be aborted if the diff is empty.

[martinvonz]

I guess the transaction could be aborted if the diff is empty.

That would not have been enough for the behavior we went with in the rebase case. That command avoids rewriting individual commits if only their timestamps would change (basically); there can still be other commits (e.g. when using multiple -s or when rebasing a merge commit) that get rewritten.

[mhammond]

From my perspective, this issue is about user surprise rather than the broader issue of "rebases should be noops in some cases" - it's about helping to grow user confidence in the tool as they start using it.

The specific warning appears to only be emitted for log, move and squash - jj move doesn't really have the same scope for "unexpected rewrite" because the command-line options are such that there's less scope for ambiguity. I think there would be an expectation of log being a read-only operation, so there's no justification for that command taking special action here.

So I'd be "happy" (which isn't to say I'd be "unhappy" with another outcome) if just squash had special handling for this case, which would (IIUC) leave rebase free to do what it does now. I do understand that the implementation is such that this might be tricky to actually do though.

Naively though, I made a hacky change so an error was returned after printing the warning here - which looks like it is only emitted when there are no changes to the tree? The only test which failed was this one, which appears to already have the expectation that no material changes were made, it's just expecting success rather than failure.

[mhammond]

I see the tree operations have already been done, sorry for the noise.

[bnjmnt4n]

I think this issue got sidetracked slightly. It seems like there are two orthogonal issues here:

1. Specifying a <path> argument which doesn't match any path shows a warning, but only when the resulting diff is empty.
2. Squash will cause commits to be rewritten even if its contents are the same.

For (1), I'd like to propose that any arguments interpreted as paths should cause the whole squash operation to exit early with the warning, instead of having the warning show up only if the resulting diff is empty.

We can probably open a separate issue for (2), where we can consider more general solutions as well.

[noahmayr]

For (1), I'd like to propose that any arguments interpreted as paths should cause the whole squash operation to exit early with the warning, instead of having the warning show up only if the resulting diff is empty.

I'm nut sure that's a good idea, running jj squash docs I'd expect that all changes to ./docs will be squashed into the parent commit and not that the command will be aborted

[martinvonz]

I'm nut sure that's a good idea, running jj squash docs I'd expect that all changes to ./docs will be squashed into the parent commit and not that the command will be aborted

With the commit linked above, it will be a no-op if there are no changes to docs. Are you both okay with that solution?

[arxanas]

I opened #3809 as my preferred solution.