| Message ID | b5950823ce90dd2476f002ed0370b7e0099a4d85.1573241590.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | learn the "summary" pretty format | expand |
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 > +'
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
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.
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 &&
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(+)