-
Notifications
You must be signed in to change notification settings - Fork 343
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
cli: implement jj root #2801
cli: implement jj root #2801
Conversation
The test handles only one case? |
4d7a040
to
ccbea9e
Compare
I forgot to put a test for the normal failure case, it's added now. Did you have specific tests in mind? I could cover the non-utf8 error, but it's not obviously worth it. If jj supports worktrees/shares, I guess that could be tested as well, it depends to what extent we trust on the internal apis forcing callers to do the right things. |
I was thinking that there are more options to |
I like this, as it obviously useful for scripting. On the whole UTF-8 path topic, @martinvonz mentioned quite a while ago that |
ccbea9e
to
e887ffb
Compare
I made the test run with both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some nits, I think it's also missing a test for multiple workspaces.
I was looking at making a test involving workspaces, and AFAICT, Should I move that command to the toplevel then? It does seem that the need for the repo root should come before the need for workspaces. If that's the plan, should I preserve some compatibility of the CLI? |
Yes, it does sound like it's the same.
The reason it's under Maybe we could add |
e887ffb
to
a3fefcf
Compare
The reasoning makes sense. As you say, for me it's the discoverability that makes me prefer having the command at the top of the command line (well, that and the fact that I tweaked the PR to:
Finally, the existing tests of |
|
UPDATE: I originally added this thought at the end, but I now decided I like it: we could have I think I think a |
Well, this is more contentious of a change than I imagined it would be ! Given that I now know of the existence of Another option would be to provide
The terminology includes: repo, working copy and workspace. IIUC "working copy" means check-out + related state, "workspace" is an alias for "working copy" except with emphasis on the shared history ("two working copies" might mean two unrelated repos, but "two workspaces" probably means two working copies attached to the same repo), and "repo" is either the store directory, or the store + working copy whose .jj directory contains the store, or perhaps the store + all working copies.
That sounds reasonable. In any case, the options so far are:
|
Another option is to stick to verbs for commands, and use |
I really like that idea, because as at some point
I'd rather not put the git repo in there, as it isn't configurable and just straight up confusing. This also was my previous idea when we talked about something like it. |
Sorry, these kind of non-urgent changes related to naming often lead to bike-sheddy discussions. I, personally, also like to ponder on the ideal outcome, even if it shouldn't necessarily be part of this particular PR.
I, personally, would be happy with 1, 3, or 4. 2 is also fine if the prerequisite is satisfied :). If we go with 3, I'd ponder switching to 4 (or something else) later, but it could be a good solution until somebody has the energy to work on that.
This sounds very reasonable if we were designing the UI from scratch. Unfortunately, |
They are defined in the glossary.
I like this proposal, even if we decide to not add any of the flags yet. I don't really like the I still like having |
So what do we do with this PR? I'm leaning towards keeping |
Im still in favor of having
I'm fine with having a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG, but let's wait for consensus on how to continue, I think I've made all my points.
I'm OK with any of the options. I don't feel very decisive at the moment. I wouldn't mind for One more tangential thought about |
Sorry, I was initially waiting to see if the discussion was going to settle, and context-switched. The intersection of what everyone is saying seems to be:
It doesn't really matter now, but just to clarify, I didn't mean to put things literally in |
Yeah, that definitely makes sense. I'm really sorry for missunderstanding your intent and only focusing on the |
a3fefcf
to
ceca905
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for your patience :)
This is a convenient command, for scripting things like `cd $(jj root) && do something`, and it seems better to allow people to find it before they learn about workspaces.
ceca905
to
2535b67
Compare
This is a convenient command when scripting around, for things like: $ cd $(jj root) && do something
Currently this fails if the path to the repo contains non-utf8 bytes, similarly to
jj status
failing on non-utf8 paths inside the repo.This is a proposal out of the blue, but since this is a small change and it seems not contentious, I figured if it was simpler to do it and possibly argue about the utility of the PR, rather than have a round of discussion followed by a round of PR.
Checklist
If applicable:
CHANGELOG.md