[v2,07/10] t4205: cover `git log --reflog -z` blindspot
diff mbox series

Message ID b5950823ce90dd2476f002ed0370b7e0099a4d85.1573241590.git.liu.denton@gmail.com
State New
Headers show
Series
  • learn the "summary" pretty format
Related show

Commit Message

Denton Liu Nov. 8, 2019, 8:08 p.m. UTC
The test suite does not include any tests where `--reflog` and `-z` are
used together in `git log`. Cover this blindspot. Note that the
`--pretty=oneline` case is written separately because it follows a
slightly different codepath.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4205-log-pretty-formats.sh | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Comments

Eric Sunshine Nov. 8, 2019, 8:36 p.m. UTC | #1
On Fri, Nov 8, 2019 at 3:08 PM Denton Liu <liu.denton@gmail.com> wrote:
> The test suite does not include any tests where `--reflog` and `-z` are
> used together in `git log`. Cover this blindspot. Note that the
> `--pretty=oneline` case is written separately because it follows a
> slightly different codepath.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> @@ -134,6 +134,41 @@ test_expect_failure C_LOCALE_OUTPUT 'NUL termination with --stat' '
> +emit_nul () {
> +       echo | tr '\n' '\000'
> +}

A simple:

    printf "\0"

would be simpler, and I don't think you even need to introduce a shell
function to encapsulate it, as it's quite clear at a glance what it
does.

> +for p in short medium full fuller email raw
> +do
> +       test_expect_success "NUL termination with --reflog --pretty=$p" '
> +               >expect &&

You can drop this line...

> +               revs="$(git rev-list --reflog)" &&
> +               for r in $revs
> +               do
> +                       git show -s "$r" --pretty='$p' >>expect || return 1
> +                       emit_nul >>expect

...and simplify all this capturing into 'expect'...

> +               done &&

... by just redirecting the output of the for-loop itself:

    for r in $(git rev-list --reflog)
    do
        git show -s --pretty="$p" "$r" &&
        printf "\0" || return 1
    done >expect &&

For completeness, the above example also drops the unnecessary 'revs'
variable, uses double quotes rather than single when interpolating $p,
and makes the loop early-exit a bit more idiomatic.

> +               git log -z --reflog --pretty='$p' >actual &&
> +               emit_nul >>actual &&

Likewise, you can capture 'actual' in its entirety:

    {
        git log -z --reflog --pretty="$p" &&
        printf "\0"
    } >actual &&

> +               test_cmp expect actual
> +       '
> +done
> +
> +test_expect_success 'NUL termination with --reflog --pretty=oneline' '
> +       >expect &&
> +       revs="$(git rev-list --reflog)" &&
> +       for r in $revs
> +       do
> +               # trim trailing newline
> +               output="$(git show -s --pretty=oneline "$r")" || return 1
> +               printf "%s" "$output" >>expect
> +               emit_nul >>expect
> +       done &&

Replacing the newline with NUL could be done more simply and
idiomatically (with regard to other test scripts) by passing the
output of "git show" through the lf_to_nul() function from
test-lib-functions.sh. Something like this should do it:

    for r in $(git rev-list --reflog)
    do
        git show -s --pretty=oneline "$r" >raw &&
        cat raw | lf_to_nul || return 1
    done >expect &&

> +       git log -z --pretty=oneline --reflog >actual &&
> +       # no trailing NUL

To what is this comment referring?

> +       test_cmp expect actual
> +'
Denton Liu Nov. 8, 2019, 9:47 p.m. UTC | #2
Hi Eric,

On Fri, Nov 08, 2019 at 03:36:25PM -0500, Eric Sunshine wrote:

[...]

>     for r in $(git rev-list --reflog)
>     do
>         git show -s --pretty="$p" "$r" &&
>         printf "\0" || return 1
>     done >expect &&
> 
> For completeness, the above example also drops the unnecessary 'revs'
> variable, uses double quotes rather than single when interpolating $p,
> and makes the loop early-exit a bit more idiomatic.

The reason why I pulled out `revs` was because putting the rev-list
within the for-loop would cause its exit code to be lost if it ever
failed. Since I've been cleaning up test scripts to not lose exit codes
from git commands, I figured it'd be a bad idea to introduce an instance
here ;)

Thanks for the other two suggestions, though. I didn't know that you
could write it like that.

> > +       git log -z --pretty=oneline --reflog >actual &&
> > +       # no trailing NUL
> 
> To what is this comment referring?

I wanted to point out how in the oneline case, the trailing NUL is
already included, unlike in the multiple cases, so we don't need to
output another one.

I'll rewrite this as

	# the trailing NUL is already produced so we don't need to
	# output another one

Thanks for your feedback,

Denton
Eric Sunshine Nov. 8, 2019, 9:58 p.m. UTC | #3
On Fri, Nov 8, 2019 at 4:47 PM Denton Liu <liu.denton@gmail.com> wrote:
> On Fri, Nov 08, 2019 at 03:36:25PM -0500, Eric Sunshine wrote:
> > For completeness, the above example also drops the unnecessary 'revs'
> > variable, uses double quotes rather than single when interpolating $p,
> > and makes the loop early-exit a bit more idiomatic.
>
> The reason why I pulled out `revs` was because putting the rev-list
> within the for-loop would cause its exit code to be lost if it ever
> failed. Since I've been cleaning up test scripts to not lose exit codes
> from git commands, I figured it'd be a bad idea to introduce an instance
> here ;)

Make sense. Ignore that suggestion of mine.

> > > +       git log -z --pretty=oneline --reflog >actual &&
> > > +       # no trailing NUL
> >
> > To what is this comment referring?
>
> I wanted to point out how in the oneline case, the trailing NUL is
> already included, unlike in the multiple cases, so we don't need to
> output another one.
>
> I'll rewrite this as
>
>         # the trailing NUL is already produced so we don't need to
>         # output another one

That will be clearer. It might be even clearer if that comment comes
before the "git log -z" invocation rather than after (though that's
subjective). Thanks.

Patch
diff mbox series

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index f42a69faa2..d35650cae7 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -134,6 +134,41 @@  test_expect_failure C_LOCALE_OUTPUT 'NUL termination with --stat' '
 	test_cmp expected actual
 '
 
+emit_nul () {
+	echo | tr '\n' '\000'
+}
+
+for p in short medium full fuller email raw
+do
+	test_expect_success "NUL termination with --reflog --pretty=$p" '
+		>expect &&
+		revs="$(git rev-list --reflog)" &&
+		for r in $revs
+		do
+			git show -s "$r" --pretty='$p' >>expect || return 1
+			emit_nul >>expect
+		done &&
+		git log -z --reflog --pretty='$p' >actual &&
+		emit_nul >>actual &&
+		test_cmp expect actual
+	'
+done
+
+test_expect_success 'NUL termination with --reflog --pretty=oneline' '
+	>expect &&
+	revs="$(git rev-list --reflog)" &&
+	for r in $revs
+	do
+		# trim trailing newline
+		output="$(git show -s --pretty=oneline "$r")" || return 1
+		printf "%s" "$output" >>expect
+		emit_nul >>expect
+	done &&
+	git log -z --pretty=oneline --reflog >actual &&
+	# no trailing NUL
+	test_cmp expect actual
+'
+
 test_expect_success 'setup more commits' '
 	test_commit "message one" one one message-one &&
 	test_commit "message two" two two message-two &&