| Message ID | 20200325143608.45141-2-zhiyou.jx@alibaba-inc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] t5543: never report what we do not push | expand |
Jiang Xin <worldhello.net@gmail.com> 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.
Junio C Hamano <gitster@pobox.com> 于2020年3月25日周三 下午11:05写道： > > Jiang Xin <worldhello.net@gmail.com> 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. > Wil use "cut_status_report_and_tidy" as the function name. > 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. I want to remove the trailing spaces in git output. I don't know why there are trailing spaces in each "remote: " message. But there is not trailing spaces in status report message, so I will remote it in next reroll. > > > + -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. To prepare expect message, sometime I have to escape the single quote like this cat >expect <<-EOF error: failed to push some refs to '"'"'../upstream'"'"' EOF It's boring to type '"'"' instead of a double quote. > > 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. That's better. > > +# 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? Will use sed -e 's/ */ /g' on output message to make it stable. > > > + 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"? It's a typo, should be "echo >&2".
diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh index 7079bcf9a0..4b4c0a262b 100755 --- a/t/t5543-atomic-push.sh +++ b/t/t5543-atomic-push.sh @@ -27,6 +27,13 @@ test_refs () { test_cmp expect actual } +format_git_output () { + awk '/^(To| !) / {print}' | \ + sed \ + -e "s/ *\$//g" \ + -e "s/'/\"/g" +} + test_expect_success 'atomic push works for a single branch' ' mk_repo_pair && ( @@ -191,4 +198,89 @@ test_expect_success 'atomic push is not advertised if configured' ' test_refs master HEAD@{1} ' +# 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 && + 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" + exit 1 + fi + EOF + chmod a+x upstream/.git/hooks/update + ) && + ( + cd workbench && + test_commit two && + git branch bar + ) && + test_must_fail git -C workbench \ + push --atomic up master two bar >out 2>&1 && + 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 + test_cmp expect actual +' + +# References in upstream : master(1) one(1) foo(1) +# References in workbench: master(2) foo(1) two(2) bar(2) +test_expect_failure 'atomic push reports (mirror, but reject by update hook)' ' + ( + cd workbench && + git remote remove up && + git remote add up ../upstream + ) && + test_must_fail git -C workbench \ + push --atomic --mirror up >out 2>&1 && + format_git_output <out >actual && + cat >expect <<-EOF && + To ../upstream + ! [remote rejected] master -> master (atomic push failure) + ! [remote rejected] one (atomic push failure) + ! [remote rejected] bar -> bar (hook declined) + ! [remote rejected] two -> two (atomic push failure) + EOF + test_cmp expect actual +' + +# References in upstream : master(2) one(1) foo(1) +# References in workbench: master(1) foo(1) two(2) bar(2) +test_expect_failure 'atomic push reports (reject by non-ff)' ' + rm upstream/.git/hooks/update && + ( + cd workbench && + git push up master && + git reset --hard HEAD^ + ) && + test_must_fail git -C workbench \ + push --atomic up master foo bar >out 2>&1 && + format_git_output <out >actual && + cat >expect <<-EOF && + To ../upstream + ! [rejected] master -> master (non-fast-forward) + ! [rejected] bar -> bar (atomic push failed) + EOF + test_cmp expect actual +' + test_done
When we push some references to the git server, we expect git to report the status of the references we are pushing; no more, no less. But when pusing with atomic mode, if some references cannot be pushed, Git reports the reject message on all references in the remote repository. Add new test cases in t5543, and fix them in latter commit. Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> --- t/t5543-atomic-push.sh | 92 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+)