[PATCH 1 of 1] mq/qqueue: enable renaming of active queue

Yann E. MORIN yann.morin.1998 at anciens.enib.fr
Sun Aug 15 09:21:57 CDT 2010


Benoit, All,

On Sunday 15 August 2010 15:54:07 Benoit Boissinot wrote:
> On Sun, Aug 15, 2010 at 01:35:27PM +0200, Yann E. MORIN wrote:
> > mq/qqueue: enable renaming of active queue
> Smallish comments (not a proper review about the proposed feature)
> > +        if name == 'patches':
> > +            newdir = repo.join('patches')
> > +        else:
> > +            newdir = repo.join('patches-' + name)
> worth a refactor? (does it happen elsewhere?)

Yes, most probably. Will do the refactor, and will try to find places that
can use it.

> > +        try:
> > +            stats = os.stat(newdir)
> > +            raise util.Abort(_('existing patch directory for queue "%s"') % name)
> > +        except OSError:
> > +            pass
> 
> if os.path.exists(newdir):
>     raise ...

Ah! Good! Python-aware power! :-)

> > +        fh = repo.opener('patches.queues.new', 'w')
> > +        for queue in existing:
> > +            if queue == current:
> > +                fh.write('%s\n' % (name,))
> > +                try:
> > +                    os.rename(olddir, newdir)
> > +                except OSError:
> 
> really? pass?

Yes, because the patch directory may not exist yet, so we do not want
to fail. Consider the following:
  hg qqueue --create me-fature   # Obvious typos here...
  hg qqueue --rename my-feature  # ... which we fix here.

And there is not yet a patch directory to hold me-fature's patches, as
the patch dir is created with the first qnew.

But I will use os.path.exists() in next iteration.

> For clarity the os.rename should probably be moved outside of the loop.

Well, I wanted the rename in the queue and on disk to appear correlated
as much as possible. There's a slight race condition here, in case another
hg mq command is run at the same time, and I do not know if the repo is
locked (is it even lockable?) during mq actions. So, the shorter the race
window, the better. Or so I think.

> (by the way, why use os.rename here, and util.rename below?)

Oh, that's because I'm no Python expert. I needed a way to rename a directory
so I looked at python's doc, and came up with os.rename.

Then I needed to set the active queue, so I copied the chunk from the create
case, just above. And I did not notice the discrepancy. Will use util.rename
in next iteration.

> cheers,

Thanks for the review!

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'


More information about the Mercurial-devel mailing list