pwclient: handle missing ~/.pwclientrc better

From: Mike Frysinger <vapier@chromium.org>

The upgrade path will crash if ~/.pwclientrc doesn't exist, so add an
explicit check for the file before we try to "upgrade" it.

The default error messages don't mention the config file, so it can be
a bit confusing what pwclient is complaining about when running.

Signed-off-by: Mike Frysinger <vapier@chromium.org>
---
 apps/patchwork/bin/pwclient | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Hi,

On Tue, 2015-05-05 at 23:37 -0400, Mike Frysinger wrote:
> From: Mike Frysinger <vapier@chromium.org>

> 

> The upgrade path will crash if ~/.pwclientrc doesn't exist, so add an

> explicit check for the file before we try to "upgrade" it.

> 

> The default error messages don't mention the config file, so it can be

> a bit confusing what pwclient is complaining about when running.

> 

> Signed-off-by: Mike Frysinger <vapier@chromium.org>

> ---

>  apps/patchwork/bin/pwclient | 6 +++---

>  1 file changed, 3 insertions(+), 3 deletions(-)

> 

> diff --git a/apps/patchwork/bin/pwclient b/apps/patchwork/bin/pwclient

> index 2a80981..56aa909 100755

> --- a/apps/patchwork/bin/pwclient

> +++ b/apps/patchwork/bin/pwclient

> @@ -552,7 +552,7 @@ def main():

>      config = ConfigParser.ConfigParser()

>      config.read([CONFIG_FILE])

>  

> -    if not config.has_section('options'):

> +    if not config.has_section('options') and os.path.exists(CONFIG_FILE):

>          sys.stderr.write('~/.pwclientrc is in the old format. Migrating it...')

>  


Doesn't it make more logical sense to check this in reverse order:

if os.path.exists(CONFIG_FILE) and not config.has_section('options'):

That makes more sense to read even if both end up working correctly?

>          old_project = config.get('base','project')

> @@ -590,10 +590,10 @@ def main():

>              sys.exit(1)

>  

>      if not config.has_section(project_str):

> -        sys.stderr.write("No section for project %s\n" % project_str)

> +        sys.stderr.write('No section for project %s in ~/.pwclientrc\n' % project_str)

>          sys.exit(1)

>      if not config.has_option(project_str, 'url'):

> -        sys.stderr.write("No URL for project %s\n" % project_str)

> +        sys.stderr.write('No URL for project %s in ~/.pwclientrc\n' % project_str)

>          sys.exit(1)

>      if not do_signoff and config.has_option('options', 'signoff'):

>          do_signoff = config.getboolean('options', 'signoff')


Regards,
Jake

On 06 May 2015 17:53, Keller, Jacob E wrote:
> On Tue, 2015-05-05 at 23:37 -0400, Mike Frysinger wrote:
> > --- a/apps/patchwork/bin/pwclient
> > +++ b/apps/patchwork/bin/pwclient
> > @@ -552,7 +552,7 @@ def main():
> >      config = ConfigParser.ConfigParser()
> >      config.read([CONFIG_FILE])
> >  
> > -    if not config.has_section('options'):
> > +    if not config.has_section('options') and os.path.exists(CONFIG_FILE):
> >          sys.stderr.write('~/.pwclientrc is in the old format. Migrating it...')
> 
> Doesn't it make more logical sense to check this in reverse order:
> 
> if os.path.exists(CONFIG_FILE) and not config.has_section('options'):
> 
> That makes more sense to read even if both end up working correctly?

the system already read & loaded the config file.  that is the normal code flow.  
so checking if it has the options section will pass quickly and thus skip the 
branch entirely.
-mike