Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

jj diff should warn when revset is used without -r (e.g. report nonexistent paths given to it) #505

Closed
ilyagr opened this issue Aug 31, 2022 · 7 comments · May be fixed by #1559
Closed

jj diff should warn when revset is used without -r (e.g. report nonexistent paths given to it) #505

ilyagr opened this issue Aug 31, 2022 · 7 comments · May be fixed by #1559

Comments

@ilyagr
Copy link
Contributor

ilyagr commented Aug 31, 2022

I often type jj diff @- instead of jj diff -r @- and see no results. I can also imagine a situation when I want to check for diffs in a file, mistype its name, and remain unaware of some important change in that file.

I think jj diff should report an error if it's given a path that doesn't exist in either of the commits it's diffing between.

There are a couple of questions I haven't yet resolved for myself:

  1. Does there need to be an option to suppress this error?
  2. Should this be implemented in more commands than just jj diff.
@martinvonz
Copy link
Member

I agree. Mercurial does that in many commands (hg diff, hg add/rm, hg cat, hg commit, ...) and it's quite helpful.

Another complication is when you do things like jj diff foo bar the foo and bar there is interpreted as a prefix and not necessarily a file. I suppose we can handle that by calculating (and displaying?) the diff as usual and record which paths we visit. If not every positional command line argument matches at least one file, then we print that error/warning. Once we support other kinds of patterns than just prefixes (e.g. like hg's set:**.rs), your first question becomes especially relevant.

@arxanas
Copy link
Contributor

arxanas commented Nov 18, 2022

I created #768 to handle a similar situation for jj log.

@ilyagr ilyagr added polish🪒🐃 Make existing features more convenient and more consistent and removed polish🪒🐃 Make existing features more convenient and more consistent labels Dec 11, 2022
@martinvonz martinvonz mentioned this issue Dec 29, 2022
Merged
3 tasks
@martinvonz martinvonz mentioned this issue Jan 30, 2023
Open
@ilyagr ilyagr mentioned this issue Apr 20, 2023
Closed
@ilyagr ilyagr changed the title jj diff should report nonexistent paths given to it jj diff should warn when revset is used without -r (e.g. report nonexistent paths given to it) Apr 20, 2023
@ilyagr
Copy link
Contributor Author

ilyagr commented Apr 20, 2023 •
edited
Loading

I stole the title of the now-closed #1538.

I created #768 to handle a similar situation for jj log.

That turned out to be extremely effective, for me at least, so it might be an easier approach than what I originally suggested.

@tp-woven tp-woven mentioned this issue Apr 28, 2023
Draft
4 tasks
@martinvonz martinvonz mentioned this issue Jun 14, 2023
Merged
2 tasks
@ilyagr
Copy link
Contributor Author

ilyagr commented Nov 3, 2023

We had an interesting discussion on this topic elsewhere, beginning at #2495 (reply in thread).

yuja added a commit to yuja/jj that referenced this issue Apr 9, 2024
@yuja
cli: warn explicit paths not exist in either of diff trees
ec45ed6 
Maybe we can optimize it to check paths during diffing, but I think it's okay
to add extra lookup cost at the end. The size of the path arguments is usually
small.

Closes jj-vcs#505
@emesterhazy
Copy link
Contributor

RE: Ilya's last comment, I would also prefer if jj diff took a revision argument instead of a path argument. I very frequently diff revisions and rarely want to limit the diff to a specific path.

yuja added a commit to yuja/jj that referenced this issue Apr 9, 2024
@yuja
cli: warn explicit paths not exist in either of diff trees
a6600ec 
Maybe we can optimize it to check paths during diffing, but I think it's okay
to add extra lookup cost at the end. The size of the path arguments is usually
small.

Closes jj-vcs#505
yuja added a commit to yuja/jj that referenced this issue Apr 10, 2024
@yuja
cli: warn explicit paths not exist in either of diff trees
82d2d9f 
Maybe we can optimize it to check paths during diffing, but I think it's okay
to add extra lookup cost at the end. The size of the path arguments is usually
small.

Closes jj-vcs#505
@yuja yuja closed this as completed in ae70db8 Apr 10, 2024
@ilyagr
Copy link
Contributor Author

ilyagr commented Apr 10, 2024

Thanks Yuya! I feel a bit nostalgic; I think this bug was either the first or one of the very first things I did in this repo.

@arxanas
Copy link
Contributor

arxanas commented Jun 1, 2024

I opened #3809 as my preferred solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

warn when file paths passed to diff look like revsets jj-vcs/jj
4 participants
@arxanas @martinvonz @ilyagr @emesterhazy