[PATCH 1 of 9 V3] cmdutil: add the class to restore dirstate at unexpected failure easily

Tue May 12 07:54:23 CDT 2015

At Mon, 11 May 2015 17:49:43 -0700,
Pierre-Yves David wrote:
> 
> On 05/11/2015 08:49 AM, FUJIWARA Katsunori wrote:
> >
> > At Fri, 08 May 2015 19:00:02 -0700,
> > Pierre-Yves David wrote:
> >>
> >> On 05/06/2015 08:15 PM, FUJIWARA Katsunori wrote:
> >>> # HG changeset patch
> >>> # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
> >>> # Date 1430968030 -32400
> >>> #      Thu May 07 12:07:10 2015 +0900
> >>> # Node ID 449a46109f0c96bb8d82d4edf2c8855341558c2f
> >>> # Parent  8174d27576a3fff28fb6f952a4c89d5c0b900214
> >>> cmdutil: add the class to restore dirstate at unexpected failure easily

[snip]

> >>> For this kind of issue, "extending transaction" approach (in
> >>> https://titanpad.com/mercurial32-sprint) seems not to be suitable,
> >>> because:
> >>>
> >>>     - transaction nesting occurs in some cases (e.g. "shelve => rebase"), and
> >>>
> >>>     - "dirstate" may be already modified since the beginning of OUTER
> >>>       transaction scope, then
> >>>
> >>>     - dirstate should be backed up into the file other than
> >>>       "dirstate.journal" at the beginning of INNER transaction scope, but
> >>>
> >>>     - such alternative backup files are useless for transaction itself,
> >>>       and increases complication of its implementation
> >>
> >> Some part about the transaction confuses me.
> >>
> >>    - Yes, you are right, the dirstate changes may have a wider life spawn
> >> than the transaction.
> >>
> >>    - Therefor doing backup there is not suitable (and is maybe never
> >> useful) (your are still right there)
> >>
> >>    - However, for any changes started inside a transaction, we should
> >> never write data in .hg/dirstate until the transaction is actually
> >> committed (because it likely refers to unwritten data in the
> >> transaction). This means that hooks/editors/etc would need to access a
> >> .hg/distate.pending in that case.
> >>
> >>    - The transaction API allow for both "wait until transaction is closed
> >> to flush that file" logic and "please write pending data for hooks"
> >> logic. It looks like the dirstate handling should collaborate with the
> >> transaction if one exist when dirstate manipulation are done. I'm not
> >> sure how much it happen.
> >
> > Committing via "repo.commit()" may cause invoking editor process out
> > of transaction scope, but flushing dirstate is still needed: for
> > example, "hg commit -A" changes status of each unknown files before
> > "repo.commit()".
> >
> > `.pending` mechanism of transaction itself can't be used directly in
> > such cases.
> >
> > But on the other hand, editor process may be invoked in transaction
> > scope, and (maybe) pending changelog and so on should become visible
> > for external process: for example, "hg import -e" with multiple
> > patches should cause such situation (BTW, rebasing multiple revisions
> > creates a transaction for each revisions to be rebased).
> >
> > So, I'll try to follow the style of `.pending` mechanism of
> > transaction for dirstate.
> 
> The way transaction works for that (roughly) is:
> 
> - write all files with a '.pending' suffix,
> - set an environment variable HG_PENDING=<repo-root>
> - Mercurial process during hooks detect the HG_PENDING vars and read the 
> .pending file if they exists.
> 
> As you point out, there is case where the dirstate writing is not done 
> during a transaction (eg: update). And in this case, we should -not- 
> collaborate with the transaction and we should -not- use 
> dirstate.pending file.
> 
> However, if we are in the middle of a transaction, the new dirstate will 
> likely point to content of the transaction and we should rely on the 
> transaction to generate a '.pending' file when needed (also meaning, we 
> should teach dirstate to read them) and to flush data to disk when the 
> the transaction is committed (see the file generator API, taking care of 
> both).
> 
> (you can use `localrepo.currenttransaction()` to check if a transaction 
> is open and if we jump onboard).
> 
> All that said, current dirstate implementation is not collaborating with 
> transaction either. So your changes are an improvement. I'll take your 
> series as is (with a couple of typo, and name fixed) and will hope for 
> more follow up on this topic.

Thank you for pushing to clowncopter with in-flight fixing.

After working with '.pending' mechanism, I realize that writing
dirstate out should be done in "cmdutil.commitforceeditor()" instead
of in "repo.commit()" for '.pending' awareness.

Could you drop only #9 of this series from clowncopter ?

I'll soon post follow up series, which also takes care of ".pending"
mechanism (regardless of dropping #9).

> >> This re-entrance business transition perfectly to my last question: It
> >> seems like this superseed the beginparentchange/endparentchange system
> >> previously in place. However, there is still multiple call to this
> >> system. Could we, in theory replace them will with the new system? If
> >> not, why?
> >
> > begin/end-parentchange system seems handy for small (a few lines)
> > scope changes, because it doesn't need writing current dirstate out
> > and backing it up.
> >
> > For example, current "heavy" dirstateguard doesn't seem suitable for
> > "committablectx.markcommitted()".
> >
> >      def markcommitted(self, node):
> >          self._repo.dirstate.beginparentchange()
> >          for f in self.modified() + self.added():
> >              self._repo.dirstate.normal(f)
> >          for f in self.removed():
> >              self._repo.dirstate.drop(f)
> >          self._repo.dirstate.setparents(node)
> >          self._repo.dirstate.endparentchange()
> >
> > For such lightweight use, we may have to add the flag to
> > enable/disable "writing current dirstate out and backing it up" to
> > dirstateguard: when backup is disabled, "dirstate.write()" may have to
> > be blocked for safety.
> >
> > On the other hand, begin/end-parentchange system doesn't seem suitable
> > for non-small scope or complicated changes, because it is difficult to
> > ensure that "dirstate.write()" isn't implied in such case (like ones
> > in mq, commands.import and cmdutil.tryimportone replaced by this
> > series).
> >
> > begin/end-parentchange in this condition should be replaced by
> > dirstateguard or so, as soon as possible.
> 
> I think we should have a single mechanism of people will use it wrong. 
> We should also makes it mandatory (or at least issue devel-warning) to 
> get people to use it.
> 
> Can we flag the old system for deletion and hunt down all it's current user?

I'll try.

> >> Also, this old system was living on the dirstate object directly. would
> >> it make sense to move the guardian business there?
> >
> > IMHO, yes, even though some more preparations may be needed to
> > complete moving.
> 
> Could be done in a follow up.

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy at lares.dti.ne.jp