[PATCH 1 of 2 v3] commands: allow debugobsolete to delete arbitrary obsmarkers

Pierre-Yves David pierre-yves.david at ens-lyon.org
Sun Apr 3 18:25:48 EDT 2016



On 04/03/2016 07:48 AM, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia at fb.com>
> # Date 1459694510 25200
> #      Sun Apr 03 07:41:50 2016 -0700
> # Node ID 183720bebbe60f248afa277dcfc08468899f5d77
> # Parent  1490e850cffc0103fd42714cf1245a55bcbdc4a6
> commands: allow debugobsolete to delete arbitrary obsmarkers
>
> Sample usage is:
>    '$ hg debugobsolete --delete 0 5'

(small note: The fact we can provide multiple indices was a bit 
surprising to me, but make senses.)

> This is a debug feature that will help people working on evolution and
> obsolescense.
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -3046,6 +3046,7 @@ def debuglocks(ui, repo, **opts):
>             _('record parent information for the precursor')),
>            ('r', 'rev', [], _('display markers relevant to REV')),
>            ('', 'index', False, _('display index of the marker')),
> +         ('', 'delete', [], _('delete markers specified by indices')),

But then the wy you declared the fact we can take multiple of then is 
actually:

   $ hg debugobsolete --delete 0 --delete 5

>           ] + commitopts2,
>            _('[OBSOLETED [REPLACEMENT ...]]'))
>   def debugobsolete(ui, repo, precursor=None, *successors, **opts):
> @@ -3066,6 +3067,32 @@ def debugobsolete(ui, repo, precursor=No
>               raise error.Abort('changeset references must be full hexadecimal '
>                                'node identifiers')
>
> +    if opts.get('delete'):
> +        try:
> +            indices = [int(v) for v in opts.get('delete')]
> +        except ValueError:
> +            raise error.Abort(_('invalid index value'),
> +                              hint=_('use integers fro indices'))

s/fro/for/

> +
> +        if repo.currenttransaction():
> +            raise error.Abort(_('Cannot delete obsmarkers in the middle '
> +                                'of transaction.'))

Drop the initial capital letter. our error messages are lower case (and 
put this in a tmp variable so that it fit in a line)

> +
> +        w = repo.wlock()
> +        l = repo.lock()

I don't think you need wlock here,

And we have context manager for lock now,

> +        try:
> +            tr = repo.transaction('debugobsolete')

You are stripping data from the repository, you can't wrap that inside a 
transaction. Transaction are unable to represent stripping.

> +            try:
> +                n = repo.obsstore.delete(indices)

Actually, let's not have the delete function on the obsstore. Let's call 
it "stripobs" and move it into mercurial.repair. This will emphasize the 
fact it is an exceptional operation.

> +                ui.write(_('Deleted %i obsolescense markers\n') % n)

drop the capital letter here too.

> +                tr.close()
> +            finally:
> +                tr.release()
> +        finally:
> +            l.release()
> +            w.release()
> +        return
> +
>       if precursor is not None:
>           if opts['rev']:
>               raise error.Abort('cannot select revision when creating marker')
> diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
> --- a/mercurial/obsolete.py
> +++ b/mercurial/obsolete.py
> @@ -633,6 +633,30 @@ class obsstore(object):
>           transaction.hookargs['new_obsmarkers'] = str(previous + len(new))
>           return len(new)
>
> +    def delete(self, indices):
> +        """Delete some obsmarkers from store and return the number of them
> +
> +        Indices is a list of ints which are the indices
> +        of the markers to be deleted."""
> +        if not indices:
> +            # we don't want to rewrite the obsstore with the same content
> +            return
> +
> +        left = []
> +        current = self._all
> +        n = 0
> +        for i, m in enumerate(current):
> +            if i in indices:
> +                n += 1
> +                continue
> +            left.append(m)
> +
> +        newobsstore = self.svfs('obsstore', 'w', atomictemp=True)
> +        for bytes in encodemarkers(left, True, self._version):
> +            newobsstore.write(bytes)
> +        newobsstore.close()
> +        return n
> +
>       def mergemarkers(self, transaction, data):
>           """merge a binary stream of markers inside the obsstore
>
> diff --git a/tests/test-completion.t b/tests/test-completion.t
> --- a/tests/test-completion.t
> +++ b/tests/test-completion.t
> @@ -261,7 +261,7 @@ Show all commands + options
>     debuglocks: force-lock, force-wlock
>     debugmergestate:
>     debugnamecomplete:
> -  debugobsolete: flags, record-parents, rev, index, date, user
> +  debugobsolete: flags, record-parents, rev, index, delete, date, user
>     debugpathcomplete: full, normal, added, removed
>     debugpushkey:
>     debugpvec:
> diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
> --- a/tests/test-obsolete.t
> +++ b/tests/test-obsolete.t
> @@ -1083,4 +1083,17 @@ Test ability to pull changeset with loca
>     |
>     @  0:a78f55e5508c (draft) [ ] 0
>
> +  $ cd ..
>
> +Test the --delete option of debugobsolete command
> +  $ hg init dorepo
> +  $ cd dorepo
> +  $ echo a > a && hg ci -Am a
> +  adding a
> +  $ hg ci --amend -m aa
> +  $ hg debugobsolete
> +  cb9a9f314b8b07ba71012fcdbc544b5a4d82ff5b f9bd49731b0b175e42992a3c8fa6c678b2bc11f1 0 (.*) {'user': 'test'} (re)
> +  $ hg debugobsolete --delete 0
> +  Deleted 1 obsolescense markers
> +  $ hg debugobsolete
> +  $ cd ..

we want a more complex test case here, at least:

- More than 1 markers in the obsstore so that we can test we preserve 
the undeleted one,
- markers deleted in the middle of the stack so that we can check we 
don't just truncate,
- deleting multiple markers,

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list