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

[Mads Kiilerich]

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? You don't have a test case to show it 
so you really need a good explanation. But there should be a test case 
anyway.

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

"and" = two patches.

> 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?

/Mads