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

[Guido Günther]

Hi,
On Fri, Jan 12, 2018 at 11:53:05AM -0800, Nish Aravamudan wrote:
> 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?

And the typo consequently copied to all locations. I've fixed this,
let's see if people are using it, if so we can bring back the variable
including the typo. The deprecations are all several years old so people
hopefully cleaned up already.
Thanks,
 -- Guido