Patchwork D1813: bookmarks: add support to specify hidden revs if directaccess config is set

login
register
mail settings
Submitter phabricator
Date Jan. 5, 2018, 5:14 p.m.
Message ID <differential-rev-PHID-DREV-mhjw77g2tqmtvrxdh74t-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/26569/
State Superseded
Headers show

Comments

phabricator - Jan. 5, 2018, 5:14 p.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This patch adds support to pass hidden revs as `--rev` without using an
  unfiltered repo. Warning is not shown while accessing hidden changeset for
  writing bookmark on it. The behaviour is copied from directaccess extension.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1813

AFFECTED FILES
  mercurial/commands.py
  tests/test-directaccess.t

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 6, 2018, 2:36 a.m.
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.


  Needs some warning since a hidden revision may be revived?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1813

To: pulkit, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
phabricator - Jan. 10, 2018, 12:55 p.m.
yuja requested changes to this revision.
yuja added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> bookmarks.py:856
>          if rev:
> +            warnm = "adding bookmarks to a hidden changeset"
> +            repo = scmutil.unhidehashlikerevs(repo, [rev], 'warn', warnm)

`_("adding ... changeset")`

> bookmarks.py:857
> +            warnm = "adding bookmarks to a hidden changeset"
> +            repo = scmutil.unhidehashlikerevs(repo, [rev], 'warn', warnm)
>              tgt = scmutil.revsingle(repo, rev).node()

Accessing hidden revisions here doesn't mean they will be bookmarked.

Perhaps we should instead warn if `tgt` is a hidden (or obsolete?) revision.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1813

To: pulkit, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
phabricator - Jan. 11, 2018, 12:47 p.m.
yuja requested changes to this revision.
yuja added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> bookmarks.py:859
> +            if ctx.hidden():
> +                repo.ui.warn(_("accessing hidden changeset %s\n") % ctx)
> +            tgt = ctx.node()

Perhaps we can say "bookmarking" or "reviving" instead of "accessing"? I don't know which is better, though.

Another minor problem I found is this warning will be repeated if
you pass more than one bookmark names.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1813

To: pulkit, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
phabricator - Jan. 12, 2018, 1:31 p.m.
yuja added a comment.


  Queued, thanks.

INLINE COMMENTS

> bookmarks.py:853
> +        repo.ui.warn(_("bookmarking hidden changeset %s\n") % \
> +                     (', '.join(hiddenrevs)))
>      marks.applychanges(repo, tr, changes)

FYI, it loops over bookmark names with the same `rev`, so
`hiddenrevs` would have at most one element, and the second
`unhidehashlikerevs()` call would be unnecessary.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1813

To: pulkit, #hg-reviewers, yuja
Cc: yuja, mercurial-devel

Patch

diff --git a/tests/test-directaccess.t b/tests/test-directaccess.t
--- a/tests/test-directaccess.t
+++ b/tests/test-directaccess.t
@@ -186,3 +186,10 @@ 
   abort: hidden revision '2'!
   (use --hidden to access hidden revisions)
   [255]
+
+Setting a bookmark will make that changeset unhidden, so this should come in end
+
+  $ hg bookmarks -r 28ad74 book
+
+  $ hg bookmarks
+     book                      2:28ad74487de9
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -974,6 +974,9 @@ 
     if not names and (delete or rev):
         raise error.Abort(_("bookmark name required"))
 
+    if rev:
+        repo = scmutil.unhidehashlikerevs(repo, [rev], 'nowarn')
+
     if delete or rename or names or inactive:
         with repo.wlock(), repo.lock(), repo.transaction('bookmark') as tr:
             if delete: