This patch adds test for accessing hidden commits by read-only commands for
which cmdtype attribute was set in an earlier patch (not part of this series).
Details
Details
- Reviewers
lothiraldan - Group Reviewers
hg-reviewers
Diff Detail
Diff Detail
- Repository
- rHG Mercurial
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
Comment Actions
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 suggest:
- 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.
Comment Actions
@lothiraldan
Are pinned revs deleted when the cache is rebuilt? I would kinda not expect them to be. If they are deleted, then yea moving these exceptions to another attribute that is persisted across cache clears makes sense.
For your concern about changing from a command-level option to a scope and revrange level option, I have a couple comments:
- If we went the current command-level flag, the examples your talking about where it's initially ambiguous if the command is actually reading or writing the hidden commit (phase, fold --reuse-msg, etc) would default to warn mode and the only negative effect would be the user gets an extra warning message telling them a hash they passed is hidden. Probably not a common occurrence, and not a huge issue. In the future we could add more logic within the command to suppress that warning when doing the read only paths, but for the short term this let's us get directaccess out the door without auditing and updating every commands logic.
- As for adding exception context manager type things, I think one of the issues with our current visibility code is that it requires the person writing a command to be too aware that visibility exists. If we start requiring commands to use this new context appropriately, and to pass the right argument to revrange at the right time, I think we just introduce more risk of commands being written incorrectly and having more complexity at the business logic layer. If we can avoid all of that at the cost of printing warnings to the user slightly more often, I think that's a good trade off.
To summarize: I think a command level flag is a reasonable starting place. We can add more inside-command control later for more precision, but I think we want to avoid it to minimize mental burden on command authors.