Patchwork [1,of,2] strip: add a delayedstrip method that works in a transaction

login
register
mail settings
Submitter Jun Wu
Date June 25, 2017, 8:32 p.m.
Message ID <b70ebbb5b3ffb417d8fe.1498422768@x1c>
Download mbox | patch
Permalink /patch/21713/
State Accepted
Headers show

Comments

Jun Wu - June 25, 2017, 8:32 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1498412325 25200
#      Sun Jun 25 10:38:45 2017 -0700
# Node ID b70ebbb5b3ffb417d8fe13548b39c9196fadda1f
# Parent  87db210d4a5ade8d5d8b288a0c7fa65b79eedc08
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r b70ebbb5b3ff
strip: add a delayedstrip method that works in a transaction

For long, the fact that strip does not work inside a transaction and some
code has to work with both obsstore and fallback to strip lead to duplicated
code like:

      with repo.transaction():
          ....
          if obsstore:
              obsstore.createmarkers(...)
      if not obsstore:
          repair.strip(...)

Things get more complex when you want to call something which may call strip
under the hood. Like you cannot simply write:

      with repo.transaction():
          ....
          rebasemod.rebase(...) # may call "strip", so this doesn't work

But you do want rebase to run inside a same transaction if possible, so the
code may look like:

      with repo.transaction():
          ....
          if obsstore:
              rebasemod.rebase(...)
              obsstore.createmarkers(...)
      if not obsstore:
          rebasemod.rebase(...)
          repair.strip(...)

That's ugly and error-prone. Ideally it's possible to just write:

      with repo.transaction():
          rebasemod.rebase(...)
          saferemovenodes(...)

This patch is the first step towards that. It adds a "delayedstrip" method
to repair.py which maintains a postclose callback in the transaction object.
via Mercurial-devel - June 26, 2017, 4:26 p.m.
On Sun, Jun 25, 2017 at 1:32 PM, Jun Wu <quark@fb.com> wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1498412325 25200
> #      Sun Jun 25 10:38:45 2017 -0700
> # Node ID b70ebbb5b3ffb417d8fe13548b39c9196fadda1f
> # Parent  87db210d4a5ade8d5d8b288a0c7fa65b79eedc08
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r b70ebbb5b3ff
> strip: add a delayedstrip method that works in a transaction
>
> For long, the fact that strip does not work inside a transaction and some
> code has to work with both obsstore and fallback to strip lead to duplicated
> code like:
>
>       with repo.transaction():
>           ....
>           if obsstore:
>               obsstore.createmarkers(...)
>       if not obsstore:
>           repair.strip(...)
>
> Things get more complex when you want to call something which may call strip
> under the hood. Like you cannot simply write:
>
>       with repo.transaction():
>           ....
>           rebasemod.rebase(...) # may call "strip", so this doesn't work
>
> But you do want rebase to run inside a same transaction if possible, so the
> code may look like:
>
>       with repo.transaction():
>           ....
>           if obsstore:
>               rebasemod.rebase(...)
>               obsstore.createmarkers(...)
>       if not obsstore:
>           rebasemod.rebase(...)
>           repair.strip(...)
>
> That's ugly and error-prone. Ideally it's possible to just write:
>
>       with repo.transaction():
>           rebasemod.rebase(...)
>           saferemovenodes(...)
>
> This patch is the first step towards that. It adds a "delayedstrip" method
> to repair.py which maintains a postclose callback in the transaction object.

Great idea!

>
> diff --git a/mercurial/repair.py b/mercurial/repair.py
> --- a/mercurial/repair.py
> +++ b/mercurial/repair.py
> @@ -254,4 +254,61 @@ def strip(ui, repo, nodelist, backup=Tru
>      return backupfile
>
> +def safestriproots(ui, repo, nodes):
> +    """return list of roots of nodes where descendants are covered by nodes"""
> +    torev = repo.unfiltered().changelog.rev
> +    revs = set(torev(n) for n in nodes)
> +    # tostrip = wanted - unsafe = wanted - ancestors(orphaned)
> +    # orphaned = affected - wanted
> +    # affected = descendants(roots(wanted))
> +    # wanted = revs
> +    tostrip = set(repo.revs('%ld-(::((roots(%ld)::)-%ld))', revs, revs, revs))
> +    notstrip = revs - tostrip
> +    if notstrip:
> +        nodestr = ', '.join(sorted(short(repo[n].node()) for n in notstrip))
> +        ui.warn(_('warning: orphaned descendants detected, '
> +                  'not stripping %s\n') % nodestr)
> +    return [c.node() for c in repo.set('roots(%ld)', tostrip)]
> +
> +class stripcallback(object):
> +    """used as a transaction postclose callback"""
> +
> +    def __init__(self, ui, repo, backup, topic):
> +        self.ui = ui
> +        self.repo = repo
> +        self.backup = backup
> +        self.topic = topic or 'backup'
> +        self.nodelist = []
> +
> +    def addnodes(self, nodes):
> +        self.nodelist.extend(nodes)
> +
> +    def __call__(self, tr):
> +        roots = safestriproots(self.ui, self.repo, self.nodelist)
> +        if roots:
> +            strip(self.ui, self.repo, roots, True, self.topic)
> +
> +def delayedstrip(ui, repo, nodelist, topic=None):
> +    """like strip, but works inside transaction and won't strip irreverent revs

I think you meant s/irreverent/irrelevant/ :-) But to make it clearer,
maybe "descendants" is even better (assuming that's what it is about)?

> +
> +    nodelist must explicitly contain all descendants.

That seems like a safer API in general. Would it be a lot of work to
move this into the normal strip() method?

> Otherwise a warning will
> +    be printed that some nodes are not stripped.

Maybe this should be a ProgrammingError?

> +
> +    Always do a backup.

Why? Is that more necessary for posthook strips than other strips?

>The last non-None "topic" will be used as the backup
> +    topic name. The default backup topic name is "backup".

Why not use a separate backup for each call to delayedstrip()?

> +    """
> +    tr = repo.currenttransaction()
> +    if not tr:
> +        nodes = safestriproots(ui, repo, nodelist)
> +        return strip(ui, repo, nodes, True, topic)
> +    # transaction postclose callbacks are called in alphabet order.
> +    # use '\xff' as prefix so we are likely to be called last.

Why do we want them to run a late as possible? Do we think other hooks
would want the to-be-stripped changesets to still be there when they
run?

> +    callback = tr.getpostclose('\xffstrip')
> +    if callback is None:
> +        callback = stripcallback(ui, repo, True, topic)
> +        tr.addpostclose('\xffstrip', callback)
> +    if topic:
> +        callback.topic = topic
> +    callback.addnodes(nodelist)
> +
>  def striptrees(repo, tr, striprev, files):
>      if 'treemanifest' in repo.requirements: # safe but unnecessary
> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
> --- a/mercurial/transaction.py
> +++ b/mercurial/transaction.py
> @@ -407,5 +407,5 @@ class transaction(object):
>      @active
>      def addpostclose(self, category, callback):
> -        """add a callback to be called after the transaction is closed
> +        """add or replace a callback to be called after the transaction closed
>
>          The transaction will be given as callback's first argument.
> @@ -417,4 +417,9 @@ class transaction(object):
>
>      @active
> +    def getpostclose(self, category):
> +        """return a postclose callback added before, or None"""
> +        return self._postclosecallback.get(category, None)
> +
> +    @active
>      def addabort(self, category, callback):
>          """add a callback to be called when the transaction is aborted.
> diff --git a/tests/test-strip.t b/tests/test-strip.t
> --- a/tests/test-strip.t
> +++ b/tests/test-strip.t
> @@ -3,4 +3,5 @@
>    $ echo "[extensions]" >> $HGRCPATH
>    $ echo "strip=" >> $HGRCPATH
> +  $ echo "drawdag=$TESTDIR/drawdag.py" >> $HGRCPATH
>
>    $ restore() {
> @@ -941,3 +942,51 @@ Error during post-close callback of the
>    [255]
>
> +Use delayedstrip to strip inside a transaction
>
> +  $ cd $TESTTMP
> +  $ hg init delayedstrip
> +  $ cd delayedstrip
> +  $ hg debugdrawdag <<'EOS'
> +  >   D
> +  >   |
> +  >   C F H    # Commit on top of "I",
> +  >   | |/|    # Strip B+D+I+E+G+H+Z
> +  > I B E G
> +  >  \|/
> +  >   A   Z
> +  > EOS
> +
> +  $ hg up -C I
> +  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ echo 3 >> I
> +  $ cat > $TESTTMP/delayedstrip.py <<EOF
> +  > from mercurial import repair, commands
> +  > def reposetup(ui, repo):
> +  >     def getnodes(expr):
> +  >         return [repo.changelog.node(r) for r in repo.revs(expr)]
> +  >     with repo.wlock():
> +  >         with repo.lock():
> +  >             with repo.transaction('delayedstrip'):
> +  >                 repair.delayedstrip(ui, repo, getnodes('B+I+Z+D+E'), 'J')
> +  >                 repair.delayedstrip(ui, repo, getnodes('G+H+Z'), 'I')
> +  >                 commands.commit(ui, repo, message='J', date='0 0')
> +  > EOF
> +  $ hg log -r . -T '\n' --config extensions.t=$TESTTMP/delayedstrip.py
> +  warning: orphaned descendants detected, not stripping 08ebfeb61bac, 112478962961, 7fb047a69f22
> +  saved backup bundle to $TESTTMP/delayedstrip/.hg/strip-backup/f585351a92f8-81fa23b0-I.hg (glob)
> +
> +  $ hg log -G -T '{rev}:{node|short} {desc}' -r 'sort(all(), topo)'
> +  @  6:2f2d51af6205 J
> +  |
> +  o  3:08ebfeb61bac I
> +  |
> +  | o  5:64a8289d2492 F
> +  | |
> +  | o  2:7fb047a69f22 E
> +  |/
> +  | o  4:26805aba1e60 C
> +  | |
> +  | o  1:112478962961 B
> +  |/
> +  o  0:426bada5c675 A
> +
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Jun Wu - June 26, 2017, 11:33 p.m.
Excerpts from Martin von Zweigbergk's message of 2017-06-26 09:26:56 -0700:
> [...]
> > +def delayedstrip(ui, repo, nodelist, topic=None):
> > +    """like strip, but works inside transaction and won't strip irreverent revs
> 
> I think you meant s/irreverent/irrelevant/ :-) But to make it clearer,
> maybe "descendants" is even better (assuming that's what it is about)?

Oops. I will fix the spelling as long as update the docstring.

The language is a bit tricker to write, it's not stripping descendants and
it won't remove ancestors of those descendants. If nodelist contains A and B
where B is a descendant of A, it's still possible to remove B, or A+B, or
nothing.

Maybe something like:

  won't strip anything that cause nodes outside nodelist to disappear

> > +
> > +    nodelist must explicitly contain all descendants.
> 
> That seems like a safer API in general. Would it be a lot of work to
> move this into the normal strip() method?

That is also what I have been considering. It seems to me that "strip()" is
a lower level function and we might want a low level function any way.

Maybe rename "strip" to "_strip" and make "strip" do the extra safe check?
I'll do that if it's simple.

> > Otherwise a warning will
> > +    be printed that some nodes are not stripped.
> 
> Maybe this should be a ProgrammingError?

No. This is not an error intentionally. It makes the API easier to use and
more flexible. For example,

  1. When rebaseset has a "hole", rebase won't need special handling before
     sending the rebaseset to delayedstrip.
  2. Even if you know that nodelist covers all descendants at the time you
     call "delayedstrip", because it's delayed, somebody else could still
     change the repo, and at the end of transaction, the original nodelist
     may have surprising new children and shouldn't be stripped.

I think supporting "2" is important. There is a test case for that (commit
after delayedstrip).

> > +    Always do a backup.
> 
> Why? Is that more necessary for posthook strips than other strips?

All the code needing to migrate to this code path does a backup.

> >The last non-None "topic" will be used as the backup
> > +    topic name. The default backup topic name is "backup".
> 
> Why not use a separate backup for each call to delayedstrip()?

For example, given the following simple DAG:

    B D
    | |
    A C

And somebody calls:

    delayedstrip(nodelist=['A', 'D'], topic='AD')
    delayedstrip(nodelist=['B', 'C'], topic='BC')

What do you think is the best behavior with that?

> > +    # transaction postclose callbacks are called in alphabet order.
> > +    # use '\xff' as prefix so we are likely to be called last.
> 
> Why do we want them to run a late as possible? Do we think other hooks
> would want the to-be-stripped changesets to still be there when they
> run?

Just to be safe in case some other postclose callbacks do crazy things (like
os.system("hg commit -m X")). They shouldn't but there is no easy way to
detect what other postclose callbacks will do, so I'd prefer safer by
default.
Jun Wu - June 27, 2017, 12:04 a.m.
Excerpts from Jun Wu's message of 2017-06-26 16:33:10 -0700:
> > Why do we want them to run a late as possible? Do we think other hooks
> > would want the to-be-stripped changesets to still be there when they
> > run?
> 
> Just to be safe in case some other postclose callbacks do crazy things
> (like os.system("hg commit -m X")). They shouldn't but there is no easy
> way to detect what other postclose callbacks will do, so I'd prefer safer
> by default.

Sorry, I misunderstood your original comment. After a second look at the
existing code, I think it's actually better to run strip as early as
possible so callbacks like warm-caches will get an up-to-date view.  I'll
send follow-ups.
via Mercurial-devel - June 27, 2017, 12:08 a.m.
On Mon, Jun 26, 2017 at 4:33 PM, Jun Wu <quark@fb.com> wrote:
> Excerpts from Martin von Zweigbergk's message of 2017-06-26 09:26:56 -0700:
>> [...]
>> > +def delayedstrip(ui, repo, nodelist, topic=None):
>> > +    """like strip, but works inside transaction and won't strip irreverent revs
>>
>> I think you meant s/irreverent/irrelevant/ :-) But to make it clearer,
>> maybe "descendants" is even better (assuming that's what it is about)?
>
> Oops. I will fix the spelling as long as update the docstring.
>
> The language is a bit tricker to write, it's not stripping descendants and
> it won't remove ancestors of those descendants. If nodelist contains A and B
> where B is a descendant of A, it's still possible to remove B, or A+B, or
> nothing.
>
> Maybe something like:
>
>   won't strip anything that cause nodes outside nodelist to disappear
>
>> > +
>> > +    nodelist must explicitly contain all descendants.
>>
>> That seems like a safer API in general. Would it be a lot of work to
>> move this into the normal strip() method?
>
> That is also what I have been considering. It seems to me that "strip()" is
> a lower level function and we might want a low level function any way.
>
> Maybe rename "strip" to "_strip" and make "strip" do the extra safe check?
> I'll do that if it's simple.

That's what I had been thinking. I just assumed you wanted to migrate
all users first and that that's the only reason you didn't do it right
away.

>
>> > Otherwise a warning will
>> > +    be printed that some nodes are not stripped.
>>
>> Maybe this should be a ProgrammingError?
>
> No. This is not an error intentionally. It makes the API easier to use and
> more flexible. For example,
>
>   1. When rebaseset has a "hole", rebase won't need special handling before
>      sending the rebaseset to delayedstrip.

I thought we decided that that that shouldn't happen. Aborting in that
case instead of making the rebase complete successfully seems fine to
me (maybe even better, I'm not sure).

>   2. Even if you know that nodelist covers all descendants at the time you
>      call "delayedstrip", because it's delayed, somebody else could still
>      change the repo, and at the end of transaction, the original nodelist
>      may have surprising new children and shouldn't be stripped.

That's the case I would like to catch by using ProgrammingError.

>
> I think supporting "2" is important. There is a test case for that (commit
> after delayedstrip).
>
>> > +    Always do a backup.
>>
>> Why? Is that more necessary for posthook strips than other strips?
>
> All the code needing to migrate to this code path does a backup.

Like I said above, I think I'd prefer delayedstrip() to subsume
strip() and take its name. So you will add the backup flag then
instead? Relatedly, stripcallback's backup option is currently
ignored.

>
>> >The last non-None "topic" will be used as the backup
>> > +    topic name. The default backup topic name is "backup".
>>
>> Why not use a separate backup for each call to delayedstrip()?
>
> For example, given the following simple DAG:
>
>     B D
>     | |
>     A C
>
> And somebody calls:
>
>     delayedstrip(nodelist=['A', 'D'], topic='AD')
>     delayedstrip(nodelist=['B', 'C'], topic='BC')
>
> What do you think is the best behavior with that?

Good point. The topic could also be the concatenation of the topics
(maybe joined by a '+' or something). Picking the last seemed
arbitrary. Anyway, not very important.

>
>> > +    # transaction postclose callbacks are called in alphabet order.
>> > +    # use '\xff' as prefix so we are likely to be called last.
>>
>> Why do we want them to run a late as possible? Do we think other hooks
>> would want the to-be-stripped changesets to still be there when they
>> run?
>
> Just to be safe in case some other postclose callbacks do crazy things (like
> os.system("hg commit -m X")). They shouldn't but there is no easy way to
> detect what other postclose callbacks will do, so I'd prefer safer by
> default.

I agree that it makes sense to run it last *or* first (otherwise some
hooks would see the commits and some wouldn't). I was wondering if
first would make as much sense. It's unclear to me. Update: I saw you
answered this :-)
Jun Wu - June 27, 2017, 12:33 a.m.
Excerpts from Martin von Zweigbergk's message of 2017-06-26 17:08:03 -0700:
> >> > Otherwise a warning will
> >> > +    be printed that some nodes are not stripped.
> >>
> >> Maybe this should be a ProgrammingError?
> >
> > No. This is not an error intentionally. It makes the API easier to use and
> > more flexible. For example,
> >
> >   1. When rebaseset has a "hole", rebase won't need special handling before
> >      sending the rebaseset to delayedstrip.
> 
> I thought we decided that that that shouldn't happen. Aborting in that
> case instead of making the rebase complete successfully seems fine to
> me (maybe even better, I'm not sure).

I think our conclusion is "it is rare", and "it's a best effort to support".

If we abort, what do you think about the "hg rebase --continue" case in
issue5606? Should we *enforce* the user to turn on evolve to continue?
I don't think that is a good idea.

Also, I'd like to be able to reuse the cleanup API in "absorb". If we raise
ProgrammingError, it will break a valid use-case of a Mozilla "hg absorb"
developer [1].

[1]: https://bitbucket.org/facebook/hg-experimental/issues/6/hg-absorb-merges-diverged-commits

> >   2. Even if you know that nodelist covers all descendants at the time you
> >      call "delayedstrip", because it's delayed, somebody else could still
> >      change the repo, and at the end of transaction, the original nodelist
> >      may have surprising new children and shouldn't be stripped.
> 
> That's the case I would like to catch by using ProgrammingError.

If it's ProgrammingError, which programmer made errors? The commit one or
the strip one, or the one putting them together?

For the commit one or the rebase one, they should not be aware of what could
happen in the rest of the same transaction - they are just making reusable
sub-routines. So it can only be the one putting them together.

Suppose I write the code:

    with repo.transaction():
        rebase(....)
        edit-repo(....)
        edit-repo2(....)
 
In order to avoid ProgrammingError, I must know the implementation details
of "rebase", "edit-repo", "edit-repo2" to do the check, or access their
internal states. That seems like a layer violation to me. And is probably
also fragile - even if I get all the new and stripped nodes from all
subroutines, writing correct code to avoid the ProgrammingError is still
non-trivial. Definitely not friendly to new developers.

> > I think supporting "2" is important. There is a test case for that (commit
> > after delayedstrip).
> >
> >> > +    Always do a backup.
> >>
> >> Why? Is that more necessary for posthook strips than other strips?
> >
> > All the code needing to migrate to this code path does a backup.
> 
> Like I said above, I think I'd prefer delayedstrip() to subsume
> strip() and take its name. So you will add the backup flag then
> instead? Relatedly, stripcallback's backup option is currently
> ignored.


> 
> >
> >> >The last non-None "topic" will be used as the backup
> >> > +    topic name. The default backup topic name is "backup".
> >>
> >> Why not use a separate backup for each call to delayedstrip()?
> >
> > For example, given the following simple DAG:
> >
> >     B D
> >     | |
> >     A C
> >
> > And somebody calls:
> >
> >     delayedstrip(nodelist=['A', 'D'], topic='AD')
> >     delayedstrip(nodelist=['B', 'C'], topic='BC')
> >
> > What do you think is the best behavior with that?
> 
> Good point. The topic could also be the concatenation of the topics
> (maybe joined by a '+' or something). Picking the last seemed
> arbitrary. Anyway, not very important.

Practically, the current design allows people to reuse functions that calls
"delayedstrip" and change its topic name. For example:

    with transaction('split'):
        ...
        rebase(...) # calls delayedstrip(topic='rebase')
        delayedstrip(topic='split')

I think it's cleaner if backup name is "x-split.hg" instead of
"x-rebase+split.hg". But I don't have strong feeling.

Handling backup=True/False will make the API more complex than desired. I
don't really have a clean way to deal with that. Take the above example, it
could also be:

    delayedstrip(nodelist=['A', 'D'], backup=True)
    delayedstrip(nodelist=['B', 'C'], backup=False)

Therefore I don't think "delayedstrip" should replace "strip", the word
"delayed" is there for a reason. But I agree it's nicer if "strip" does the
orphan check. So the change will look like:

    def strip(....):
        roots = safestriproots(nodelist)
        _strip(roots)
          
    def _strip(....):
        # the original "strip"

> [...]
> 
> I agree that it makes sense to run it last *or* first (otherwise some
> hooks would see the commits and some wouldn't). I was wondering if
> first would make as much sense. It's unclear to me. Update: I saw you
> answered this :-)
Jun Wu - June 27, 2017, 4:23 a.m.
Excerpts from Jun Wu's message of 2017-06-26 17:33:26 -0700:
> Handling backup=True/False will make the API more complex than desired. I
> don't really have a clean way to deal with that. Take the above example, it
> could also be:
> 
>     delayedstrip(nodelist=['A', 'D'], backup=True)
>     delayedstrip(nodelist=['B', 'C'], backup=False)
> 
> Therefore I don't think "delayedstrip" should replace "strip", the word
> "delayed" is there for a reason. But I agree it's nicer if "strip" does the
> orphan check. So the change will look like:
> 
>     def strip(....):
>         roots = safestriproots(nodelist)
>         _strip(roots)
>           
>     def _strip(....):
>         # the original "strip"

After thinking it again, the "backup" parameter issue is not a big deal. We
can do something like "the last non-None value is respected".

And it seems there is no valid use cases where strip has to run explicitly
outside a transaction. So replacing "strip" makes sense to me.

I'll try to migrate the existing code to the new strip function. Thanks for
the idea!


That said, I still insist that strip warning is better than ProgrammingError
since test-rebase-interruptions.t has a case where the error maker is a user
running "hg commit", not a programmer :)
via Mercurial-devel - June 27, 2017, 4:33 a.m.
On Mon, Jun 26, 2017 at 5:33 PM, Jun Wu <quark@fb.com> wrote:
> Excerpts from Martin von Zweigbergk's message of 2017-06-26 17:08:03 -0700:
>> >> > Otherwise a warning will
>> >> > +    be printed that some nodes are not stripped.
>> >>
>> >> Maybe this should be a ProgrammingError?
>> >
>> > No. This is not an error intentionally. It makes the API easier to use and
>> > more flexible. For example,
>> >
>> >   1. When rebaseset has a "hole", rebase won't need special handling before
>> >      sending the rebaseset to delayedstrip.
>>
>> I thought we decided that that that shouldn't happen. Aborting in that
>> case instead of making the rebase complete successfully seems fine to
>> me (maybe even better, I'm not sure).
>
> I think our conclusion is "it is rare", and "it's a best effort to support".

Yes, agreed. Sorry, I mean to clarify that, but I was typing it out
quickly before leaving work.

>
> If we abort, what do you think about the "hg rebase --continue" case in
> issue5606? Should we *enforce* the user to turn on evolve to continue?
> I don't think that is a good idea.

Yes, either turn on evolve or abort the rebase (aborting the rebase
was what I was thinking while writing the paragraph above, but
continuing with evolve on should also be a solution). This is such a
corner case that I don't care much.

>
> Also, I'd like to be able to reuse the cleanup API in "absorb". If we raise
> ProgrammingError, it will break a valid use-case of a Mozilla "hg absorb"
> developer [1].
>
> [1]: https://bitbucket.org/facebook/hg-experimental/issues/6/hg-absorb-merges-diverged-commits
>

I spent a few minutes, but I couldn't understand how it would break.

>> >   2. Even if you know that nodelist covers all descendants at the time you
>> >      call "delayedstrip", because it's delayed, somebody else could still
>> >      change the repo, and at the end of transaction, the original nodelist
>> >      may have surprising new children and shouldn't be stripped.
>>
>> That's the case I would like to catch by using ProgrammingError.
>
> If it's ProgrammingError, which programmer made errors? The commit one or
> the strip one, or the one putting them together?
>
> For the commit one or the rebase one, they should not be aware of what could
> happen in the rest of the same transaction - they are just making reusable
> sub-routines. So it can only be the one putting them together.
>
> Suppose I write the code:
>
>     with repo.transaction():
>         rebase(....)
>         edit-repo(....)
>         edit-repo2(....)
>
> In order to avoid ProgrammingError, I must know the implementation details
> of "rebase", "edit-repo", "edit-repo2" to do the check, or access their
> internal states. That seems like a layer violation to me. And is probably
> also fragile - even if I get all the new and stripped nodes from all
> subroutines, writing correct code to avoid the ProgrammingError is still
> non-trivial. Definitely not friendly to new developers.

I'm not sure, but it's probably the person writing the "hg
rebase-and-commit" command made the mistake. To me, just looking at
the result from the user's perspective makes it clearly wrong. If the
rebasing leaves both an old version and a new version in the repo, I
would definitely say that there's a bug in the command.

While writing this, I saw your other reply about users also producing
commits on top. That's an interesting case, and I'm glad we have a
test case for it. However, I don't see how that case will actually
trigger the ProgrammingError I'm suggesting. The ProgrammingError
would happen in the postclose hook, which happens after the
transaction close, but still before the repo lock is released, right?
If that's the case, I don't see how the user could create a commit in
the meantime.

>
>> > I think supporting "2" is important. There is a test case for that (commit
>> > after delayedstrip).
>> >
>> >> > +    Always do a backup.
>> >>
>> >> Why? Is that more necessary for posthook strips than other strips?
>> >
>> > All the code needing to migrate to this code path does a backup.
>>
>> Like I said above, I think I'd prefer delayedstrip() to subsume
>> strip() and take its name. So you will add the backup flag then
>> instead? Relatedly, stripcallback's backup option is currently
>> ignored.
>
>
>>
>> >
>> >> >The last non-None "topic" will be used as the backup
>> >> > +    topic name. The default backup topic name is "backup".
>> >>
>> >> Why not use a separate backup for each call to delayedstrip()?
>> >
>> > For example, given the following simple DAG:
>> >
>> >     B D
>> >     | |
>> >     A C
>> >
>> > And somebody calls:
>> >
>> >     delayedstrip(nodelist=['A', 'D'], topic='AD')
>> >     delayedstrip(nodelist=['B', 'C'], topic='BC')
>> >
>> > What do you think is the best behavior with that?
>>
>> Good point. The topic could also be the concatenation of the topics
>> (maybe joined by a '+' or something). Picking the last seemed
>> arbitrary. Anyway, not very important.
>
> Practically, the current design allows people to reuse functions that calls
> "delayedstrip" and change its topic name. For example:
>
>     with transaction('split'):
>         ...
>         rebase(...) # calls delayedstrip(topic='rebase')
>         delayedstrip(topic='split')
>
> I think it's cleaner if backup name is "x-split.hg" instead of
> "x-rebase+split.hg". But I don't have strong feeling.
>
> Handling backup=True/False will make the API more complex than desired. I
> don't really have a clean way to deal with that. Take the above example, it
> could also be:
>
>     delayedstrip(nodelist=['A', 'D'], backup=True)
>     delayedstrip(nodelist=['B', 'C'], backup=False)

I saw your other email about finding a way to make that work. Even if
you didn't, the case seems so obscure that I wouldn't mind losing
support for it.

Let me know if it's not actually as obscure as it seems; i.e. I'd be
curious to hear of a useful command that might produce that.

>
> Therefore I don't think "delayedstrip" should replace "strip", the word
> "delayed" is there for a reason. But I agree it's nicer if "strip" does the
> orphan check. So the change will look like:
>
>     def strip(....):
>         roots = safestriproots(nodelist)
>         _strip(roots)
>
>     def _strip(....):
>         # the original "strip"
>
>> [...]
>>
>> I agree that it makes sense to run it last *or* first (otherwise some
>> hooks would see the commits and some wouldn't). I was wondering if
>> first would make as much sense. It's unclear to me. Update: I saw you
>> answered this :-)
Jun Wu - June 27, 2017, 5:38 p.m.
Excerpts from Martin von Zweigbergk's message of 2017-06-26 21:33:50 -0700:
> >
> > If we abort, what do you think about the "hg rebase --continue" case in
> > issue5606? Should we *enforce* the user to turn on evolve to continue?
> > I don't think that is a good idea.
> 
> Yes, either turn on evolve or abort the rebase (aborting the rebase
> was what I was thinking while writing the paragraph above, but
> continuing with evolve on should also be a solution). This is such a
> corner case that I don't care much.

I think the key disagreement is whether should we "allowunstable" when
obsstore is disabled. I think for power users, if they explicitly ask for,
we should continue.

> > [1]: https://bitbucket.org/facebook/hg-experimental/issues/6/hg-absorb-merges-diverged-commits 
> 
> I spent a few minutes, but I couldn't understand how it would break.

It's similar to amend a non-head with obsstore disabled. He is a power user
that wants advanced stack editing without obsstore.

> I'm not sure, but it's probably the person writing the "hg
> rebase-and-commit" command made the mistake. To me, just looking at
> the result from the user's perspective makes it clearly wrong. If the
> rebasing leaves both an old version and a new version in the repo, I
> would definitely say that there's a bug in the command.

How about:

  1. Add a way to allowunstable without evolve, maybe call it
     "allowduplicates" for power users that I care about.
  2. ProgrammingError if allowduplicates is not set (which is the default),
     but continue with warning if allowduplicates is set.

> While writing this, I saw your other reply about users also producing
> commits on top. That's an interesting case, and I'm glad we have a
> test case for it. However, I don't see how that case will actually
> trigger the ProgrammingError I'm suggesting. The ProgrammingError
> would happen in the postclose hook, which happens after the
> transaction close, but still before the repo lock is released, right?
> If that's the case, I don't see how the user could create a commit in
> the meantime.

The rebase was interrupted because of merge conflicts. So the user can
modify the DAG during the time they were supposed to fix conflicts. I guess
you might want to make the repo read-only for non-rebase commands if
.hg/rebasestate exists.

> I saw your other email about finding a way to make that work. Even if
> you didn't, the case seems so obscure that I wouldn't mind losing
> support for it.
> 
> Let me know if it's not actually as obscure as it seems; i.e. I'd be
> curious to hear of a useful command that might produce that.

I think a reasonable path to move ahead is:

  1. Add the allowduplicates config for power users
  2. Set that config to True in test-rebase-interruptions.t
  3. In strip code, raise ProgrammingError if allowduplicates is not set,
     continue with warning otherwise.
  4. Make repo read-only for non-rebase commands when .hg/rebasestate exists
     and allowduplicates is not set.

I can complete 1, 2, 3. 4 is not easy and is not of my main focus, so I'll
leave it for grabs.

What do you think?
via Mercurial-devel - June 27, 2017, 8:16 p.m.
On Tue, Jun 27, 2017 at 10:38 AM, Jun Wu <quark@fb.com> wrote:
> Excerpts from Martin von Zweigbergk's message of 2017-06-26 21:33:50 -0700:
>> >
>> > If we abort, what do you think about the "hg rebase --continue" case in
>> > issue5606? Should we *enforce* the user to turn on evolve to continue?
>> > I don't think that is a good idea.
>>
>> Yes, either turn on evolve or abort the rebase (aborting the rebase
>> was what I was thinking while writing the paragraph above, but
>> continuing with evolve on should also be a solution). This is such a
>> corner case that I don't care much.
>
> I think the key disagreement is whether should we "allowunstable" when
> obsstore is disabled. I think for power users, if they explicitly ask for,
> we should continue.

I think allowduplicates is quite different from allowunstable. We can
automate resolution of unstable commits, but we can't automate
resolution of duplicates. I'm still convinced it's a bug in a the
command that creates the duplicate commit (unless that's the command's
purpose, of course, but then it really shouldn't attempt to strip one
of the commits).

>
>> > [1]: https://bitbucket.org/facebook/hg-experimental/issues/6/hg-absorb-merges-diverged-commits
>>
>> I spent a few minutes, but I couldn't understand how it would break.
>
> It's similar to amend a non-head with obsstore disabled. He is a power user
> that wants advanced stack editing without obsstore.
>
>> I'm not sure, but it's probably the person writing the "hg
>> rebase-and-commit" command made the mistake. To me, just looking at
>> the result from the user's perspective makes it clearly wrong. If the
>> rebasing leaves both an old version and a new version in the repo, I
>> would definitely say that there's a bug in the command.
>
> How about:
>
>   1. Add a way to allowunstable without evolve, maybe call it
>      "allowduplicates" for power users that I care about.
>   2. ProgrammingError if allowduplicates is not set (which is the default),
>      but continue with warning if allowduplicates is set.

As I said above, I don't think 1 is a good idea. Maybe I'm just being
stubborn (I encourage a third party to step in and say that if that's
the case), but I'd really prefer to make it just a ProgrammingError.
If we ever come up with a valid use case, we can add add the
allowduplicates config then (we'll know, because the user/developer
ran into the exception).

>
>> While writing this, I saw your other reply about users also producing
>> commits on top. That's an interesting case, and I'm glad we have a
>> test case for it. However, I don't see how that case will actually
>> trigger the ProgrammingError I'm suggesting. The ProgrammingError
>> would happen in the postclose hook, which happens after the
>> transaction close, but still before the repo lock is released, right?
>> If that's the case, I don't see how the user could create a commit in
>> the meantime.
>
> The rebase was interrupted because of merge conflicts. So the user can
> modify the DAG during the time they were supposed to fix conflicts. I guess
> you might want to make the repo read-only for non-rebase commands if
> .hg/rebasestate exists.

I understand that the user can add commits, and I suppose that's
covered by that test-rebase-interruptions.t. However, I don't see how
they can trigger the ProgrammingError.

>
>> I saw your other email about finding a way to make that work. Even if
>> you didn't, the case seems so obscure that I wouldn't mind losing
>> support for it.
>>
>> Let me know if it's not actually as obscure as it seems; i.e. I'd be
>> curious to hear of a useful command that might produce that.
>
> I think a reasonable path to move ahead is:
>
>   1. Add the allowduplicates config for power users
>   2. Set that config to True in test-rebase-interruptions.t
>   3. In strip code, raise ProgrammingError if allowduplicates is not set,
>      continue with warning otherwise.
>   4. Make repo read-only for non-rebase commands when .hg/rebasestate exists
>      and allowduplicates is not set.
>
> I can complete 1, 2, 3.

See above.

> 4 is not easy and is not of my main focus, so I'll
> leave it for grabs.

Sure, 4 is definitely out of scope.

>
> What do you think?
Jun Wu - June 27, 2017, 8:46 p.m.
Excerpts from Martin von Zweigbergk's message of 2017-06-27 13:16:17 -0700:
> I think allowduplicates is quite different from allowunstable. We can
> automate resolution of unstable commits, but we can't automate
> resolution of duplicates. I'm still convinced it's a bug in a the
> command that creates the duplicate commit (unless that's the command's
> purpose, of course, but then it really shouldn't attempt to strip one
> of the commits).

That's why it's "power user" only. I agree that the default behavior could
be abort. But I think it's reasonable to make it possible for power users to
not abort. If you think nobody is such power user, then my question is, why
not remove "--force" from "hg phase" ? since that also helps create
duplicated changesets indirectly.

> I understand that the user can add commits, and I suppose that's
> covered by that test-rebase-interruptions.t. However, I don't see how
> they can trigger the ProgrammingError.

The current code without modification will. I guess you mean moving the
unstable check to "rebase --continue" code path. That is a BC to
test-rebase-interruptions.t that I'd prefer not to break.

At this point, I'd also like a third party to jump in. Since my silly
"allowduplicates" seems bad, and I don't want to break
test-rebase-interruptions.t, I'll leave the code as is. Feel free to send
patches and start a new discussion if you see fit.
Augie Fackler - June 27, 2017, 9:01 p.m.
On Tue, Jun 27, 2017 at 01:16:17PM -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> On Tue, Jun 27, 2017 at 10:38 AM, Jun Wu <quark@fb.com> wrote:
> > Excerpts from Martin von Zweigbergk's message of 2017-06-26 21:33:50 -0700:
> >> >
> >> > If we abort, what do you think about the "hg rebase --continue" case in
> >> > issue5606? Should we *enforce* the user to turn on evolve to continue?
> >> > I don't think that is a good idea.
> >>
> >> Yes, either turn on evolve or abort the rebase (aborting the rebase
> >> was what I was thinking while writing the paragraph above, but
> >> continuing with evolve on should also be a solution). This is such a
> >> corner case that I don't care much.
> >
> > I think the key disagreement is whether should we "allowunstable" when
> > obsstore is disabled. I think for power users, if they explicitly ask for,
> > we should continue.
>
> I think allowduplicates is quite different from allowunstable. We can
> automate resolution of unstable commits, but we can't automate
> resolution of duplicates. I'm still convinced it's a bug in a the
> command that creates the duplicate commit (unless that's the command's
> purpose, of course, but then it really shouldn't attempt to strip one
> of the commits).
>
> >
> >> > [1]: https://bitbucket.org/facebook/hg-experimental/issues/6/hg-absorb-merges-diverged-commits
> >>
> >> I spent a few minutes, but I couldn't understand how it would break.
> >
> > It's similar to amend a non-head with obsstore disabled. He is a power user
> > that wants advanced stack editing without obsstore.
> >
> >> I'm not sure, but it's probably the person writing the "hg
> >> rebase-and-commit" command made the mistake. To me, just looking at
> >> the result from the user's perspective makes it clearly wrong. If the
> >> rebasing leaves both an old version and a new version in the repo, I
> >> would definitely say that there's a bug in the command.
> >
> > How about:
> >
> >   1. Add a way to allowunstable without evolve, maybe call it
> >      "allowduplicates" for power users that I care about.
> >   2. ProgrammingError if allowduplicates is not set (which is the default),
> >      but continue with warning if allowduplicates is set.
>
> As I said above, I don't think 1 is a good idea. Maybe I'm just being
> stubborn (I encourage a third party to step in and say that if that's
> the case), but I'd really prefer to make it just a ProgrammingError.
> If we ever come up with a valid use case, we can add add the
> allowduplicates config then (we'll know, because the user/developer
> ran into the exception).

I think it'd be fine to add a TODO that we should look for cases where
this is valid and try to constrain it - my gut reaction is that Jun is
right, and that it can't be a programming error if we ever allow
instability (which is sometimes useful for power users, and might be
useful if we get a little more smarts around defering stabilization of
changes.)

Does that make sense? Or have I not quite understood this conversation?
via Mercurial-devel - June 27, 2017, 11:06 p.m.
On Tue, Jun 27, 2017 at 2:01 PM, Augie Fackler <raf@durin42.com> wrote:
> On Tue, Jun 27, 2017 at 01:16:17PM -0700, Martin von Zweigbergk via Mercurial-devel wrote:
>> On Tue, Jun 27, 2017 at 10:38 AM, Jun Wu <quark@fb.com> wrote:
>> > Excerpts from Martin von Zweigbergk's message of 2017-06-26 21:33:50 -0700:
>> >> >
>> >> > If we abort, what do you think about the "hg rebase --continue" case in
>> >> > issue5606? Should we *enforce* the user to turn on evolve to continue?
>> >> > I don't think that is a good idea.
>> >>
>> >> Yes, either turn on evolve or abort the rebase (aborting the rebase
>> >> was what I was thinking while writing the paragraph above, but
>> >> continuing with evolve on should also be a solution). This is such a
>> >> corner case that I don't care much.
>> >
>> > I think the key disagreement is whether should we "allowunstable" when
>> > obsstore is disabled. I think for power users, if they explicitly ask for,
>> > we should continue.
>>
>> I think allowduplicates is quite different from allowunstable. We can
>> automate resolution of unstable commits, but we can't automate
>> resolution of duplicates. I'm still convinced it's a bug in a the
>> command that creates the duplicate commit (unless that's the command's
>> purpose, of course, but then it really shouldn't attempt to strip one
>> of the commits).
>>
>> >
>> >> > [1]: https://bitbucket.org/facebook/hg-experimental/issues/6/hg-absorb-merges-diverged-commits
>> >>
>> >> I spent a few minutes, but I couldn't understand how it would break.
>> >
>> > It's similar to amend a non-head with obsstore disabled. He is a power user
>> > that wants advanced stack editing without obsstore.
>> >
>> >> I'm not sure, but it's probably the person writing the "hg
>> >> rebase-and-commit" command made the mistake. To me, just looking at
>> >> the result from the user's perspective makes it clearly wrong. If the
>> >> rebasing leaves both an old version and a new version in the repo, I
>> >> would definitely say that there's a bug in the command.
>> >
>> > How about:
>> >
>> >   1. Add a way to allowunstable without evolve, maybe call it
>> >      "allowduplicates" for power users that I care about.
>> >   2. ProgrammingError if allowduplicates is not set (which is the default),
>> >      but continue with warning if allowduplicates is set.
>>
>> As I said above, I don't think 1 is a good idea. Maybe I'm just being
>> stubborn (I encourage a third party to step in and say that if that's
>> the case), but I'd really prefer to make it just a ProgrammingError.
>> If we ever come up with a valid use case, we can add add the
>> allowduplicates config then (we'll know, because the user/developer
>> ran into the exception).
>
> I think it'd be fine to add a TODO that we should look for cases where
> this is valid and try to constrain it - my gut reaction is that Jun is
> right, and that it can't be a programming error if we ever allow
> instability (which is sometimes useful for power users, and might be
> useful if we get a little more smarts around defering stabilization of
> changes.)
>
> Does that make sense? Or have I not quite understood this conversation?

I'm not convinced either way, but let's just drop this for now. Let's
instead get the other patches that add callers of this method in.
Then, when that's done, we can revisit this. It should be clearer then
if we have any legitimate cases where the warning happens.

Patch

diff --git a/mercurial/repair.py b/mercurial/repair.py
--- a/mercurial/repair.py
+++ b/mercurial/repair.py
@@ -254,4 +254,61 @@  def strip(ui, repo, nodelist, backup=Tru
     return backupfile
 
+def safestriproots(ui, repo, nodes):
+    """return list of roots of nodes where descendants are covered by nodes"""
+    torev = repo.unfiltered().changelog.rev
+    revs = set(torev(n) for n in nodes)
+    # tostrip = wanted - unsafe = wanted - ancestors(orphaned)
+    # orphaned = affected - wanted
+    # affected = descendants(roots(wanted))
+    # wanted = revs
+    tostrip = set(repo.revs('%ld-(::((roots(%ld)::)-%ld))', revs, revs, revs))
+    notstrip = revs - tostrip
+    if notstrip:
+        nodestr = ', '.join(sorted(short(repo[n].node()) for n in notstrip))
+        ui.warn(_('warning: orphaned descendants detected, '
+                  'not stripping %s\n') % nodestr)
+    return [c.node() for c in repo.set('roots(%ld)', tostrip)]
+
+class stripcallback(object):
+    """used as a transaction postclose callback"""
+
+    def __init__(self, ui, repo, backup, topic):
+        self.ui = ui
+        self.repo = repo
+        self.backup = backup
+        self.topic = topic or 'backup'
+        self.nodelist = []
+
+    def addnodes(self, nodes):
+        self.nodelist.extend(nodes)
+
+    def __call__(self, tr):
+        roots = safestriproots(self.ui, self.repo, self.nodelist)
+        if roots:
+            strip(self.ui, self.repo, roots, True, self.topic)
+
+def delayedstrip(ui, repo, nodelist, topic=None):
+    """like strip, but works inside transaction and won't strip irreverent revs
+
+    nodelist must explicitly contain all descendants. Otherwise a warning will
+    be printed that some nodes are not stripped.
+
+    Always do a backup. The last non-None "topic" will be used as the backup
+    topic name. The default backup topic name is "backup".
+    """
+    tr = repo.currenttransaction()
+    if not tr:
+        nodes = safestriproots(ui, repo, nodelist)
+        return strip(ui, repo, nodes, True, topic)
+    # transaction postclose callbacks are called in alphabet order.
+    # use '\xff' as prefix so we are likely to be called last.
+    callback = tr.getpostclose('\xffstrip')
+    if callback is None:
+        callback = stripcallback(ui, repo, True, topic)
+        tr.addpostclose('\xffstrip', callback)
+    if topic:
+        callback.topic = topic
+    callback.addnodes(nodelist)
+
 def striptrees(repo, tr, striprev, files):
     if 'treemanifest' in repo.requirements: # safe but unnecessary
diff --git a/mercurial/transaction.py b/mercurial/transaction.py
--- a/mercurial/transaction.py
+++ b/mercurial/transaction.py
@@ -407,5 +407,5 @@  class transaction(object):
     @active
     def addpostclose(self, category, callback):
-        """add a callback to be called after the transaction is closed
+        """add or replace a callback to be called after the transaction closed
 
         The transaction will be given as callback's first argument.
@@ -417,4 +417,9 @@  class transaction(object):
 
     @active
+    def getpostclose(self, category):
+        """return a postclose callback added before, or None"""
+        return self._postclosecallback.get(category, None)
+
+    @active
     def addabort(self, category, callback):
         """add a callback to be called when the transaction is aborted.
diff --git a/tests/test-strip.t b/tests/test-strip.t
--- a/tests/test-strip.t
+++ b/tests/test-strip.t
@@ -3,4 +3,5 @@ 
   $ echo "[extensions]" >> $HGRCPATH
   $ echo "strip=" >> $HGRCPATH
+  $ echo "drawdag=$TESTDIR/drawdag.py" >> $HGRCPATH
 
   $ restore() {
@@ -941,3 +942,51 @@  Error during post-close callback of the 
   [255]
 
+Use delayedstrip to strip inside a transaction
 
+  $ cd $TESTTMP
+  $ hg init delayedstrip
+  $ cd delayedstrip
+  $ hg debugdrawdag <<'EOS'
+  >   D
+  >   |
+  >   C F H    # Commit on top of "I",
+  >   | |/|    # Strip B+D+I+E+G+H+Z
+  > I B E G
+  >  \|/
+  >   A   Z
+  > EOS
+
+  $ hg up -C I
+  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ echo 3 >> I
+  $ cat > $TESTTMP/delayedstrip.py <<EOF
+  > from mercurial import repair, commands
+  > def reposetup(ui, repo):
+  >     def getnodes(expr):
+  >         return [repo.changelog.node(r) for r in repo.revs(expr)]
+  >     with repo.wlock():
+  >         with repo.lock():
+  >             with repo.transaction('delayedstrip'):
+  >                 repair.delayedstrip(ui, repo, getnodes('B+I+Z+D+E'), 'J')
+  >                 repair.delayedstrip(ui, repo, getnodes('G+H+Z'), 'I')
+  >                 commands.commit(ui, repo, message='J', date='0 0')
+  > EOF
+  $ hg log -r . -T '\n' --config extensions.t=$TESTTMP/delayedstrip.py
+  warning: orphaned descendants detected, not stripping 08ebfeb61bac, 112478962961, 7fb047a69f22
+  saved backup bundle to $TESTTMP/delayedstrip/.hg/strip-backup/f585351a92f8-81fa23b0-I.hg (glob)
+  
+  $ hg log -G -T '{rev}:{node|short} {desc}' -r 'sort(all(), topo)'
+  @  6:2f2d51af6205 J
+  |
+  o  3:08ebfeb61bac I
+  |
+  | o  5:64a8289d2492 F
+  | |
+  | o  2:7fb047a69f22 E
+  |/
+  | o  4:26805aba1e60 C
+  | |
+  | o  1:112478962961 B
+  |/
+  o  0:426bada5c675 A
+