[PATCH 2 of 2] subrepo: check that subrepo paths are valid (issue4363)

Jordi Gutiérrez Hermoso jordigh at octave.org
Wed Sep 10 09:57:31 CDT 2014


On Wed, 2014-09-10 at 01:31 +0200, Mads Kiilerich wrote:
> On 09/09/2014 07:56 PM, Jordi Gutiérrez Hermoso wrote:
> > # HG changeset patch
> > # User Jordi Gutiérrez Hermoso <jordigh at octave.org>
> > # Date 1410279641 14400
> > #      Tue Sep 09 12:20:41 2014 -0400
> > # Node ID 3db2b7dd861d7134412678f96df7b7bb1cfe9c82
> > # Parent  7611a7833657301d9a881095828d5e75558d82f9
> > subrepo: check that subrepo paths are valid (issue4363)
> >
> > This checks that proposed subrepo paths are valid using
> > pathutil.pathauditor. This check is already done when creating a
> > subrepo object in the subrepo.subrepo function, but I think this is
> > too late and doesn't catch the case where a modified .hgsub has had
> > new paths added but hasn't been committed yet.
> 
> Why do you think that is too late?

I say right here, "doesn't catch the case where a modified..." Testing
for bad paths later doesn't catch this case.

> > Moving this check earlier and catching the exception to provide a
> > better error message both seem like improvements.
> 
> "and" = two patches.

Okay.

> > diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
> > --- a/mercurial/subrepo.py
> > +++ b/mercurial/subrepo.py
> > @@ -130,6 +130,12 @@ def state(ctx, ui):
> >           if os.path.isfile(path):
> >               raise util.Abort(_('subrepository path is an existing file: %s')
> >                                % path)
> > +        try:
> > +            pathutil.pathauditor(ctx._repo.root)(path)
> > +        except util.Abort, e:
> > +            raise util.Abort(_('bad subrepository path; %s') % e.message,
> > +                             hint=e.hint)
> > +
> >           kind = 'hg'
> >           if src.startswith('['):
> >               if ']' not in src:
> > @@ -344,7 +350,6 @@ def subrepo(ctx, path):
> >       import hg as h
> >       hg = h
> >   
> > -    pathutil.pathauditor(ctx._repo.root)(path)
> 
> Are you 200% confident that removing this pathauditor in another method 
> doesn't open up for any un-audited code paths?

It passes the test suite. And I know that before creating a subrepo
object, .hgsub must be parsed, so erroring out when parsing .hgsub
seems to be the best thing to do.

There's a bigger problem here, really, that I changed the error
message in order to actually make it clear what the bad path was
about. The error message before was really cryptic and made it
impossible to know that it was about subrepos. So, that really is a
BC, which I guess passed the test suite because we don't have a test
for it.

Assuming this BC can happen (and my guess is it will be shot down),
Augie raised the point that it means I should change how
pathutil.pathauditor works, since apparently catching util.Abort is
undesirable, although I'm not 100% clear on why. I suppose it's too
generic of a catch. If that is the problem, then I would prefer to
modify pathauditor to raise a subclass of util.Abort, but Augie
indicated this was bad technical debt too. This explanation is still
obscure to me.

So what's the problem with catching util.Abort? I'd rather not make
the more annoying change of modifying pathauditor and all of its
callers to return status and error messages instead of raising
exceptions.

- Jordi G. H.




More information about the Mercurial-devel mailing list