Re: [PATCH v3 1/2] maint.mk: Split long argument lists

2019-01-02 Thread Eric Blake
On 12/13/18 9:34 AM, Roman Bolshakov wrote:
> $(VC_LIST_EXCEPT) is usually expanded into arguments for a command.
> When a project contains too many, some operating systems can't pass all
> the arguments because they hit the limit of arguments. FreeBSD and macOS
> are known to have the limit of 256k of arguments.
> 
> More on the issue:
> http://lists.gnu.org/archive/html/bug-gnulib/2015-08/msg00019.html
> https://www.redhat.com/archives/libvir-list/2015-August/msg00758.html
> 
> xargs without flags can be used to limit number of arguments.

This part is fine.

> The
> default number of arguments (max-args for "-n" flag) is
> platform-specific. If argument length exceeds default value for "-s"
> flag (max-chars), xargs will feed less arguments than max-args.

This was relevant to earlier versions, but is now just fluff; I'd drop
it.  What's more, you didn't mention that we are relying on 'echo
$(long_list) | xargs' working because the shell's built-in echo does not
use exec and can therefore process a larger list than what exec
enforces.  I don't mind tweaking that on commit.

> 
> Signed-off-by: Roman Bolshakov 
> ---
>  top/maint.mk | 135 ---
>  1 file changed, 86 insertions(+), 49 deletions(-)
> 
> diff --git a/top/maint.mk b/top/maint.mk
> index 4889ebacc..1152ad698 100644
> --- a/top/maint.mk
> +++ b/top/maint.mk
> @@ -302,32 +302,46 @@ define _sc_search_regexp
> fi;   
> \
>   \
> : Filter by content;  
> \
> -   test -n "$$files" && test -n "$$containing"   
> \
> - && { files=$$(grep -l "$$containing" $$files); } || :;  \
> -   test -n "$$files" && test -n "$$non_containing"   \
> - && { files=$$(grep -vl "$$non_containing" $$files); } || :; \
> +   test -n "$$files" \
> + && test -n "$$containing"   
> \

It feels like extra churn to have split this line with no change to its
content, but I also don't mind keeping it split.

> + && { files=$$(echo "$$files" | xargs grep -l "$$containing"); } \
> + || :;   \
> +   test -n "$$files" \
> + && test -n "$$non_containing"   \
> + && { files=$$(echo "$$files" | xargs grep -vl "$$non_containing"); } \
> + || :;   \
>   \
> : Check for the construct;
> \
> if test -n "$$files"; then
> \
>   if test -n "$$prohibit"; then   \
> -   grep $$with_grep_options $(_ignore_case) -nE "$$prohibit" $$files \
> +   echo "$$files" /dev/null  
> \
> + | xargs \
> + grep $$with_grep_options $(_ignore_case) -nE "$$prohibit"   
> \

Not quite canonical. That can cause xargs to invoke:
grep a b c
grep /dev/null

instead of the more typical:

grep /dev/null a b
grep /dev/null c

Remember, 'grep one' output differs from 'grep one two' - so the GOAL is
to ensure that grep always has at least two file, by always including
/dev/null in the list of arguments to EVERY grep invocation, rather than
only to the LAST grep invocation.

But, that said, your formulation "works" in the sense that it is
unlikely that any one single filename would push beyond exec limits, and
therefore xargs will either make a single call (no difference); or split
things so that the first call has more than one file (no /dev/null, but
since there are multiple files things work) and the second call has just
/dev/null (which will have no output, so it won't matter that grep was
called with one file instead of multiple); or split things so that the
first has more than one file, and the second has more than one file
(including /dev/null).

I'm still inclined to touch this up to use the canonical formulation:

'echo $(long_list) | xargs grep /dev/null'

rather than your formulation:

'echo $(long_list) /dev/null | xargs grep'

just so future readers don't have to revisit why yours happens to work.

>   | grep -vE "$${exclude:-^$$}"   
> \
> - && { msg="$$halt" $(_sc_say_and_exit) } || :;   
> \
> + && { msg="$$halt" $(_sc_say_and_exit) } \
> + || :;   
> \

Again, this change is just churn.  I don't know that I 

[PATCH v3 1/2] maint.mk: Split long argument lists

2018-12-13 Thread Roman Bolshakov
$(VC_LIST_EXCEPT) is usually expanded into arguments for a command.
When a project contains too many, some operating systems can't pass all
the arguments because they hit the limit of arguments. FreeBSD and macOS
are known to have the limit of 256k of arguments.

More on the issue:
http://lists.gnu.org/archive/html/bug-gnulib/2015-08/msg00019.html
https://www.redhat.com/archives/libvir-list/2015-August/msg00758.html

xargs without flags can be used to limit number of arguments. The
default number of arguments (max-args for "-n" flag) is
platform-specific. If argument length exceeds default value for "-s"
flag (max-chars), xargs will feed less arguments than max-args.

Signed-off-by: Roman Bolshakov 
---
 top/maint.mk | 135 ---
 1 file changed, 86 insertions(+), 49 deletions(-)

diff --git a/top/maint.mk b/top/maint.mk
index 4889ebacc..1152ad698 100644
--- a/top/maint.mk
+++ b/top/maint.mk
@@ -302,32 +302,46 @@ define _sc_search_regexp
fi; \
\
: Filter by content;
\
-   test -n "$$files" && test -n "$$containing" \
- && { files=$$(grep -l "$$containing" $$files); } || :;\
-   test -n "$$files" && test -n "$$non_containing" \
- && { files=$$(grep -vl "$$non_containing" $$files); } || :;   \
+   test -n "$$files"   \
+ && test -n "$$containing" \
+ && { files=$$(echo "$$files" | xargs grep -l "$$containing"); }   \
+ || :; \
+   test -n "$$files"   \
+ && test -n "$$non_containing" \
+ && { files=$$(echo "$$files" | xargs grep -vl "$$non_containing"); } \
+ || :; \
\
: Check for the construct;  \
if test -n "$$files"; then  \
  if test -n "$$prohibit"; then \
-   grep $$with_grep_options $(_ignore_case) -nE "$$prohibit" $$files \
+   echo "$$files" /dev/null
\
+ | xargs   \
+ grep $$with_grep_options $(_ignore_case) -nE "$$prohibit" \
  | grep -vE "$${exclude:-^$$}" \
- && { msg="$$halt" $(_sc_say_and_exit) } || :; \
+ && { msg="$$halt" $(_sc_say_and_exit) }   \
+ || :; \
  else  \
-   grep $$with_grep_options $(_ignore_case) -LE "$$require" $$files \
-   | grep .\
- && { msg="$$halt" $(_sc_say_and_exit) } || :; \
+   echo "$$files"  \
+ | xargs   \
+ grep $$with_grep_options $(_ignore_case) -LE "$$require"  \
+ | grep .  \
+ && { msg="$$halt" $(_sc_say_and_exit) }   \
+ || :; \
  fi
\
else :; \
fi || :;
 endef
 
 sc_avoid_if_before_free:
-   @$(srcdir)/$(_build-aux)/useless-if-before-free \
-   $(useless_free_options) \
-   $$($(VC_LIST_EXCEPT) | grep -v useless-if-before-free) &&   \
- { echo '$(ME): found useless "if" before "free" above' 1>&2;  \
-   exit 1; } || :
+   @$(VC_LIST_EXCEPT)  \
+ | grep -v useless-if-before-free  \
+ | xargs   \
+ $(srcdir)/$(_build-aux)/useless-if-before-free\
+ $(useless_free_options)   \
+ && { printf '$(ME): found useless "if"'   \
+ ' before "free" above\n' 1>&2;\
+  exit 1; }\
+ || :
 
 sc_cast_of_argument_to_free:
@prohibit='\&2;   \
-   exit 1; } || :
+