Re: [PATCH 1/3] t5543: never report what we do not push

[Jiang Xin <worldhello.net@gmail.com>]

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".