Re: PATCHES: start adding Mercurial backend (HgOutputOption)

On Sat, Sep 26, 2009 at 9:56 PM, Michael Haggerty
> I noticed comments in a couple of places that question what "post
> commits" are and why you should care about them.  Here is the
> explanation:

Thanks for that!  I did end up implementing support for "post" commits
by slavishly imitating the git and svn backends, and it seems to
mostly work.  I think that's coming in future patches, which I haven't
prepared yet.  (I was waiting for feedback on the first set and have
since got busy with other stuff.)

> I did skim through your patches and have a
> few basic comments:
>
> >   def _commit_memctx(self, mctx):
> >     # XXX should I be wrapping my txn in weakref.proxy()?
> >     txn = self.repo.transaction()
> >     try:
> >       node = self.repo.commitctx(mctx)
> >       txn.close()
> >     finally:
> >       del txn
> >     return node
>
> I don't understand the point of the try...finally here.  "del txn" does
> not force txn.__del__() to be run; it only unbinds the local "txn"
> variable (possibly allowing __del__() to run if the reference count goes
> to zero).  But txn will be unbound anyway when the function returns on
> the next statement, so "del txn" doesn't accomplish anything.

Quite right!  I think I was just following what appears to be
established practice in the Mercurial code.  The canonical example is
probably localrepository.commitctx(): see
http://hg.intevation.org/mercurial/stable/file/tip/mercurial/localrepo.py
around line 900.  Specifically, see the "finally" block around like
965:

  finally:
      del tr
      lock.release()

(Mercurial uses 'tr' where I used 'txn'.  I like short snappy variable
names, but not *that* short and snappy.  ;-)  That is right at the end
of the method, so the "del tr" sure looks pointless to me.

I'll ask on mercurial-devel about this idiom.

[from one of the patches]
> +    # XXX it ought to be possible for a CVS tag to have a split
beginning, just
> +    # like a CVS branch can.  Should handle it the same we way handle split
> +    # branches.

My reasoning was faulty when I wrote that comment.  I'm pretty sure I
have since deleted it, but I forgot to scrub it from history when I
was preparing the patch series.  (Or scrubbing it caused painful
conflicts: can't remember.)

Greg

------------------------------------------------------
http://cvs2svn.tigris.org/ds/viewMessage.do?dsForumId=1667&dsMessageId=2403263

To unsubscribe from this discussion, e-mail: [[email protected]].