[v10,3/4] cat-file: add remove_timestamp helper

Commit Message

John Cai Feb. 18, 2022, 6:23 p.m. UTC

From: John Cai <[e-mail]>

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 <[e-mail]>
---
 t/t1006-cat-file.sh | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Feb. 19, 2022, 6:33 a.m. UTC | #1

On Fri, Feb 18 2022, John Cai via GitGitGadget wrote:

> From: John Cai <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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...

John Cai Feb. 22, 2022, 3:31 a.m. UTC | #2

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 <[e-mail]>
>>
>> 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 <[e-mail]>
>> ---
>>  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.

Patch

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