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.

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

config: introduce default at = @ revset alias #4432

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

Conversation

thoughtpolice
Copy link
Collaborator

@thoughtpolice thoughtpolice commented Sep 10, 2024

When using Windows, @ is a bit of an annoyance to type in the terminal due to Windows Terminal escape rules, so you actually have to write it with a backtick in front. It's annoying to remember this and not possible to configure a different behavior, as far as I can see.

However, there is a smart trick you can do to make this a lot easier: just alias @ to the name at and be happy. Because:

  • It's short: sweet and easy to read, and requires no backticks.
  • It's unambiguous: a is from commit alphabet, but t is from the change alphabet
  • It can be typed without a huge loss in efficiency (and one hand, assuming QWERTY).

I believe it was Stephen Jennings who came up with this trick first in Discord, and I have it in my aliases, but I run into it every time I try to use JJ on a fresh Windows VM to test stuff. Let's make life a little easier for everyone and just ship this by default.

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

@thoughtpolice
Copy link
Collaborator Author

There's no changelog or docs yet but I'll get to it shortly assuming nobody has strong feelings on this.

@jennings
Copy link
Collaborator

I am honored!

but z is from the change alphabet

"z"? 🤔

When using Windows, `@` is a bit of an annoyance to type in the terminal due
to Windows Terminal escape rules, so you actually have to write it with a
backtick in front. It's annoying to remember this and not possible to configure
a different behavior, as far as I can see.

However, there is a smart trick you can do to make this a lot easier: just alias
`@` to the name `at` and be happy. Because:

- It's short: sweet and easy to read, and requires no backticks.
- It's unambiguous: `a` is from commit alphabet, but `t` is from the change alphabet
- It can be typed without a huge loss in efficiency (and one hand, assuming QWERTY).

I believe it was Stephen Jennings who came up with this trick first in Discord,
and I have it in my aliases, but I run into it every time I try to use JJ on a
fresh Windows VM to test stuff. Let's make life a little easier for everyone and
just ship this by default.

Signed-off-by: Austin Seipp <[email protected]>
@@ -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

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

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.

thoughtpolice added a commit that referenced this pull request Sep 11, 2024
In #4432 I submitted a patch to ship a symbol alias in the default
configuration. However, Yuya quickly pointed out that this isn't acceptable
because symbols override all branch and trunk names, meaning that the name `at`
would have at best been taken  away from users, and at worst been very confusing
for them down the road had they ever tried to use it.

I should have thought of this myself (I encountered it long ago when first using
Jujutsu, actually) but didn't. Such is life.

But this didn't cause any tests to fail, as it was not immediately clear this
would impede anything or cause downstream behavioral changes; the problems it
adds are actually latent and it's very possible someone will want to make the
same mistake in the future. And Yuya might not be able to stop them (me) in
time; requiring him to go out of his way and then handle the fallout. I'm not a
fan of this.

Instead, let's patch this latent hole for good, or until we decide otherwise, by
applying the Beyonce Rule: "if you liked that behavior, then you shoulda put a
test on it."

Signed-off-by: Austin Seipp <[email protected]>
thoughtpolice added a commit that referenced this pull request Sep 11, 2024
In #4432 I submitted a patch to ship a symbol alias in the default
configuration. However, Yuya quickly pointed out that this isn't acceptable
because symbols override all branch and tag names, meaning that the name `at`
would have at best been taken  away from users, and at worst been very confusing
for them down the road had they ever tried to use it.

I should have thought of this myself (I encountered it long ago when first using
Jujutsu, actually) but didn't. Such is life.

But this didn't cause any tests to fail, as it was not immediately clear this
would impede anything or cause downstream behavioral changes; the problems it
adds are actually latent and it's very possible someone will want to make the
same mistake in the future. And Yuya might not be able to stop them (me) in
time; requiring him to go out of his way and then handle the fallout. I'm not a
fan of this.

Instead, let's patch this latent hole for good, or until we decide otherwise, by
applying the Beyonce Rule: "if you liked that behavior, then you shoulda put a
test on it."

Signed-off-by: Austin Seipp <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants