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

[proposal] sl goto top/bottom/prev/next ? #444

Closed
francois opened this issue Jan 10, 2023 · 2 comments
Closed

[proposal] sl goto top/bottom/prev/next ? #444

francois opened this issue Jan 10, 2023 · 2 comments
Labels
enhancement ux

Comments

@francois
Copy link

I have found myself trying to run sl goto --bottom and sl goto --top a few times. I know --bottom and --top are supposed to be used with next and prev, but my fingers do the typing, not my brain.

I would argue that for consistency, having a single top-level gateway into moving the current tip would be preferable:

sl goto next # sl next
sl goto prev # sl prev
sl goto top # sl next --top
sl goto bottom # sl prev --bottom
sl goto $SHA1 # same as currently
sl goto $BOOKMARK # same as currently

I also know that currently, nothing prevents a bookmark from being named top, bottom, next or prev. Those would have to become reserved words in bookmarks.

What do you think?
@vegerot
Copy link
Contributor

vegerot commented Jan 11, 2023 •
edited
Loading

I too make this mistake frequently.

FYI top and bottom are already valid revsets.

We should add a revset aliases for prev to be .~1 and next to be .~-1

vegerot added a commit to vegerot/sapling that referenced this issue Jan 11, 2023
@vegerot
feat(scm): Add revset aliases for next and prev
7f715c6 
Summary: In the same way you can `sl go top`, it seems logical to also have
`sl go next`. However, instead it is `sl next`.  While I normally remember to
do `sl prev`, I sometimes accidentally do `sl go prev` out of habbit and get an
error.  Instead, let's simply add two revset aliases for `next` and `prev` to
make sl more intuitive.

Closes facebook#444

Test Plan: Automated tested blocked on facebook#447

Manually tested by running `sl go next` and `sl go prev` and verifying I ended
up in the right spot.

Question for reviewer: I don't know if this is relevant to this patch or not
but while testing this I would sometimes have this error

```py
./sl
Traceback (most recent call last):
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/__init__.py", line 86, in run
    dispatch.runchgserver(args[2:])
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/dispatch.py", line 368, in runchgserver
    _preimportmodules()
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/dispatch.py", line 348, in _preimportmodules
    extensions.preimport(extname)
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/extensions.py", line 208, in preimport
    mod = getattr(__import__("edenscm.ext.%s" % name).ext, name)
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/ext/copytrace.py", line 169, in <module>
    ) -> Tuple[Optional[dbm._Database], Optional[Tuple[Type[dbm._error], Type[OSError]]]]:
AttributeError: module 'dbm' has no attribute '_Database'
```

and other times I would get this warning

```python
CHGDISABLE=1 ./sl go prev
Traceback (most recent call last):
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/extensions.py", line 399, in _runextsetup
    extsetup(ui)
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/ext/copytrace.py", line 113, in extsetup
    extensions.wrapfunction(copiesmod, "mergecopies", _mergecopies)
NameError: name '_mergecopies' is not defined
warning: failed to set up extension copytrace: name '_mergecopies' is not defined
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
```
@vegerot vegerot mentioned this issue Jan 11, 2023
Closed
vegerot added a commit to vegerot/sapling that referenced this issue Jan 12, 2023
@vegerot
feat(scm): Add revset aliases for next and prev
a2d44c2 
Summary: In the same way you can `sl go top`, it seems logical to also have
`sl go next`. However, instead it is `sl next`.  While I normally remember to
do `sl prev`, I sometimes accidentally do `sl go prev` out of habbit and get an
error.  Instead, let's simply add two revset aliases for `next` and `prev` to
make sl more intuitive.

Differences from `sl next/prev`:
- Cannot specify number of commits: `sl prev 2` works but `sl goto prev 2`
doesn't. (but `sl go prev~1` would)
- Handles merge commits differently.  No interactive selection and no `sl next --newest`

Closes facebook#444

Test Plan: Automated tested blocked on facebook#447

Manually tested by running `sl go next` and `sl go prev` and verifying I ended
up in the right spot.

Question for reviewer: I don't know if this is relevant to this patch or not
but while testing this I would sometimes have this error

```py
./sl
Traceback (most recent call last):
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/__init__.py", line 86, in run
    dispatch.runchgserver(args[2:])
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/dispatch.py", line 368, in runchgserver
    _preimportmodules()
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/dispatch.py", line 348, in _preimportmodules
    extensions.preimport(extname)
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/extensions.py", line 208, in preimport
    mod = getattr(__import__("edenscm.ext.%s" % name).ext, name)
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/ext/copytrace.py", line 169, in <module>
    ) -> Tuple[Optional[dbm._Database], Optional[Tuple[Type[dbm._error], Type[OSError]]]]:
AttributeError: module 'dbm' has no attribute '_Database'
```

and other times I would get this warning

```python
CHGDISABLE=1 ./sl go prev
Traceback (most recent call last):
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/extensions.py", line 399, in _runextsetup
    extsetup(ui)
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/ext/copytrace.py", line 113, in extsetup
    extensions.wrapfunction(copiesmod, "mergecopies", _mergecopies)
NameError: name '_mergecopies' is not defined
warning: failed to set up extension copytrace: name '_mergecopies' is not defined
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
```
@francois
Copy link
Author

Oh wow! I never realized that sl go bottom and sl go top already work! This is neat! #448 is a neat alias and that's super useful. Thanks a bunch!

vegerot added a commit to vegerot/sapling that referenced this issue Jan 13, 2023
@vegerot
feat(scm): Add revset aliases for next and prev
a0e48b7 
Summary: In the same way you can `sl go top`, it seems logical to also have
`sl go next`. However, instead it is `sl next`.  While I normally remember to
do `sl prev`, I sometimes accidentally do `sl go prev` out of habbit and get an
error.  Instead, let's simply add two revset aliases for `next` and `prev` to
make sl more intuitive.

Differences from `sl next/prev`:
- Cannot specify number of commits: `sl prev 2` works but `sl goto prev 2`
doesn't. (but `sl go prev~1` would)
- Handles merge commits differently.  No interactive selection and no `sl next --newest`

Closes facebook#444

Test Plan: Automated tested blocked on facebook#447

Manually tested by running `sl go next` and `sl go prev` and verifying I ended
up in the right spot.

Question for reviewer: I don't know if this is relevant to this patch or not
but while testing this I would sometimes have this error

```py
./sl
Traceback (most recent call last):
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/__init__.py", line 86, in run
    dispatch.runchgserver(args[2:])
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/dispatch.py", line 368, in runchgserver
    _preimportmodules()
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/dispatch.py", line 348, in _preimportmodules
    extensions.preimport(extname)
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/extensions.py", line 208, in preimport
    mod = getattr(__import__("edenscm.ext.%s" % name).ext, name)
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/ext/copytrace.py", line 169, in <module>
    ) -> Tuple[Optional[dbm._Database], Optional[Tuple[Type[dbm._error], Type[OSError]]]]:
AttributeError: module 'dbm' has no attribute '_Database'
```

and other times I would get this warning

```python
CHGDISABLE=1 ./sl go prev
Traceback (most recent call last):
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/extensions.py", line 399, in _runextsetup
    extsetup(ui)
  File "/Users/m0c0j7y/workspace/github.com/facebook/sapling/eden/scm/edenscm/ext/copytrace.py", line 113, in extsetup
    extensions.wrapfunction(copiesmod, "mergecopies", _mergecopies)
NameError: name '_mergecopies' is not defined
warning: failed to set up extension copytrace: name '_mergecopies' is not defined
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement ux
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(scm): Add revset aliases for next and prev vegerot/sapling
3 participants
@francois @vegerot @sggutier