[Rpm-maint] [rpm-software-management/rpm] Implement one-shot macro modifier <o> (PR #4169)

[Michal Domonkos]

@dmnks commented on this pull request.

Minor nitpicks inline. Design-wise, I have [commented](https://github.com/rpm-software-management/rpm/issues/1155#issuecomment-4261566792) in the original ticket with some (additional) ideas, but otherwise, this looks good overall.

> @@ -292,8 +302,11 @@ callee(s).
 
 To define a global macro inside a parametric macro, you _must_ use *%global*
 instead of *%define*. Also note that because such a macro may be referring
-to other macros only visible in the current scope, *%global* _expands the
-macro body once at the time of definition_, unless it's a *l*iteral macro.
+to other macros only visible in the current scope, *%global* expands the
+macro body once at the time of definition, unless it's a *l*iteral macro.
+Note that the body of a *o*ne-shot macro defined via *%global* is actually
+expanded twice: once at time of definition, and the result of that expansion
+is expanded again at on first use.

Double preposition: "at on first use"

> @@ -724,7 +734,12 @@ doDefine(rpmMacroBuf mb, const std::string & str, int level, int expandbody, siz
     }
 
     /* Check for modifier compatibility */
-    if (o && (flags & ME_LITERAL)) {
+    if (o && (flags & (ME_LITERAL|ME_ONESHOT))) {
+	rpmMacroBufErr(mb, 1, _("Macro %%%s has incompatible modifiers\n"), n);

This error message was already added in commit ddcd6fae58ec29baa609e310ec29c6ea61f1f47c, but I wonder if it shouldn't be more precise, since the incompatibility here is not between the modifiers themselves but rather due to options being used *with* these particular modifiers.

> @@ -63,7 +63,17 @@ Macro behavior can be altered by supplying modifiers: (Added: 6.1.0)
 	_NAME_*<*_MODIFIERS_*>* _BODY_
 
 Supported modifiers are:
-- *l*iteral - macro body is literal, *%* is not processed when expanding
+- *l*: Literal expansion. *%* is not processed when expanding.
+- *o*: One-shot expansion. The macro is expanded on first use and the
+  literal result is pushed on top of the original macro.

I feel like the term "is pushed on top of" is a bit too low-level here. The end user may not necessarily know what that means. Would it make sense to instead use something like "replaces" here?

> @@ -63,7 +63,17 @@ Macro behavior can be altered by supplying modifiers: (Added: 6.1.0)
 	_NAME_*<*_MODIFIERS_*>* _BODY_
 
 Supported modifiers are:
-- *l*iteral - macro body is literal, *%* is not processed when expanding
+- *l*: Literal expansion. *%* is not processed when expanding.
+- *o*: One-shot expansion. The macro is expanded on first use and the
+  literal result is pushed on top of the original macro.
+
+Literal macros can simplify dealing with content that may contain
+percent signs (*%*) which otherwise would need to be escaped.
+
+One-shot macros are useful for caching expansion results, for example to
+guard a result against underlying changes, or to perform an expensive
+shell-invocation just once.

The hyphen in "shell-invocation" seemes extra here 😅 

-- 
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/4169#pullrequestreview-4127352234
You are receiving this because you are subscribed to this thread.

Message ID: <rpm-software-management/rpm/pull/4169/review/4127352234 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20260417/bce23f18/attachment-0001.htm>