[libvirt] [PATCH v2 3/3] maint: split long lines for BSD syntax-check

[Eric Blake]

On 1/7/19 5:00 AM, Ján Tomko wrote:
> On Thu, Jan 03, 2019 at 01:41:59PM -0600, Eric Blake wrote:
>> Similar to the gnulib changes we just incorporated into maint.mk,
>> it's time to use '$(VC_LIST) | xargs program' instead of
>> 'program $$($(VC_LIST))', in order to bypass the problem of hitting
>> argv limits due to our large set of files.
>>
>> Drop several uses of $$files as a temporary variable when we can
>> instead directly use xargs. While at it, fix a typo in the
>> prohibit_windows_special_chars error message.
>>

>> -    @{ $(GREP)     -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \
>> -       $(GREP) -A1 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT));
>> } \
>> +    @{ $(VC_LIST_EXCEPT) | xargs \
>> +        $(GREP)     -nE '\<$(func_re) *\(.*;$$' /dev/null; \
>> +       $(VC_LIST_EXCEPT) | xargs \
>> +        $(GREP) -A1 -nE '\<$(func_re) *\(.*,$$' /dev/null; } \
> 
> Not sure why the /dev/null is needed.

Because of grep semantics.

grep pattern file1
grep pattern file1 file2

have different output, and the syntax checkers depend on the grep output
that is produced when there are multiple files on the command line.  The
canonical rewrite of 'grep pattern $(list of files)" into xargs uses
"list of files | xargs grep pattern /dev/null", because you presume that
'list of files' produced a non-empty list, but worry that xargs might do
a worst-case split into 'grep pattern all but one; grep pattern last',
where the different invocation of the last file name alone produces
unusual output that messes up the subsequent pipeline.  Putting
/dev/null in the command line argument to grep ensures that we instead
get a split into 'grep pattern /dev/null all but one; grep pattern
/dev/null last' where all invocations are with at least two file names.

The case of 0 files is different - for that, you would normally use
'xargs -r' to ensure that grep is never called with 0 arguments. But
since grep'ing /dev/null will never produce output, we don't need to
worry about xargs -r, even if we could end up with an empty list.  And
since the old code wasn't worrying about an empty list, I didn't see a
reason to worry about an empty list in the new code either.

> 
> If the syntax check rule were to operate on an empty list of files, we
> can just delete it.
> 
>>        | $(SED) 's/_("\([^\"]\|\\.\)\+"//;s/[     ]"%s"//' \
>>        | $(GREP) '[     ]"' && \
>>       { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \
> 
> With the /dev/null changes removed or justified:

I hope that was the justification you were looking for (and it matches
the gnulib rewrite that uses /dev/null in every conversion to xargs grep
where grep's output differs based on number of file names present - note
that 'grep -L' does not have different output, so it doesn't need the
/dev/null trick, but we didn't have uses of grep -L in cfg.mk).

> 
> Reviewed-by: Ján Tomko <jtomko at redhat.com>
> 
> Jano

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190107/fda985f2/attachment-0001.sig>