Re: [PATCH (optional)] t0021: use arithmetic expansion to trim whitespace from wc -c output
- From
Lars Schneider <larsxschneider@gmail.com>
- Date
- Nov 6, 2016, 19:43 UTC
- Message-ID
- <283FAD19-35C3-4167-8946-A0F2A9CEDC57@gmail.com>
- In-Reply-To
- <b87ddffd-3de1-4481-b484-9f03a73b6ad1@kdbg.org>
Show 52 quoted lines
> On 06 Nov 2016, at 20:31, Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 06.11.2016 um 16:45 schrieb Lars Schneider:
>>
>>> On 03 Nov 2016, at 21:22, Johannes Sixt <j6t@kdbg.org> wrote:
>>> This is a pure optimization that reduces the number of forks, which
>>> helps a bit on Windows.
>>>
>>> There would be a solution with perl that does not require trimming
>>> of whitespace, but perl startup times are unbearable on Windows.
>>> wc -c is better.
>
> I was wrong here. I had looked at the perl invocations due to
> git-sendemail, and they are awfully slow. A do-almost-nothing perl
> invocation is in the same ballpark as wc. Therefore I changed my mind
> and suggest the patch below instead.
>
>> Since the file size function became very simple with your patch,
>> shouldn't we get rid of it? If you agree, then we could squash the
>> patch below into your patch.
>
> In the new patch, the function is not that trivial (it uses perl), and
> the call sites can remain as they are (simple shell variables and
> substitutions).
>
> ---- 8< ----
> [PATCH] t0021: compute file size with a single process instead of a pipeline
>
> Avoid unwanted coding patterns (prodigal use of pipelines), and in
> particular a useless use of cat.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> t/t0021-conversion.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index db71acacb3..cb72fa49de 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -22,7 +22,7 @@ generate_random_characters () {
> }
>
> file_size () {
> - cat "$1" | wc -c | sed "s/^[ ]*//"
> + perl -e 'print -s $ARGV[0]' "$1"
> }
>
> filter_git () {
> --
> 2.11.0.rc0.55.gd967357
> I like this better, too. Looks good to me and works on macOS.
Thanks, Lars