Mozilla: hg as a collaboration tool

[Alexis S. L. Carvalho]

Thus spake Andrei Vermel:
> Here's a patch against crew with a test. 
> 
> Andrei
> 
> mq: add qtimes command to save and restore modification times

I don't have any opinions on this feature, but this patch needs some
more work.  In particular:

- what should happen if I run this:

  hg qpush -a
  hg up -C some-other-revision
  hg qtimes -s
  
  Does it make a difference if some-other-revision is managed by mq?
  What if I make some changes after the update?
  
  The easy answer is to require the user to be at the top of the queue,
  with a clean working dir, but that may be too restrictive.

  OTOH, qtimes --restore can optimistically try to restore the mtimes,
  no matter what revision is the parent of the working dir.

- files should be opened in binary mode

- patched_mtimes should be in .hg/patches, probably renamed to something
  like .mq-patched-mtimes

- the format "<filename>,<checksum>,<mtime>" won't work with files that
  have a ',' in them.  Something like "<checksum> <mtime> <filename>" is
  probably better

In details:

> diff --git a/hgext/mq.py b/hgext/mq.py
> --- a/hgext/mq.py
> +++ b/hgext/mq.py
> @@ -2132,6 +2132,46 @@
>          repo.__class__ = mqrepo
>          repo.mq = queue(ui, repo.join(""))
>  
> +import md5

Move this to the start of the file.

> +def times(ui, repo, *pats, **opts):
> +    """save or restore modification times of patched files"""
> +    q = repo.mq
> +    node1, node2 = cmdutil.revpair(repo, ['qparent', 'tip'])

You want either '.' (parent of the working dir) or 'qtip', but not
'tip'.

It's better to use repo.lookup than cmdutil.revpair.  It's even better
to just get the nodes directly:

qparent:
    base = revlog.bin(q.applied[0].rev)
    node1 = repo.changelog.parents(base)[0]

'.':
   node2 = q.qparents(repo)

qtip:
   node2 = revlog.bin(q.applied[-1].rev)

> +    if node1 == node2:
> +        ui.warn(_('no patches applied - nothing to do'))
> +        return True

Instead of testing "node1 == node2", check for "not q.applied".  Do it
before looking up the revisions.

You may want to add other checks after this one, but still before
looking up the revisions.  E.g. to check that we're at the top patch,
call q.check_toppatch(repo) (this already handily returns qtip).

> +    files, matchfn, anypats = cmdutil.matchpats(repo, pats, opts)
> +    cwd = (pats and repo.getcwd()) or ''
> +    modified, added, removed, deleted, unknown, ignored, clean = [
> +        n for n in repo.status(node1=node1, node2=node2, files=files,
> +                             match=matchfn)]

You only use these guys with --save, so move these lines into the
"if opts['save']".

You don't use the cwd anywhere, so you can just remove the second line.

The call to repo.status can be simplified to

modified, added = repo.status(node1=node1, node2=node2, files=files,
                              match=matchfn)[:2]

> +    if opts['save']:
> +        pm = file(repo.path+'/patched_mtimes', 'w')

pm = q.opener('.mq-patched-mtimes', 'w')

> +        patched = modified + added
> +        for f in patched:        
> +            fname = repo.wjoin(f)
> +            mtime = os.stat(fname).st_mtime

How should symlinks be dealt with?  AFAICS, os.utime doesn't work on the
symlink itself, so this should probably use os.lstat and skip the file
if it's a symlink.  Using os.stat (but catching OSError in case of
broken symlinks) is another option, but I'm a bit wary of calling
os.utime without knowing where the symlink points to.

> +            checksum = md5.new()
> +            checksum.update(file(fname, 'r').read())

checksum.update(repo.wfile(f).read())

> +            pm.write("%s,%s,%s\n" % (f, checksum.hexdigest(), mtime))

As I said, change this to "<checksum> <mtime> <filename>\n"

> +    elif opts['restore']:
> +        pm = file(repo.path+'/patched_mtimes', 'r')

pm = q.opener('.mq-patched-mtimes')

> +        for line in pm.readlines():
> +            f, checksum, mtime = line.strip().split(',')

checksum, mtime, f = line.strip('\n').split(None, 2)

> +            fname = repo.wjoin(f)
> +            cs = md5.new()
> +            cs.update(file(fname, 'r').read())

cs.update(repo.wfile(f).read())

> +            if cs.hexdigest() != checksum:
> +                ui.warn(_('Not restoring timestamp of file %s\n') % f)
> +            else:
> +                st = os.stat(fname)
> +                os.utime(fname, (st.st_atime, type(st.st_mtime)(mtime)))

can't you just use float(mtime)?  AFAICS os.utime doesn't complain even
if stat returns ints.

> +    else:
> +        ui.warn(_("hg qtimes: either -r or -s option is needed\n"))
> +        commands.help_(ui, 'qtimes')
> +
> +    return True
> +
>  seriesopts = [('s', 'summary', None, _('print first line of patch
> header'))]
>  
>  cmdtable = {
> @@ -2249,6 +2289,11 @@
>            ('b', 'backup', None, _('bundle unrelated changesets')),
>            ('n', 'nobackup', None, _('no backups'))],
>           _('hg strip [-f] [-b] [-n] REV')),
> +    "qtimes": 
> +        (times, 
> +         [('s', 'save', None, _('save modification times')),
> +          ('r', 'restore', None, _('restore modification times'))], 

Also include standard walk options - i.e. "+ commands.walkopts" after
the "]".

I think the only place where we've used '-r' for something different
from '--rev' is hg status, and that is still haunting us.  So it might
be better to use something else here.  Maybe -l/--reload or -l/--load?

Alexis