"Basil L. Contovounesios" <[email protected]> writes: > Subject: [PATCH 1/4] announce-gen: Pacify POD errors.
Looks good to me! This seems unrelated to NEWS.md, so maybe best to just commit this asap to get it out of the review process. > 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. > Subject: [PATCH 3/4] maintainer-makefile: Avoid recursive $(shell) expansion. ... > -gnulib_dir ?= $(shell if test -n "$(GNULIB_SRCDIR)" && test -f > "$(GNULIB_SRCDIR)/gnulib-tool"; then \ > - echo "$(GNULIB_SRCDIR)"; \ > - else \ > - echo $(srcdir)/gnulib; \ > - fi) > +ifeq ($(origin gnulib_dir),undefined) > + gnulib_dir := $(shell if test -n "$(GNULIB_SRCDIR)" \ > + && test -f "$(GNULIB_SRCDIR)/gnulib-tool"; then > \ > + echo "$(GNULIB_SRCDIR)"; \ > + else \ > + echo $(srcdir)/gnulib; \ > + fi) > +endif What problem are you solving here? I know this part of cfg.mk is really horrible, and I often ran into bugs caused by this guessing. So this is fairly fragile code, and I worry that you may be breaking existing assumptions about this code. Not saying we shouldn't fix this, but it has to be done carefully. I think you are on to something here by using the 'undefined' comparison though. > 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. > # 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. > # 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? /Simon
signature.asc
Description: PGP signature
