Simon Josefsson [2025-11-15 13:21 +0100] wrote: > "Basil L. Contovounesios" <[email protected]> writes: > >> Subject: [PATCH 2/4] announce-gen: Accommodate alternative NEWS formats. > ... >> + --news-prefix=PRE string identifying the start of a NEWS >> section >> + heading > ... >> -between versions C<$prev_version> and C<$curr_version>. >> +between versions C<$prev_version> and C<$curr_version>. Expects the >> +corresponding section headings to start with C<$news_pre>. > ... >> - my $re_prefix = qr/(?:\* )?(?:Noteworthy c|Major c|C)(?i:hanges)/; >> + my $re_prefix = qr/(?:\Q$news_pre\E+ )?(?:Noteworthy c|Major >> c|C)(?i:hanges)/; > > The patch is okay by me, but I'm a bit surprised here -- reading just > the documentation I would get the impression that the --news-prefix > string would be the 'Noteworth changes' regexp string. Would it make > sense to move more of this hard-coded magical string into --news-prefix? > > No objection to push this patch, but would be nice to fix the above if > you have ideas how to achieve that.
Originally I was thinking of making the whole regexp configurable, but I had two concerns, that: 1. accepting a Perl regexp as input opens the program up to arbitrary code execution (but I am not a Perl programmer, so perhaps this concern is misplaced); and 2. the default regexp already allows for six distinct heading styles, which is more than in any other Gnulib tool. So, if a user wanted to specify an extra pattern (in addition to the default patterns), they would need to replicate the default patterns. If do-release-commit-and-tag's API is treated as safe enough in environments that use Gnulib, then concern (1) can be ignored. Some mitigating ideas: a. Assign fixed names to a set of 'standard' heading patterns, and allow the command-line option to pick from among them. This is less automatic, but avoids arbitrary regexp input. b. Include the command-line argument as an extra alternative to the current alternatives. Overrides won't need to replicate the default patterns, but it still requires arbitrary regexp input. c. Allow the command-line argument to override the whole set of patterns. This requires arbitrary regexp input, but overrides can be more precise. d. Extend the default patterns to match all reasonably conceivable heading patterns. This addresses both concerns (1) and (2), and allows automatic Markdown detection, but is not configurable. e. Document --news-prefix better and keep its current limited scope. Thoughts/suggestions? >> Subject: [PATCH 4/4] maintainer-makefile: Auto-detect NEWS.md. > > I have some concerns about commiting this right now: > >> +# NEWS takes precedence over NEWS.md. >> +# Override this in cfg.mk if you use a different file name. >> +ifeq ($(origin NEWS_file),undefined) >> + NEWS_file := NEWS$(if $(wildcard $(srcdir)/NEWS),,$\ >> + $(and $(wildcard $(srcdir)/NEWS.md),.md)) >> +endif > > Isn't this a bit too conservative? If NEWS.md exists, I think the > default should be to use it automatically. No objections from me. >> # An ERE quoted for the shell, for matching a version+date line prefix. >> -news-check-regexp ?= '^\*.* $(VERSION_REGEXP) \($(today)\)' >> +news-check-regexp ?= \ >> + '^$(if $(filter %.md,$(NEWS_file)),#,\*).* $(VERSION_REGEXP) \($(today)\)' >> >> # Like news-check-regexp, but as an unquoted BRE for .prev-version. >> -news-check-regexp-prev ?= ^\*.* $(PREV_VERSION_REGEXP) ([0-9-]*) >> +news-check-regexp-prev ?= \ >> + ^$(if $(filter %.md,$(NEWS_file)),#,\*).* $(PREV_VERSION_REGEXP) ([0-9-]*) > > Couldn't we accept both syntaxes in either file? It will be a hassle to > main separate regexp's for each file. > >> + $$(case $(NEWS_file) in (*.md) echo --news-prefix=#;; esac) \ > > Ouch. I think announce-gen should support both formats automatically. Also fine with me. In which case we can do away with the --news-prefix option altogether and adopt strategy (d) above, right? >> # Keep consistent with news-check-regexp and news-check-regexp-prev. >> -gl_noteworthy_news_ ?= * Noteworthy changes in release ?.? (????-??-??) [?] >> +gl_noteworthy_news_ ?= \ >> + $(if $(filter %.md,$(NEWS_file)),#,*) \ >> + Noteworthy changes in release ?.? (????-??-??) [?] > > Same here, what about a regexp that covers both formats? The problem here is that this is a literal string to be inserted, so it should probably match the expected heading style. Thanks, -- Basil
