lothiraldan requested changes to this revision.
lothiraldan added a comment.
This revision now requires changes to proceed.
I'm +1 on the feature.
But I have several concerns with the current implementation:
- Marking a whole command as readonly or not is too simplistic I'm afraid, we
have commands (like phase) that can be readonly or not based on their
parameters. I think we need a more fine-grained selection and that commands
needs to pass the desired directaccess mode to `scmutil.revrange`. For example,
imagine we add a `--reuse-message REV` argument to fold, that to select a
revision to use the message from. The usual argument of fold rewrite the
changes (so direct access is off). However having a more relax direct access
for the --reuse-message make sense.
- Putting direct access hashes in pinnedrevs attribute while having the same
filter function for all filters could lead to have visible filter excludind the
direct accessed hashes when we bust the volatile sets which will require a full
cache bust at the next check.
Instead, I propose:
- To clarify the vocabulary: direct access allow for node explicitly
specified to be made visible for the duration of a command. We could call such
revisions `visibility exceptions`. To achieve this the `exceptions` need to be
excluded from the usual filter applied to the repository during a command.
- Put the direct access hashes, not in pinnedrevs but in a separate
attribute, maybe with a dedicated API `repo.addvisibilityexception(revs)` that
could also be used by rebase and histedit as they already have some exception
mechanism.
- Keep the new filter name, but with a dedicated filter function
(`computehiddenexceptions` for example). The new dedicated function could use
the attribute updated by `repo.addvisibilityexception(revs)` to exclude them
from the result without ever impacting the `visible` filter.
- Makes `scmutil.revrange` accepts a new parameter
`visibility_exception_mode` for example which could have 3 values and could be
used to replace this condition `if repo and repo.filtername in
['visible-hidden', 'visible-warnhidden']:`:
- None, current behavior
- `warning`, warns when directly accessed hashes are detected, could be
used to replace this condition `if repo.filtername == 'visible-warnhidden':`
- `silent`, unhide directly accessed hashed without warning
- Now that we can distinguish between warning mode or silent mode, we could
use only a single filter, `visible+exceptions` for example, and limit the scope
of it with a context manager so consumers could get revisions with exceptions
without needing to reset the filter by hand, I have something like this in mind:
def my_command(ui, repo, *revs, foo=[]):
with repo.allowexception() as repo:
...
foorevs = scmutil.revrange(repo, foo, allowexception='warn')
...
targetrevs = scmutil.revrange(repo, foo)
...
I don't think it's the best API right now, but being able to limit explicitly
the "scope" of the new filter would be beneficial for avoiding hard to debug
issues and performances regressions. I couldn't find a better API than the
context manager + new revrange parameter, if you have ideas please share.
This proposal is focused on removing the implicit global state carried by the
filter name and pinnedrevs and make the filter change scope more controllable
and explicit.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D1144
To: pulkit, #hg-reviewers, lothiraldan
Cc: lothiraldan, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
