| Message ID | bf74b6cd75bd886c1b5954693beeaccdfd2e51ec.1645208594.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4cf5d53b62a8e5fce64db97f830e00fa38bd0994 |
| Headers | show |
| Series | Add cat-file --batch-command flag | expand |
On Fri, Feb 18 2022, John Cai via GitGitGadget wrote: > From: John Cai <johncai86@gmail.com> > > 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@gmail.com> > --- > 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...
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@gmail.com> >> >> 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@gmail.com> >> --- >> 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.
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