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

config: introduce default at = @ revset alias #4432

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

config: introduce default at = @ revset alias #4432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
(inherit from parent; default), `full` (full working copy), or `empty` (the
empty working copy).

* A revset alias named `at` is now provided, which functions identically to the
working-copy symbol `@`. This symbol cannot conflict with any Change or Commit
ID, and may be used in places where `@` may require escaping, like the Windows
terminal.

### Fixed bugs

* Fixed panic when parsing invalid conflict markers of a particular form.
Expand Down
3 changes: 3 additions & 0 deletions cli/src/config/revsets.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,6 @@ latest(
'immutable_heads()' = 'builtin_immutable_heads()'
'immutable()' = '::(immutable_heads() | root())'
'mutable()' = '~immutable()'

# NOTE: for Windows users, who don't want to type `@ all the time
'at' = '@'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Symbol aliases overrides tags and branches, so builtin aliases have to be all functions. And at() wouldn't be easy to type.

Maybe add doc example instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's unfortunately a pretty big hole in the side of the ship, so to speak. A doc example is at least in order though.

I also feel like this is something we should should have tests for i.e. iff there exists any keys in [revset-aliases] in any of the builtin configs, and they are not functions, then panic — since I wouldn't have thought of this without your suggestion. So maybe I should fix that, too.

Copy link
Collaborator

@emilazy emilazy Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a more verbose way to specify a branch/tag specifically? I feel like at will not be a common name for those, so as long as there’s an override it might be okay.

Also, it should maybe say “PowerShell” rather than “Windows” here, as… technically you can use it on other OSes. Technically. (And there was some confusion about cmd.exe not requiring it on the Discord.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"at" or tags(exact:at) works, but less special case is better. It would be weird if trunk was a builtin alias and not pointing to the trunk branch. The at alias looks similar.

I also think that, to work around PowerShell, it's better to find another symbol (e.g. .) to replace @, instead of adding a builtin alias which only PowerShell users would practically use.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One reason I didn't pick . for the working copy was that it seemed like it would be confusing to former Mercurial users (. is the parent of the working copy there). Also, the @ symbol works well in the graph log output. I'm not sure . would work as well there. It might be hard to notice, and it might be hard to realize the correspondence with the revset symbol. Were you thinking of shipping . as an alias or to eventually have it replace @?

Shipping at by default on Windows might be okay if we think only (a subset of) Windows users would use it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do people think about Yuya's proposal to simply document the alias? Seems fine to at least start with that and see how many reports we get after that?

Btw, if the @ symbol is annoying for PowerShell users, then our use of it in remote bookmarks and workspace identifiers will also be a problem, although a much less frequent one. I suppose one possible alternative syntax that works particularly well for remote bookmarks is origin/main, but I'm not sure I like / for the working copy and /other for the working copy in workspace other as much.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s okay in those contexts because it isn’t the first character in a token. main@origin isn’t a problem because PowerShell doesn’t interpret that as the start of an array since it’s after a word character.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. @other (which refers to the working-copy in workspace other) is still a problem then.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so default@, ..@, etc. work. The workspace id syntax is <id>@, btw.

Copy link
Collaborator

@ilyagr ilyagr Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do people think about Yuya's proposal to simply document the alias?

We actually already document it, sort of, in https://martinvonz.github.io/jj/latest/windows/#typing-in-powershell. That recommends HEAD, which is not as good as at IMO.

Should we make rename it and make it more prominent? I think so. It'd make sense to explain it in the main config doc and then link to it again from the Windows doc. Update: Even on non-Windows, it's nice to not have to reach for the Shift key sometimes.

Separately (and not blocking the more obvious improvements), should we explain the "at" syntax for a branch/tag next to where we describe revset aliases?

Separately again, I still wonder about the larger issue. While we reduce the problem of overlaps with branch names if we don't define at ourselves, it'd be nice to have a solution that also works for users who configured it themselves (and maybe forget). But we can ponder and/or discuss this question long after we fix the easy-to-fix things.

4 changes: 4 additions & 0 deletions docs/revsets.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ ID or a Git ref pointing to them).
## Symbols

The `@` expression refers to the working copy commit in the current workspace.
In places where the `@` symbol needs to be escaped, such as inside a Windows
terminal, you may use the name `at` instead, which is identical to `@` and
requires no quoting.

Use `<workspace name>@` to refer to the working-copy commit in another
workspace. Use `<name>@<remote>` to refer to a remote-tracking branch.

Expand Down