Patchwork [1,of,2] subrepo: check that subrepo paths are not existing files

login
register
mail settings
Submitter Jordi Gutiérrez Hermoso
Date Sept. 9, 2014, 5:56 p.m.
Message ID <7611a7833657301d9a88.1410285401@Iris>
Download mbox | patch
Permalink /patch/5745/
State Changes Requested
Headers show

Comments

Jordi Gutiérrez Hermoso - Sept. 9, 2014, 5:56 p.m.
# HG changeset patch
# User Jordi Gutiérrez Hermoso <jordigh@octave.org>
# Date 1410279390 14400
#      Tue Sep 09 12:16:30 2014 -0400
# Node ID 7611a7833657301d9a881095828d5e75558d82f9
# Parent  897041f6b025778193c6da5b9795da09a91c866e
subrepo: check that subrepo paths are not existing files

This check has to happen early one, as the .hgsub file is being
parsed. This should provide an early error message.
Mads Kiilerich - Sept. 9, 2014, 11:31 p.m.
On 09/09/2014 07:56 PM, Jordi Gutiérrez Hermoso wrote:
> # HG changeset patch
> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> # Date 1410279390 14400
> #      Tue Sep 09 12:16:30 2014 -0400
> # Node ID 7611a7833657301d9a881095828d5e75558d82f9
> # Parent  897041f6b025778193c6da5b9795da09a91c866e
> subrepo: check that subrepo paths are not existing files
>
> This check has to happen early one, as the .hgsub file is being
> parsed.

Why?

> This should provide an early error message.

Could we please have a test to show it?

> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
> --- a/mercurial/subrepo.py
> +++ b/mercurial/subrepo.py
> @@ -126,6 +126,10 @@ def state(ctx, ui):
>   
>       state = {}
>       for path, src in p[''].items():
> +

Why add an extra empty line here? It is stupid to spend time complaining 
about it but it also doesn't work to add an empty after every for loop.

> +        if os.path.isfile(path):

Depending on what problem this patch is solving, shouldn't it also 
consider directories and symlinks and other file types?

> +            raise util.Abort(_('subrepository path is an existing file: %s')
> +                             % path)
>           kind = 'hg'
>           if src.startswith('['):
>               if ']' not in src:

/Mads
Jordi Gutiérrez Hermoso - Sept. 10, 2014, 2:48 p.m.
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@octave.org>
> > # Date 1410279390 14400
> > #      Tue Sep 09 12:16:30 2014 -0400
> > # Node ID 7611a7833657301d9a881095828d5e75558d82f9
> > # Parent  897041f6b025778193c6da5b9795da09a91c866e
> > subrepo: check that subrepo paths are not existing files
> >
> > This check has to happen early one, as the .hgsub file is being
> > parsed.
> 
> Why?

Because why not catch an error as early as possible?

Currently these problems are caught later after hgsub has been parsed
and when attempting to create a subrepo Python object. This is too
late, and is the core problem in bug #4363.

> > This should provide an early error message.
> 
> Could we please have a test to show it?

Sure, I'll write one.

> > diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
> > --- a/mercurial/subrepo.py
> > +++ b/mercurial/subrepo.py
> > @@ -126,6 +126,10 @@ def state(ctx, ui):
> >   
> >       state = {}
> >       for path, src in p[''].items():
> > +
> 
> Why add an extra empty line here? It is stupid to spend time
> complaining about it but it also doesn't work to add an empty after
> every for loop.

It made sense to me, but I don't really care, I'll get rid of it.

> > +        if os.path.isfile(path):
> 
> Depending on what problem this patch is solving, shouldn't it also 
> consider directories and symlinks and other file types?

Those other problems are already caught by pathauditor in the other
patch.

I'll explain that in the commit message.

- Jordi G. H.
Mads Kiilerich - Sept. 10, 2014, 5:31 p.m.
On 09/10/2014 04:48 PM, Jordi Gutiérrez Hermoso wrote:
> 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@octave.org>
>>> # Date 1410279390 14400
>>> #      Tue Sep 09 12:16:30 2014 -0400
>>> # Node ID 7611a7833657301d9a881095828d5e75558d82f9
>>> # Parent  897041f6b025778193c6da5b9795da09a91c866e
>>> subrepo: check that subrepo paths are not existing files
>>>
>>> This check has to happen early one, as the .hgsub file is being
>>> parsed.
>> Why?
> Because why not catch an error as early as possible?

What would happen if we don't catch it? I would expect that an existing 
file to be an error that automatically is handled just fine. The updated 
patch description might give the answer.

> Currently these problems are caught later after hgsub has been parsed
> and when attempting to create a subrepo Python object. This is too
> late, and is the core problem in bug #4363.

It is not obvious how you get from that bug report to that conclusion. 
You might be right, but my gut feeling is that the core problem is 
something else.

/Mads
Jordi Gutiérrez Hermoso - Sept. 10, 2014, 6:55 p.m.
On Wed, 2014-09-10 at 19:31 +0200, Mads Kiilerich wrote:
> On 09/10/2014 04:48 PM, Jordi Gutiérrez Hermoso wrote:
> > 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@octave.org>
> >>> # Date 1410279390 14400
> >>> #      Tue Sep 09 12:16:30 2014 -0400
> >>> # Node ID 7611a7833657301d9a881095828d5e75558d82f9
> >>> # Parent  897041f6b025778193c6da5b9795da09a91c866e
> >>> subrepo: check that subrepo paths are not existing files
> >>>
> >>> This check has to happen early one, as the .hgsub file is being
> >>> parsed.
> >> Why?
> > Because why not catch an error as early as possible?
> 
> What would happen if we don't catch it?

What happens right now, bug #4363.

> > Currently these problems are caught later after hgsub has been parsed
> > and when attempting to create a subrepo Python object. This is too
> > late, and is the core problem in bug #4363.
> 
> It is not obvious how you get from that bug report to that
> conclusion. 

Running hg in ipdb, basically. It creates a subrepos dict that
includes .hg, and then at the point in which the exception happens
it's iterating through this list before any subrepo object is created.
This list then deletes the ".hg" key at some point, and has extra code
to delete this key again, causing a stack trace. The problem is
obviously (well, obviously to me) that this subrepos list shouldn't be
allowed to contain this list in the first place.

I could add this explanation to the commit message, if you think it's
appropriate.

- Jordi G. H.

Patch

diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -126,6 +126,10 @@  def state(ctx, ui):
 
     state = {}
     for path, src in p[''].items():
+
+        if os.path.isfile(path):
+            raise util.Abort(_('subrepository path is an existing file: %s')
+                             % path)
         kind = 'hg'
         if src.startswith('['):
             if ']' not in src: