[git-buildpackage] could gbp.config.GbpOptionParser.get_config_files handle GBP_CONF_FILES=''

[Nish Aravamudan]

On 12.01.2018 [09:28:15 +0100], Guido Günther wrote:
> Hi,
> On Thu, Jan 11, 2018 at 09:25:34AM -0800, Nish Aravamudan wrote:
> > On 11.01.2018 [18:04:08 +0100], Guido Günther wrote:
> > > Hi,
> > > On Thu, Jan 11, 2018 at 08:22:13AM -0800, Nish Aravamudan wrote:
> > > > On 11.01.2018 [08:39:10 +0100], Guido Günther wrote:
> > > > > Hi Nish,
> > > > > On Wed, Jan 10, 2018 at 02:21:52PM -0800, Nish Aravamudan wrote:
> > > > > > For the reproducibility/debuggability of the git-ubuntu snap, we want to
> > > > > > ensure that everyone will end up running certain commands (e.g., quilt,
> > > > > > gbp) the same, no matter what their local configuration is. This is
> > > > > > especially true for the importer itself, but is also true for other
> > > > > > subcommands in our case.
> > > > > > 
> > > > > > I wonder if it might be reasonable to do:
> > > > > > 
> > > > > > diff --git a/gbp/config.py b/gbp/config.py
> > > > > > index 8acf15da..c2feb2ab 100644
> > > > > > --- a/gbp/config.py
> > > > > > +++ b/gbp/config.py
> > > > > > @@ -409,7 +409,7 @@ class GbpOptionParser(OptionParser):
> > > > > >          >>> if conf_backup is not None: os.environ['GBP_CONF_FILES'] = conf_backup
> > > > > >          """
> > > > > >          envvar = os.environ.get('GBP_CONF_FILES')
> > > > > > -        files = envvar.split(':') if envvar else [f for (f, _) in cls.def_config_files]
> > > > > > +        files = envvar.split(':') if envvar is not None else [f for (f, _) in cls.def_config_files]
> > > > > >          files = [os.path.expanduser(fname) for fname in files]
> > > > > >          if no_local:
> > > > > >              files = [fname for fname in files if fname.startswith('/')]
> > > > > > 
> > > > > > This would allow environments that wish to disable config file parsing
> > > > > > (and thus behavioral variability) to juset set GBP_CONF_FILES='' rather
> > > > > > than to something like '/dev/null' (which seems to be the only choice
> > > > > > currently)?
> > > > > 
> > > > > Isn't it be more explicit to have GBP_CONF_FILES=/does/not/exist or
> > > > > GBP_CONF_FILES=/dev/null than to make a distinction between unset or
> > > > > set to the empty string? I'd rather not change the current behaviour.
> > > > 
> > > > I'm not sure, right now the behavior with GBP_CONF_FILES='' is
> > > > surprising -- it seems like it should mean no files are parsed, but
> > > > currently it means all the normal files are parsed.
> > > 
> > > Is this spelled out somewhere? When thinking in shell I would assume to
> > > that:
> > > 
> > >    if [ -z "${GBP_CONF_FILES}" ]; then
> > >      do something
> > >    fi
> > > 
> > > do the same regardless if GBP_CONF_FILES is unset or
> > > GBP_CONF_FILES=''. This wouldn't be true anymore. (and in fact that's
> > > the only reason I have to justify the current behavior).
> > 
> > Yeah, I can see both sides to the choice, admittedly.
> > 
> > I'm going by this text (`man gbp.conf`):
> > 
> >        GBP_CONF_FILES
> >               Colon separated list of files to parse. The default is the above
> >               list of configuration files.
> > 
> > 
> > If I specify an empty colon-separated list of files, I expected none to
> > be parsed. I would be happy with just an amendment to that manpage :)
> 
>    https://github.com/agx/git-buildpackage/commit/e36592d4b8c37756100dddbe0d6a1332bf31d57f

+1. I did notice that the commit duplicates a typo from the above, which
I guess is now history -- DEPRECTATION instead of DEPRECATION?

-- 
Nishanth Aravamudan
Ubuntu Server
Canonical Ltd