[PATCH] Added ability to clone from a local repository to a (new) remote one

Sean Meiners sean.meiners at linspire.com
Tue Jun 20 13:13:35 CDT 2006


Comments in-line, new patch on its way (via the ever-useful patchbomb).

On Monday 19 June 2006 19:37, Bryan O'Sullivan wrote:
> On Mon, 2006-06-19 at 17:08 -0700, Sean Meiners wrote:
> > Added ability to clone from a local repository to a (new) remote one.
>
> Thanks!  This will be a welcome feature to a lot of people, myself
> included.  However, I do have some comments - see below.  None of them
> is major, so they shouldn't take long to fix up.
>
> > Updating the <repo>/.hg/hgrc file is now optional and will
> > automatically set the default pull location when cloning to a remote
> > repository.
>
> So if I clone from "a" to "b", hg updates "a"'s "default" to point to
> "b"?  This properly belongs in a separate patch so we can haggle over
> its merits independent of the rest of the work.  If you could split it
> out, that would be good, because I expect that the behaviour will be a
> sticking point, and I'd like to see the rest of the functionality in.

Maybe what we really need is a separate command, but for our use case this is 
necessary. We have a large tool chain built around centralized revision 
control (we currently use TLA). A normal workflow for a developer is:
1) create new source tree (tla init-tree / hg init)
2) add files (tla add-id / hg add)
3) check it into the source control file server (tla import / hg ?)
4) tell one of our build machines to pull the source from the central server 
and build it.

While simply supporting cloning to a remote location meets 90% of the 
requirements for step 3, it's missing one thing. The ability for the 
developer to easily commit future changes to the central server (tla commit / 
hg push). Having the clone operation automatically update .hg/hgrc means that 
when the developer goes to push their next set of changes it 'just works' and 
they don't have to remember to edit the hgrc file themselves.

However, in the interest of getting this patch in, I've reversed the option. 
The user must now explicitly ask clone to update the hgrc file when cloning 
to a remote location, all other cases will behave as before.

>
> > I also changed the names of the 'repo' and 'other' variables to
> > 'destRepo' and 'srcRepo' to maintain my own sanity.
>
> That's fine in principle, but please pay attention to the surrounding
> coding style when you make changes like this.  There are no camelCaps
> anywhere else in the code.

Fair enough, changed to src_repo and dest_repo instead.

>
> Also, your patch adds lots of trailing white space and empty lines full
> of white space.

I couldn't find more than 3 or 4 lines when extraneous whitespace, those have 
been cleaned up.

>
> > +    destRepo = None
> > +    try:
> > +        destRepo = hg.repository(ui, dest)
> > +    except hg.RepoError:
> > +        pass
> > +
> > +    if destRepo:
> > +        error = _("destination '%s' already exists." % dest)
> > +        raise util.Abort(error)
> > +    destRepo = hg.repository(ui, dest, create=1)
>
> This is quite messy and unclear.  What's wrong with this instead?
>
> try:
>     dest_repo = hg.repository(ui, dest)
>     raise util.Abort(_("dest exists"))
> except hg.RepoError:
>     dest_repo = hg.repository(ui, dest, create=True)
>
> It's much more compact and clear in its intention.

Yes it is, changed.

>
> > +    elif srcRepo.dev() != -1:
> > +        if not opts['nohgrc']:
> > +            inPaths = False
> > +            groupRE = re.compile(r'^\s*\[([^\]]+)\].*')
> > +            defaultRE = re.compile(r'^\s*default\s*=.*')
> > +            lines = []
> > +            f = srcRepo.opener("hgrc", "r", text=True)
> > +            for line in f:
> > +                match = groupRE.match(line)
> > +                if match:
> > +                    if match.group(1) == 'paths':
> > +                        inPaths = True
> > +                    else:
> > +                        inPaths = False
> > +                elif inPaths:
> > +                    if defaultRE.match(line):
> > +                        lines.append( 'default = %s\n' % dest )
> > +                        continue
> > +                lines.append(line)
> > +            f.close()
> > +
> > +            f = srcRepo.opener("hgrc", "w", text=True)
> > +            for line in lines:
> > +                f.write(line)
> > +            f.close()
> > +
> > +    if d:
> > +        d.close()
>
> This is the stuff that needs splitting out.  I think the likelihood of
> it being accepted is very low, because it will (a) update the local hgrc
> at all and (b) modify the local hgrc every time you clone from local to
> remote.
>
> > +        if create:
> > +            try:
> > +                self.validate_repo(ui, sshcmd, args, remotecmd)
> > +                return # the repo is good, nothing more to do
> > +            except hg.RepoError:
> > +                pass
>
> This doesn't look right.  If validate_repo passes, and we've been passed
> create=True, we should bomb out because the destination repo exists;
> this is what "hg clone" and "hg init" do now.

I'm just following the behavior of localrepo.py's __init__. If create=0 then 
it fails is the repo doesn't exist. If create=1 then it only fails if it's 
unable to create it, note that it does not fail if the repo already exists.

>
> > +                raise hg.RepoError(_("Could not create remote repo"))
>
> Minor nit: error messages tend to be lowercase.

Good point, fixed.

>
> 	<b

-- 
Sean Meiners
sean.meiners at linspireinc.com


Perl - $Just @when->$you ${thought} s/yn/tax/ &couldn\'t %get $worse;


More information about the Mercurial mailing list