Hi Ævar,
On 19 Feb 2022, at 1:33, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Feb 18 2022, John Cai via GitGitGadget wrote:
>
>> From: John Cai <johncai86@xxxxxxxxx>
>>
>> maybe_remove_timestamp() takes arguments, but it would be useful to have
>> a function that reads from stdin and strips the timestamp. This would
>> allow tests to pipe data into a function to remove timestamps, and
>> wouldn't have to always assign a variable. This is especially helpful
>> when the data is multiple lines.
>>
>> Keep maybe_remove_timestamp() the same, but add a remove_timestamp
>> helper that reads from stdin.
>>
>> The tests in the next patch will make use of this.
>>
>> Signed-off-by: John Cai <johncai86@xxxxxxxxx>
>> ---
>> t/t1006-cat-file.sh | 15 ++++++++++-----
>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
>> index 145eee11df9..2d52851dadc 100755
>> --- a/t/t1006-cat-file.sh
>> +++ b/t/t1006-cat-file.sh
>> @@ -105,13 +105,18 @@ strlen () {
>> }
>>
>> maybe_remove_timestamp () {
>> - if test -z "$2"; then
>> - echo_without_newline "$1"
>> - else
>> - echo_without_newline "$(printf '%s\n' "$1" | sed -e 's/ [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$//')"
>> - fi
>> + if test -z "$2"; then
>> + echo_without_newline "$1"
>> + else
>> + echo_without_newline "$(printf '%s\n' "$1" | remove_timestamp)"
>> + fi
>> }
>>
>> +remove_timestamp () {
>> + sed -e 's/ [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$//'
>> +}
>> +
>> +
>> run_tests () {
>> type=$1
>> sha1=$2
>
> I may have missed some previous discussions, but is there a reason this
> echo_without_newline dance is needed? At this point this on top passes
> all tests for me on both dash and bash:
>
> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> index 2d52851dadc..8266a939f99 100755
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -104,18 +104,19 @@ strlen () {
> echo_without_newline "$1" | wc -c | sed -e 's/^ *//'
> }
>
> -maybe_remove_timestamp () {
> - if test -z "$2"; then
> - echo_without_newline "$1"
> - else
> - echo_without_newline "$(printf '%s\n' "$1" | remove_timestamp)"
> - fi
> -}
> -
> remove_timestamp () {
> sed -e 's/ [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$//'
> }
>
> +maybe_remove_timestamp () {
> + if test -n "$2"
> + then
> + echo "$1" | remove_timestamp
> + return 0
> + fi
> +
> + echo "$1"
> +}
>
> run_tests () {
> type=$1
>
> The move is another comment, if we're adding a remove_timestamp() let's
> define it before maybe_remove_timestamp() which uses it, even though in
> this case we can get away with it...
Thanks for these suggestions! I'll adjust 3/4 to include these changes.
