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

[Kostia Balytskyi]

On 4/3/16 11:25 PM, Pierre-Yves David wrote:
>
>
> 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'
Will change to the correct example.

>
> (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 thought that it's best to now allow writing to .hg at all during the
--delete execution. I guess it's relatively save to only lock .hg/store.
>
> 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.
I thought of it more like of a representation of the fact that operation 
will be atomic. Also, checking whether we're not in transaction and then 
*not* acquiring a transaction feels like a race
condition to me.

>
>> +            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.

1. "stripobs" does not seem like a good name to me. What are "obs"? Are 
those obsolete nodes or obsolescense markers? This name (especially, not
prefixed by obsstore) is confusing.
2. can we figure out a more cautious name and leave it in obsstore 
class? The whole point of this class is to isolate work with 
.hg/store/obsstore
and I think it's a bad idea to move such a low-level thing out. My 
proposed "more cautious name" is _deletemarkers(). Would that work?

>
>> +                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,
>