> FTR, I'm not part of the devscripts team, the thoughts below are from > that perspective, I expect the team have a different one.
> On Thu, 2019-09-19 at 17:39 +0200, Nicolas Boulenguez wrote: > > > The devscripts config loading module got it wrong until recently, is > ----------------------------------------------------------^^^^^^^^ > A patch has been waiting for 19 months. > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=863118#65 > One of those patches adds some terrifying code in Confvar.pm that is > similar to the terrifying code that was in lib/Devscripts/Config.pm > before I fixed it recently in these two commits: > https://salsa.debian.org/debian/devscripts/commit/bd287e8f9d7f979c39010da681565fbcab2827f4 > https://salsa.debian.org/debian/devscripts/commit/595d5777a2bca6a7de5fb35a15d39e730253ab5d When writting Confvar, I have been careful to keep the existing behaviour. This seemed good separation of concerns. Confvar only searches in /etc/devscripts.conf and $HOME/.devscripts. $HOME is escaped by single quotes when invoking the subshell. Config introduces a new --conf[-]file option. It takes its own responsibilities. Confvar returns the correct value when a configuration file contains var=bla\ bla\'bla\"bla\$bla\\bla\\nbla (extract from test/test_confvar_*, which tests both Perl and Shell) > > The tests submitted alongside Confvar.pm, applied to Config.pm, would > > have detected the bug with special characters fixed by > > https://salsa.debian.org/debian/devscripts/commit/bd287e8f9d7f979c39010da681565fbcab2827f4 > That commit was what I referred to as "hacky", but I corrected it here: > https://salsa.debian.org/debian/devscripts/commit/595d5777a2bca6a7de5fb35a15d39e730253ab5d I do not understand your point. > I encourage you to update Confvar to use the style of parsing I have > implemented in the latest Config commit, it is as safe as is possible > without removing support for shell. Could you give an example of unsafety not yet covered by Confvar tests? I fail to understand what you mean by reading your commits. > > > [..] still needs migration of many of the scripts from their own > > > bad config loading implementations to the config loading module. > > Nice euphemism for "almost all". > Yeah, I expect the reason is that for many years devscripts didn't have > any way for scripts to share code between them. Everyone agrees that the old configuration must go away. My point was that your sentence suggests - that several scripts use Config. Actually, only one does. - that migrating the 24 other scripts is an obvious need. Another option is to migrate the 25 scripts to Confvar. The work is already done and partially tested. > > I can tell that Config.pm is not ready yet to cover the needs of all > > scripts. > Could you detail some of the needed differences? Config only targets perl, not shell. It does not anticipate the needs of * debuild.pl (DEBUILD_*_HOOKS, DEBUILD_*_OPTS, DEBUILD_SET_ENVVAR) * rmadison.pl (RMADISON_URL_MAP_*) I am confident that Confvar does because I have extended its interface while migrating and testing a script after the other. > > The longer Confvar.pm is completely ignored, the more efforts will be > > duplicated for Config.pm. Maintainers, please decide in favor of > > Confvar, Config, or a mix of both. > > Unless Confvar is updated to use the fixed parsing I added to Config, I > don't think it should be used. Could you detail some of the needed differences? > Even if it were updated, I think incremental improvements to Config > would be better than wholesale replacement. Even if Config brings valuable updates, like parsing of related command line arguments, I think incremental improvements to Confvar would have been, and would still be, better than wholesale replacement. > > Would a merge request on salsa be more useful than of a bunch of > > patches in the bug tracker? > > In my experience salsa merge requests are paid attention to more than > existing BTS patches. BTW, it is also possible to send merge requests > via email by attaching the output of `git format-patch` to an email: > > https://docs.gitlab.com/ce/user/project/merge_requests/#create-new-merge-requests-by-email It seemed right to answer a bug requesting help in the bug log, but let us try a merge request instead. https://salsa.debian.org/debian/devscripts/merge_requests/162
