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

[Na'Tosha Bard]

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.

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20111129/a0c17a04/attachment.html>