D1144: directaccess: add support to export and tests to demonstrate things

[lothiraldan (Boris Feld)]

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