Jiang Xin <worldhello.net@xxxxxxxxx> writes:
> +format_git_output () {
Unless this helper is able to take any git output and massage,
please describe what kind of git output it is meant to handle.
Also, "format" does not tell anything to the readers why it wants to
transform its input or what its output is supposed to look like. It
does not help readers and future developers.
> + awk '/^(To| !) / {print}' | \
> + sed \
> + -e "s/ *\$//g" \
What's the point of /g? You are anchoring the pattern (i.e. one or
more SP) to the right end of the line, so it's not like it would
transform "a b c " into "abc". Also it would be sufficient to
say "zero or more" and that would be shorter, I think.
> + -e "s/'/\"/g"
It is unclear what this thing is for. If the output from a git
subcommand you are munging with the helper says <don't>, this will
turn it into <don"t>, presumably before comparing it with the
expected output you'd literally prepare in the test script. Are the
git subcommands whose output you are munging unstable and sometimes
use single and sometimes use double quotes?
If not, if you used single quotes when preparing the expected
output, wouldn't you be able to do without this?
Is it because you'd have the code that prepares the expected output
inside a sq pair (because it is in test_expect_thing), and it is
cumbersome to write a literal single quote? If that is the reason,
that is understandable, but I think readers deserve some comments
explaining why these transformations are done. Please do not waste
readers' time.
It looks wasteful to pipe awk output to sed. I wonder if something
along the lines of
sed -ne "/^\(To\| !\) /{
s/ *\$//
s/'/\"/g
p
}"
would do the same thing with a single tool.
> +# References in upstream : master(1) one(1) foo(1)
> +# References in workbench: master(2) foo(1) two(2) bar(2)
> +# Atomic push : master(2) two(2) bar(2)
> +test_expect_failure 'atomic push reports (reject by update hook)' '
> + mk_repo_pair &&
> + (
> + cd workbench &&
> + # Keep constant output.
> + git config core.abbrev 7 &&
This '7' is "use at least seven hexdigits", so it does not give any
guarantee that your output will be stable. Depending on chance,
some objects may be shown using 8 or more---is our "munge output
before comparison" helper prepared for such a case?
> + test_commit one &&
> + git branch foo &&
> + git push up master one foo &&
> + git tag -d one
> + ) &&
> + (
> + mkdir -p upstream/.git/hooks &&
> + cat >upstream/.git/hooks/update <<-EOF &&
> + #!/bin/sh
> +
> + if test "\$1" = "refs/heads/bar"
> + then
> + echo >2 "Pusing to branch bar is prohibited"
Meant to redirect to the standard error stream, not a file "2"?
> + exit 1
> + fi
> + EOF
> + chmod a+x upstream/.git/hooks/update
> + ) &&
> + (
> + cd workbench &&
> + test_commit two &&
> + git branch bar
> + ) &&
At this point, we have the refs described in the comment at the
beginning (which is greatly appreciated---it hels understanding what
is going on).
> + test_must_fail git -C workbench \
> + push --atomic up master two bar >out 2>&1 &&
As "git push" does not auto-follow tags, what we push will be
exactly these three refs.
> + format_git_output <out >actual &&
> + cat >expect <<-EOF &&
> + To ../upstream
> + ! [remote rejected] master -> master (atomic push failure)
> + ! [remote rejected] two -> two (atomic push failure)
> + ! [remote rejected] bar -> bar (hook declined)
> + EOF
And we expect them to be rejected, as 'bar' needs to stay constant
there.
> + test_cmp expect actual
All makes sense.
