[PATCH 1 of 2] largefiles: fix rename (Issue 3093)

[Matt Mackall]

On Tue, 2011-11-29 at 22:10 +0100, Na'Tosha Bard wrote:
> On Tue, Nov 29, 2011 at 9:49 PM, Matt Mackall <mpm at selenic.com> wrote:
> 
> > On Tue, 2011-11-29 at 16:15 +0100, Na'Tosha Bard wrote:
> > > # HG changeset patch
> > > # User Na'Tosha Bard <natosha at unity3d.com>
> > > # Date 1322579324 -3600
> > > # Node ID dfe717266e728cb2f780899ad218f3a4fa2a5ee9
> > > # Parent  ad686c818e1c7d5ed0335327321003ba8ea04664
> > > largefiles: fix rename (Issue 3093)
> >
> > Note that hgbot is picky as computers are often wont to be and thus so
> > am I by proxy: that must be '(issue3093)'. Lowercase, no space.
> >
> 
> Sorry, didn't know that.
> 
> 
> > > diff -r ad686c818e1c -r dfe717266e72 hgext/largefiles/overrides.py
> > > --- a/hgext/largefiles/overrides.py   Fri Nov 25 02:11:12 2011 +0100
> > > +++ b/hgext/largefiles/overrides.py   Tue Nov 29 16:08:44 2011 +0100
> > > @@ -389,17 +389,18 @@
> > >              lfdirstate = lfutil.openlfdirstate(ui, repo)
> > >              for (src, dest) in copiedfiles:
> > >                  if lfutil.shortname in src and lfutil.shortname in dest:
> > > -                    srclfile = src.replace(lfutil.shortname, '')
> > > -                    destlfile = dest.replace(lfutil.shortname, '')
> >
> > This is not robust enough. It'll mangle anything that happens to have
> > ".hglf" in it, which means we'll eventually get a weird bug report where
> > we go back and forth five times until the user finally reveals the
> > secret filename they've been hiding from us and we go "aha, perhaps it's
> > a largefiles bug, why would you name a file that, what is wrong with
> > you?"
> >
> > ..which means breaking down the path components with os.path.dirname and
> > friends.
> >
> 
> OK, I think checking for the entire repo.root + lfutil.shortname substring
> at the start of the string should do the trick here, since we know the
> strings we are interested in contain (and starts with) the repo root.
> 
> I didn't actually change this in the patch, but I'll fix it anyway.
> 
> 
> >
> > > +                    substr = repo.root + "/" + lfutil.shortname + "/"
> >
> > We should probably use os.path.join or repo.wjoin. A helper function
> > like lfjoin is probably warranted. Did you test this on a system where
> > the separator is "\", by any chance?
> >
> 
> Does Mercurial not use "/" as the internal separator, even on Windows?  It
> certainly does for its output to the user.

Depends what you mean by internal. In general, we should be prepared for
'\' or '/' to show up in filenames from the command line or otherwise
connected with the working directory on Windows.

> The real problem is that we have to make sure we remove that trailing "/";
> I'll see if I can find something that looks nicer, and will also test it on
> Windows, and send another patch.
> 
> 
> > > +                    srclfile = src.replace(substr, '')
> > > +                    destlfile = dest.replace(substr, '')
> > >                      destlfiledir = os.path.dirname(destlfile) or '.'
> > >                      if not os.path.isdir(destlfiledir):
> > >                          os.makedirs(destlfiledir)
> > >                      if rename:
> > > -                        os.rename(srclfile, destlfile)
> > > -                        lfdirstate.remove(repo.wjoin(srclfile))
> > > +                        os.rename(repo.wjoin(srclfile),
> > repo.wjoin(destlfile))
> > > +                        lfdirstate.remove(srclfile)
> > >                      else:
> > >                          util.copyfile(srclfile, destlfile)
> > > -                    lfdirstate.add(repo.wjoin(destlfile))
> > > +                    lfdirstate.add(destlfile)
> > >              lfdirstate.write()
> > >          except util.Abort, e:
> > >              if str(e) != 'no files to copy':
> > > _______________________________________________
> > > Mercurial-devel mailing list
> > > Mercurial-devel at selenic.com
> > > http://selenic.com/mailman/listinfo/mercurial-devel
> >
> >
> > --
> > Mathematics is the supreme nostalgia of our time.
> >
> >
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel at selenic.com
> > http://selenic.com/mailman/listinfo/mercurial-devel
> >


-- 
Mathematics is the supreme nostalgia of our time.