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

[Nish Aravamudan]

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 :)

-Nish

-- 
Nishanth Aravamudan
Ubuntu Server
Canonical Ltd