[PATCH 0/1] ci: new github-action for git-l10n code review

[PATCH 0/1] ci: new github-action for git-l10n code review

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Git l10n uses github pull request for code review. A helper program
"git-po-helper" can be used to check typos in ".po" files, validate
syntax, and check commit message. It would be convenient to integrate
this helper program to CI and add comments in pull request.

A repository is created for testing git-l10n CI workflow. L10n
contributors can fork and try.

- https://github.com/jiangxin/github-action-test


Jiang Xin (1):
  ci: new github-action for git-l10n code review

 .github/workflows/l10n.yml | 143 +++++++++++++++++++++++++++++++++++++
 1 file changed, 143 insertions(+)
 create mode 100644 .github/workflows/l10n.yml

-- 
2.33.0

[PATCH 1/1] ci: new github-action for git-l10n code review

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Git l10n uses github pull request for code review. A helper program
"git-po-helper" can be used to check typos in ".po" files, validate
syntax, and check commit message. It would be convenient to integrate
this helper program to CI and add comments in pull request.

The new github-action workflow is added in ".github/workflows/l10n.yml",
which is disabled by default. To turn it on for the git-l10n related
repositories, such as "git-l10n/git-po", we can add a new branch named
"ci-config" and create a simple shell script at "ci/config/allow-l10n"
in this branch.

The new l10n workflow listens to two types of github events: push and
pull_request.

For a push event, it will scan commits one by one. If a commit does not
look like a l10n commit (no file in "po/" has been changed), it will
immediately fail without checking for further commits. While for a
pull_request event, all new introduced commits will be scanned.

"git-po-helper" will generate two kinds of suggestions, errors and
warnings. A l10n contributor should try to fix all the errors, and
should pay attention to the warnings. All the errors and warnings will
be reported in the last step of the l10n workflow as two message groups.
For a pull_request event, will create additional comments in pull
request to report the result.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 .github/workflows/l10n.yml | 143 +++++++++++++++++++++++++++++++++++++
 1 file changed, 143 insertions(+)
 create mode 100644 .github/workflows/l10n.yml

diff --git a/.github/workflows/l10n.yml b/.github/workflows/l10n.yml
new file mode 100644
index 0000000000..60f162c121
--- /dev/null
+++ b/.github/workflows/l10n.yml
@@ -0,0 +1,143 @@
+name: git-l10n
+
+on: [push, pull_request]
+
+jobs:
+  ci-config:
+    runs-on: ubuntu-latest
+    outputs:
+      enabled: ${{ steps.check-l10n.outputs.enabled }}
+    steps:
+      - name: try to clone ci-config branch
+        run: |
+          git -c protocol.version=2 clone \
+            --no-tags \
+            --single-branch \
+            -b ci-config \
+            --depth 1 \
+            --no-checkout \
+            --filter=blob:none \
+            https://github.com/${{ github.repository }} \
+            config-repo &&
+          cd config-repo &&
+          git checkout HEAD -- ci/config || : ignore
+      - id: check-l10n
+        name: check whether CI is enabled for l10n
+        run: |
+          enabled=no
+          if test -x config-repo/ci/config/allow-l10n &&
+             config-repo/ci/config/allow-l10n '${{ github.ref }}'
+          then
+            enabled=yes
+          fi
+          echo "::set-output name=enabled::$enabled"
+
+  git-po-helper:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
+      with:
+        fetch-depth: '0'
+    - uses: actions/setup-go@v2
+      with:
+        go-version: ">=1.16"
+    - name: Install git-po-helper
+      run: |
+        go install github.com/git-l10n/git-po-helper@main
+    - name: Install other dependencies
+      run: |
+        sudo apt-get update -q &&
+        sudo apt-get install -q -y gettext
+    - id: check-commits
+      name: Run git-po-helper
+      run: |
+        if test "${{ github.event_name }}" = "pull_request"
+        then
+          commit_from=${{ github.event.pull_request.base.sha }}
+          commit_to=${{ github.event.pull_request.head.sha }}
+        else
+          commit_from=${{ github.event.before }}
+          commit_to=${{ github.event.after }}
+          if ! echo $commit_from | grep -q "^00*$"
+          then
+            if ! git rev-parse "$commit_from^{commit}"
+            then
+              git fetch origin $commit_from
+            fi
+          fi
+        fi
+        exit_code=0
+        git-po-helper check-commits --github-action -- \
+          $commit_from..$commit_to >git-po-helper.out 2>&1 ||
+        exit_code=$?
+        echo "::set-output name=exit_code::$exit_code"
+        has_error_msg=
+        has_warning_msg=
+        if test $exit_code -ne 0
+        then
+          has_error_msg=yes
+          if test "${{ github.event_name }}" = "pull_request"
+          then
+            echo "ERROR_MSG<<EOF" >>$GITHUB_ENV
+            grep -v -e "^level=warning" -e WARNING git-po-helper.out |
+              perl -pe 's/\e\[[0-9;]*m//g' >>$GITHUB_ENV
+            echo "EOF" >>$GITHUB_ENV
+          fi
+        fi
+        if grep -q -e "^level=warning" -e WARNING git-po-helper.out
+        then
+          has_warning_msg=yes
+          if test "${{ github.event_name }}" = "pull_request"
+          then
+            echo "WARNING_MSG<<EOF" >>$GITHUB_ENV
+            grep -v -e "^level=error" -e ERROR git-po-helper.out |
+              perl -pe 's/\e\[[0-9;]*m//g' >>$GITHUB_ENV
+            echo "EOF" >>$GITHUB_ENV
+          fi
+        fi
+        echo "::set-output name=has_error_msg::$has_error_msg"
+        echo "::set-output name=has_warning_msg::$has_warning_msg"
+    - name: Report errors in comment for pull request
+      uses: mshick/add-pr-comment@v1
+      if: steps.check-commits.outputs.has_error_msg == 'yes' && github.event_name == 'pull_request'
+      continue-on-error: true
+      with:
+        repo-token: ${{ secrets.GITHUB_TOKEN }}
+        repo-token-user-login: 'github-actions[bot]'
+        message: |
+          Errors found by git-po-helper in workflow ${{ github.workflow }}:
+          ```
+          ${{ env.ERROR_MSG }}
+          ```
+    - name: Report warnings in comment for pull request
+      uses: mshick/add-pr-comment@v1
+      if: steps.check-commits.outputs.has_warning_msg == 'yes' && github.event_name == 'pull_request'
+      continue-on-error: true
+      with:
+        repo-token: ${{ secrets.GITHUB_TOKEN }}
+        repo-token-user-login: 'github-actions[bot]'
+        message: |
+          Warnings found by git-po-helper in workflow ${{ github.workflow }}:
+          ```
+          ${{ env.WARNING_MSG }}
+          ```
+    - name: Report and quit
+      run: |
+        if test "${{ steps.check-commits.outputs.has_error_msg }}" = "yes"
+        then
+          echo "::group::Errors found by git-po-helper"
+          grep -v -e "^level=warning" -e WARNING git-po-helper.out
+          echo "::endgroup::"
+        fi
+        if test "${{ steps.check-commits.outputs.has_warning_msg }}" = "yes"
+        then
+          echo "::group::Warnings found by git-po-helper"
+          grep -v -e "^level=error" -e ERROR git-po-helper.out
+          echo "::endgroup::"
+        fi
+        if test ${{ steps.check-commits.outputs.exit_code }} -ne 0
+        then
+          exit ${{ steps.check-commits.outputs.exit_code }}
+        fi
-- 
2.33.0

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Bagas Sanjaya]

On 22/08/21 23.13, Jiang Xin wrote:
> +    - uses: actions/setup-go@v2
> +      with:
> +        go-version: ">=1.16"

Currently there is newer Go (1.17) [1], why don't you update to it?

[1]: https://golang.org/dl/go1.17.linux-amd64.tar.gz

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Jiang Xin]

On Mon, Aug 23, 2021 at 2:28 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> On 22/08/21 23.13, Jiang Xin wrote:
> > +    - uses: actions/setup-go@v2
> > +      with:
> > +        go-version: ">=1.16"
>
> Currently there is newer Go (1.17) [1], why don't you update to it?

Only go 1.16 and above can install a go package in "path@revision"
format. Use semversion ">=1.16" fits my needs and makes this workflow
file stable. It's not good to upgrade this yml file regularly.  And
I'm not sure "action/setup-go" has a cache for go 1.17.

--
Jiang Xin

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Johannes Schindelin]

Hi,

On Mon, 23 Aug 2021, Jiang Xin wrote:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> Git l10n uses github pull request for code review. A helper program
> "git-po-helper" can be used to check typos in ".po" files, validate
> syntax, and check commit message. It would be convenient to integrate
> this helper program to CI and add comments in pull request.
>
> The new github-action workflow is added in ".github/workflows/l10n.yml",
> which is disabled by default. To turn it on for the git-l10n related
> repositories, such as "git-l10n/git-po", we can add a new branch named
> "ci-config" and create a simple shell script at "ci/config/allow-l10n"
> in this branch.
>
> The new l10n workflow listens to two types of github events: push and
> pull_request.
>
> For a push event, it will scan commits one by one. If a commit does not
> look like a l10n commit (no file in "po/" has been changed), it will
> immediately fail without checking for further commits. While for a
> pull_request event, all new introduced commits will be scanned.
>
> "git-po-helper" will generate two kinds of suggestions, errors and
> warnings. A l10n contributor should try to fix all the errors, and
> should pay attention to the warnings. All the errors and warnings will
> be reported in the last step of the l10n workflow as two message groups.
> For a pull_request event, will create additional comments in pull
> request to report the result.

It is a good idea to automate this.

I am a bit concerned that the `ci-config` approach, even if we use it in
the Git project itself, is quite cumbersome to use, though. So I hope that
we can find an alternative solution.

One such solution could be to make the `git-po-helper` job contingent on
part of the repository name. For example:

  git-po-helper:
    if: endsWith(github.repository, '/git-po')
    [...]

would skip the job unless the target repository's name is `git-po`.

A couple more questions/suggestions are below:

> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  .github/workflows/l10n.yml | 143 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 143 insertions(+)
>  create mode 100644 .github/workflows/l10n.yml
>
> diff --git a/.github/workflows/l10n.yml b/.github/workflows/l10n.yml
> new file mode 100644
> index 0000000000..60f162c121
> --- /dev/null
> +++ b/.github/workflows/l10n.yml
> @@ -0,0 +1,143 @@
> +name: git-l10n
> +
> +on: [push, pull_request]
> +
> +jobs:
> +  ci-config:
> +    runs-on: ubuntu-latest
> +    outputs:
> +      enabled: ${{ steps.check-l10n.outputs.enabled }}
> +    steps:
> +      - name: try to clone ci-config branch
> +        run: |
> +          git -c protocol.version=2 clone \
> +            --no-tags \
> +            --single-branch \
> +            -b ci-config \
> +            --depth 1 \
> +            --no-checkout \
> +            --filter=blob:none \
> +            https://github.com/${{ github.repository }} \
> +            config-repo &&
> +          cd config-repo &&
> +          git checkout HEAD -- ci/config || : ignore
> +      - id: check-l10n
> +        name: check whether CI is enabled for l10n
> +        run: |
> +          enabled=no
> +          if test -x config-repo/ci/config/allow-l10n &&
> +             config-repo/ci/config/allow-l10n '${{ github.ref }}'
> +          then
> +            enabled=yes
> +          fi
> +          echo "::set-output name=enabled::$enabled"
> +
> +  git-po-helper:
> +    needs: ci-config
> +    if: needs.ci-config.outputs.enabled == 'yes'
> +    runs-on: ubuntu-latest
> +    steps:
> +    - uses: actions/checkout@v2
> +      with:
> +        fetch-depth: '0'

There is code below to specifically fetch the base commit, but setting the
`fetch-depth` to zero means to do a full clone anyway.

So maybe the `git fetch` code below should be dropped, or the
`fetch-depth` override?

Since we cannot know how deep we need to fetch, though, I figure that it
would be indeed better to have a non-shallow clone (and a code comment
would help clarify this).

An even better alternative would be a partial clone, of course, but I fear
that there is no convenient way yet to configure `actions/checkout` to do
so.

> +    - uses: actions/setup-go@v2
> +      with:
> +        go-version: ">=1.16"
> +    - name: Install git-po-helper
> +      run: |

Since this is a one-liner, it would probably make sense to avoid that `|`
continuation.

> +        go install github.com/git-l10n/git-po-helper@main
> +    - name: Install other dependencies
> +      run: |
> +        sudo apt-get update -q &&
> +        sudo apt-get install -q -y gettext
> +    - id: check-commits
> +      name: Run git-po-helper
> +      run: |
> +        if test "${{ github.event_name }}" = "pull_request"
> +        then
> +          commit_from=${{ github.event.pull_request.base.sha }}
> +          commit_to=${{ github.event.pull_request.head.sha }}
> +        else
> +          commit_from=${{ github.event.before }}
> +          commit_to=${{ github.event.after }}
> +          if ! echo $commit_from | grep -q "^00*$"

This would probably read better as:

		case "$commit_from" in
		*[^0]*|'') ;; # not all zeros
		*)
			[...]
			;;
		esac

But we might not need it anyway. See the comment on the `git fetch`
command below.

> +          then
> +            if ! git rev-parse "$commit_from^{commit}"
> +            then
> +              git fetch origin $commit_from

As pointed out above, we cannot know how many commits we need to fetch.
Therefore, I would suggest to simply drop this `git fetch`.

> +            fi
> +          fi
> +        fi
> +        exit_code=0
> +        git-po-helper check-commits --github-action -- \
> +          $commit_from..$commit_to >git-po-helper.out 2>&1 ||

What does the `--github-action` option do? Might be good to document this
here.

> +        exit_code=$?
> +        echo "::set-output name=exit_code::$exit_code"
> +        has_error_msg=
> +        has_warning_msg=
> +        if test $exit_code -ne 0
> +        then
> +          has_error_msg=yes

Do we really need this `has_error_msg` variable? It would be much easier
to test the condition `env.ERROR_MSG != ''` in subsequent steps, and that
would already do the job, without having to go through output variables.

> +          if test "${{ github.event_name }}" = "pull_request"
> +          then
> +            echo "ERROR_MSG<<EOF" >>$GITHUB_ENV
> +            grep -v -e "^level=warning" -e WARNING git-po-helper.out |
> +              perl -pe 's/\e\[[0-9;]*m//g' >>$GITHUB_ENV

This is a bit dangerous. First of all, how can you be sure that
`git-po-helper.out` does not contain lines that consist of the letters
`EOF` (and would therefore interfere with the here-doc)?

Second, isn't it conceivable to have an `error:` line which contains the
characters `WARNING` too? That line would be filtered out...

Third, can `git-po-helper` be convinced _not_ to print color codes? The
output was redirected into a file, after all, and it does not go to a
terminal...

> +            echo "EOF" >>$GITHUB_ENV
> +          fi
> +        fi
> +        if grep -q -e "^level=warning" -e WARNING git-po-helper.out
> +        then
> +          has_warning_msg=yes
> +          if test "${{ github.event_name }}" = "pull_request"
> +          then
> +            echo "WARNING_MSG<<EOF" >>$GITHUB_ENV
> +            grep -v -e "^level=error" -e ERROR git-po-helper.out |
> +              perl -pe 's/\e\[[0-9;]*m//g' >>$GITHUB_ENV
> +            echo "EOF" >>$GITHUB_ENV
> +          fi
> +        fi
> +        echo "::set-output name=has_error_msg::$has_error_msg"
> +        echo "::set-output name=has_warning_msg::$has_warning_msg"
> +    - name: Report errors in comment for pull request
> +      uses: mshick/add-pr-comment@v1
> +      if: steps.check-commits.outputs.has_error_msg == 'yes' && github.event_name == 'pull_request'
> +      continue-on-error: true
> +      with:
> +        repo-token: ${{ secrets.GITHUB_TOKEN }}

This requires the `GITHUB_TOKEN` to have write permissions, which I would
highly recommend not to require.

Instead, it would probably make more sense to keep the output in the logs
of the workflow run.

> +        repo-token-user-login: 'github-actions[bot]'
> +        message: |
> +          Errors found by git-po-helper in workflow ${{ github.workflow }}:
> +          ```
> +          ${{ env.ERROR_MSG }}
> +          ```
> +    - name: Report warnings in comment for pull request
> +      uses: mshick/add-pr-comment@v1
> +      if: steps.check-commits.outputs.has_warning_msg == 'yes' && github.event_name == 'pull_request'
> +      continue-on-error: true
> +      with:
> +        repo-token: ${{ secrets.GITHUB_TOKEN }}
> +        repo-token-user-login: 'github-actions[bot]'
> +        message: |
> +          Warnings found by git-po-helper in workflow ${{ github.workflow }}:
> +          ```
> +          ${{ env.WARNING_MSG }}
> +          ```
> +    - name: Report and quit
> +      run: |
> +        if test "${{ steps.check-commits.outputs.has_error_msg }}" = "yes"

This would be easier to read and maintain if it was upgraded to an `if:`
condition:

    - name: Report errors
      if: step.check-commits.outputs.has_error_msg = "yes"
      run: |
        [...]

And then do the same for warnings.

Also, it would be more natural if the `Run git-po-helper` step was allowed
to fail when `git-po-helper` outputs errors. You would then have to use
this conditional in the `Report errors` step:

      if: failure() && step.check-commits.outputs.has_error_msg = "yes"

(compare to how Git's `CI/PR` workflow prints errors:
https://github.com/git/git/blob/v2.33.0/.github/workflows/main.yml#L120).

For the `Report warnings` step, you would however have to use something
slightly less intuitive:

      if: (success() || failure()) && step.check-commits.outputs.has_warning_msg = "yes"

Finally, I _do_ think that this line would be easier to read like this,
while basically doing the same thing but with less effort (because the
outputs would no longer have to be set):

      if: (success() || failure()) && env.WARNING_MSG != ""

Ciao,
Dscho

> +        then
> +          echo "::group::Errors found by git-po-helper"
> +          grep -v -e "^level=warning" -e WARNING git-po-helper.out
> +          echo "::endgroup::"
> +        fi
> +        if test "${{ steps.check-commits.outputs.has_warning_msg }}" = "yes"
> +        then
> +          echo "::group::Warnings found by git-po-helper"
> +          grep -v -e "^level=error" -e ERROR git-po-helper.out
> +          echo "::endgroup::"
> +        fi
> +        if test ${{ steps.check-commits.outputs.exit_code }} -ne 0
> +        then
> +          exit ${{ steps.check-commits.outputs.exit_code }}
> +        fi
> --
> 2.33.0
>
>

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> For a push event, it will scan commits one by one. If a commit does not
>> look like a l10n commit (no file in "po/" has been changed), it will
>> immediately fail without checking for further commits. While for a
>> pull_request event, all new introduced commits will be scanned.
>>
>> "git-po-helper" will generate two kinds of suggestions, errors and
>> warnings. A l10n contributor should try to fix all the errors, and
>> should pay attention to the warnings. All the errors and warnings will
>> be reported in the last step of the l10n workflow as two message groups.
>> For a pull_request event, will create additional comments in pull
>> request to report the result.
>
> It is a good idea to automate this.
>
> I am a bit concerned that the `ci-config` approach, even if we use it in
> the Git project itself, is quite cumbersome to use, though. So I hope that
> we can find an alternative solution.
>
> One such solution could be to make the `git-po-helper` job contingent on
> part of the repository name. For example:
>
>   git-po-helper:
>     if: endsWith(github.repository, '/git-po')
>     [...]
>
> would skip the job unless the target repository's name is `git-po`.

Nice.

Can this be made into a matter purely local to git-l10n/git-po
repository and not git/git repository?  I am wondering if we can ee
if the current repository is git-l10n/git-po or its fork and run it
only if that is true.

Thanks.

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Johannes Schindelin]

Hi Junio,

On Mon, 23 Aug 2021, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> For a push event, it will scan commits one by one. If a commit does not
> >> look like a l10n commit (no file in "po/" has been changed), it will
> >> immediately fail without checking for further commits. While for a
> >> pull_request event, all new introduced commits will be scanned.
> >>
> >> "git-po-helper" will generate two kinds of suggestions, errors and
> >> warnings. A l10n contributor should try to fix all the errors, and
> >> should pay attention to the warnings. All the errors and warnings will
> >> be reported in the last step of the l10n workflow as two message groups.
> >> For a pull_request event, will create additional comments in pull
> >> request to report the result.
> >
> > It is a good idea to automate this.
> >
> > I am a bit concerned that the `ci-config` approach, even if we use it in
> > the Git project itself, is quite cumbersome to use, though. So I hope that
> > we can find an alternative solution.
> >
> > One such solution could be to make the `git-po-helper` job contingent on
> > part of the repository name. For example:
> >
> >   git-po-helper:
> >     if: endsWith(github.repository, '/git-po')
> >     [...]
> >
> > would skip the job unless the target repository's name is `git-po`.
>
> Nice.
>
> Can this be made into a matter purely local to git-l10n/git-po
> repository and not git/git repository? I am wondering if we can ee if
> the current repository is git-l10n/git-po or its fork and run it only if
> that is true.

The biggest problem is that there are forks of `git-l10n/git-po` that
accept PRs in their own right. That is what I tried to address by using
just the repository name, without the org/user prefix.

Ciao,
Dscho

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Can this be made into a matter purely local to git-l10n/git-po
>> repository and not git/git repository? I am wondering if we can ee if
>> the current repository is git-l10n/git-po or its fork and run it only if
>> that is true.
>
> The biggest problem is that there are forks of `git-l10n/git-po` that
> accept PRs in their own right. That is what I tried to address by using
> just the repository name, without the org/user prefix.

Well, it is why I wondered if we can see "if the repository is
git-l10n/git-po or its fork".  Perhaps I should have written "or
(recursively) its fork"?  I guess the repository name being git-po
may be a usable approximation ;-)

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Jean-Noël AVILA]

Le mardi 24 août 2021, 11:27:44 CEST Johannes Schindelin a écrit :
> Hi Junio,
> 
> On Mon, 23 Aug 2021, Junio C Hamano wrote:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> > >> For a push event, it will scan commits one by one. If a commit does not
> > >> look like a l10n commit (no file in "po/" has been changed), it will
> > >> immediately fail without checking for further commits. While for a
> > >> pull_request event, all new introduced commits will be scanned.
> > >>
> > >> "git-po-helper" will generate two kinds of suggestions, errors and
> > >> warnings. A l10n contributor should try to fix all the errors, and
> > >> should pay attention to the warnings. All the errors and warnings will
> > >> be reported in the last step of the l10n workflow as two message groups.
> > >> For a pull_request event, will create additional comments in pull
> > >> request to report the result.
> > >
> > > It is a good idea to automate this.
> > >
> > > I am a bit concerned that the `ci-config` approach, even if we use it in
> > > the Git project itself, is quite cumbersome to use, though. So I hope that
> > > we can find an alternative solution.
> > >
> > > One such solution could be to make the `git-po-helper` job contingent on
> > > part of the repository name. For example:
> > >
> > >   git-po-helper:
> > >     if: endsWith(github.repository, '/git-po')
> > >     [...]
> > >
> > > would skip the job unless the target repository's name is `git-po`.
> >
> > Nice.
> >
> > Can this be made into a matter purely local to git-l10n/git-po
> > repository and not git/git repository? I am wondering if we can ee if
> > the current repository is git-l10n/git-po or its fork and run it only if
> > that is true.
> 
> The biggest problem is that there are forks of `git-l10n/git-po` that
> accept PRs in their own right. That is what I tried to address by using
> just the repository name, without the org/user prefix.
> 
> Ciao,
> Dscho
> 

Well, I'm in this case, plus my repo is a fork of git/git. 

Would it not possible to formally require the presence of a "dumb" secret to run this rule? 

JN

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Jiang Xin]

On Tue, Aug 31, 2021 at 4:49 AM Jean-Noël AVILA <jn.avila@free.fr> wrote:
>
> Le mardi 24 août 2021, 11:27:44 CEST Johannes Schindelin a écrit :
> >
> > The biggest problem is that there are forks of `git-l10n/git-po` that
> > accept PRs in their own right. That is what I tried to address by using
> > just the repository name, without the org/user prefix.
> >
> > Ciao,
> > Dscho
> >
>
> Well, I'm in this case, plus my repo is a fork of git/git.
>
> Would it not possible to formally require the presence of a "dumb" secret to run this rule?
>

It is not supported to use secrets/env in if condition. See this post:

    https://github.community/t/jobs-job-id-if-does-not-work-with-env-secrets/16928

In patch v3, will check both repo name and branch names. See:

    https://lore.kernel.org/git/20210827071320.14183-1-worldhello.net@gmail.com/

--
Jiang Xin

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Jiang Xin]

On Tue, Aug 24, 2021 at 5:36 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> For a push event, it will scan commits one by one. If a commit does not
> >> look like a l10n commit (no file in "po/" has been changed), it will
> >> immediately fail without checking for further commits. While for a
> >> pull_request event, all new introduced commits will be scanned.
> >>
> >> "git-po-helper" will generate two kinds of suggestions, errors and
> >> warnings. A l10n contributor should try to fix all the errors, and
> >> should pay attention to the warnings. All the errors and warnings will
> >> be reported in the last step of the l10n workflow as two message groups.
> >> For a pull_request event, will create additional comments in pull
> >> request to report the result.
> >
> > It is a good idea to automate this.
> >
> > I am a bit concerned that the `ci-config` approach, even if we use it in
> > the Git project itself, is quite cumbersome to use, though. So I hope that
> > we can find an alternative solution.
> >
> > One such solution could be to make the `git-po-helper` job contingent on
> > part of the repository name. For example:
> >
> >   git-po-helper:
> >     if: endsWith(github.repository, '/git-po')
> >     [...]
> >
> > would skip the job unless the target repository's name is `git-po`.
>
> Nice.
>
> Can this be made into a matter purely local to git-l10n/git-po
> repository and not git/git repository?  I am wondering if we can ee
> if the current repository is git-l10n/git-po or its fork and run it
> only if that is true.

I have read almost all the github documents on github actions, and
tried to find if I can use a local branch, such as "github-action" to
hold local github-actions, but no locky.

That is to say, the workflow file must be introduced to the master
branch of “git-l10n/git-po”. As "git-l10n/git-po" is a fork of
"git/git", the new workflow should be part of "git/git", and provide a
way to disable it by default.

--
Jiang Xin

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

>> > One such solution could be to make the `git-po-helper` job contingent on
>> > part of the repository name. For example:
>> >
>> >   git-po-helper:
>> >     if: endsWith(github.repository, '/git-po')
>> >     [...]
>> >
>> > would skip the job unless the target repository's name is `git-po`.
>>
>> Nice.
>>
>> Can this be made into a matter purely local to git-l10n/git-po
>> repository and not git/git repository?  I am wondering if we can ee
>> if the current repository is git-l10n/git-po or its fork and run it
>> only if that is true.
>
> I have read almost all the github documents on github actions, and
> tried to find if I can use a local branch, such as "github-action" to
> hold local github-actions, but no locky.
>
> That is to say, the workflow file must be introduced to the master
> branch of “git-l10n/git-po”. As "git-l10n/git-po" is a fork of
> "git/git", the new workflow should be part of "git/git", and provide a
> way to disable it by default.

Yeah, that part I am agreeing with you.  I was just wondering if we
can do better than Dscho's heuristics (the repository name ending
with /git-po) to catch git-l10n/git-po or its fork to enable the
workflow in.

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Jiang Xin]

On Tue, Aug 24, 2021 at 5:03 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> It is a good idea to automate this.
>
> I am a bit concerned that the `ci-config` approach, even if we use it in
> the Git project itself, is quite cumbersome to use, though. So I hope that
> we can find an alternative solution.
>
> One such solution could be to make the `git-po-helper` job contingent on
> part of the repository name. For example:
>
>   git-po-helper:
>     if: endsWith(github.repository, '/git-po')
>     [...]
>
> would skip the job unless the target repository's name is `git-po`.

Yes, this is a solution I also want to try at the very beginning. But
some l10n team leaders may fork their repositories directly from
git/git, and name their repo as "{OWNER}/git".  I want to try another
solution: check existence of file "ci/config/allow-l10n" in branch
"ci-config" using a GitHub API, instead of cloning the ci-config
branch and execute the shell script "ci/config/allow-l10n".

> A couple more questions/suggestions are below:
>
> > +  git-po-helper:
> > +    needs: ci-config
> > +    if: needs.ci-config.outputs.enabled == 'yes'
> > +    runs-on: ubuntu-latest
> > +    steps:
> > +    - uses: actions/checkout@v2
> > +      with:
> > +        fetch-depth: '0'
>
> There is code below to specifically fetch the base commit, but setting the
> `fetch-depth` to zero means to do a full clone anyway.
>
> So maybe the `git fetch` code below should be dropped, or the
> `fetch-depth` override?
>
> Since we cannot know how deep we need to fetch, though, I figure that it
> would be indeed better to have a non-shallow clone (and a code comment
> would help clarify this).

If we want do a shallow clone, maybe we can try like this:

1.  Make 'git-po-helper' works with a bare repository, so the initial clone
    can be a shallow bare repository without checkout.
2.  Will get two revisions, base and head revision, when the workflow is
    triggered. (In patch v1, base and head revision are named as
    commit_from and commit_to)
3. Fetch the base revision and head revision using command:
    git fetch origin --depth=100 <base> <head>

We used a fixed depth 100, because we don't know how many commits
these two revisions are diverged.

Will feed the result of "git rev-list <base>..<head>" to
"git-po-helper", if depth 100 is not deep enough, rev-list will fail,
and should try to
run "git rev-list <head> -100".

I think the first version of l10n.yml should use a full clone, and try
to refactor later to use a shallow or partial clone.

> An even better alternative would be a partial clone, of course, but I fear
> that there is no convenient way yet to configure `actions/checkout` to do
> so.

Good idea.

> > +    - uses: actions/setup-go@v2
> > +      with:
> > +        go-version: ">=1.16"
> > +    - name: Install git-po-helper
> > +      run: |
>
> Since this is a one-liner, it would probably make sense to avoid that `|`
> continuation.

Will do.

> > +      run: |
> > +        if test "${{ github.event_name }}" = "pull_request"
> > +        then
> > +          commit_from=${{ github.event.pull_request.base.sha }}
> > +          commit_to=${{ github.event.pull_request.head.sha }}
> > +        else
> > +          commit_from=${{ github.event.before }}
> > +          commit_to=${{ github.event.after }}
> > +          if ! echo $commit_from | grep -q "^00*$"
>
> This would probably read better as:
>
>                 case "$commit_from" in
>                 *[^0]*|'') ;; # not all zeros
>                 *)

It's better than my version "echo .. | grep".

>                         [...]
>                         ;;
>                 esac
>
> But we might not need it anyway. See the comment on the `git fetch`
> command below.
>
> > +          then
> > +            if ! git rev-parse "$commit_from^{commit}"
> > +            then
> > +              git fetch origin $commit_from
>
> As pointed out above, we cannot know how many commits we need to fetch.
> Therefore, I would suggest to simply drop this `git fetch`.

v2 renamed $commit_from to $base, and renamed $commit_to to $head.

For a force push, the base commit is missing from the initial clone,
and the missing revision can be only accessed (fetched) using a full
SHA. For a "pull_request_target" event, the "head" revision is also
missing, because "pull_request_target" only the base commit is
checkout.

> > +            fi
> > +          fi
> > +        fi
> > +        exit_code=0
> > +        git-po-helper check-commits --github-action -- \
> > +          $commit_from..$commit_to >git-po-helper.out 2>&1 ||
>
> What does the `--github-action` option do? Might be good to document this
> here.

The option "--github-action" will enable "--no-gpg" and
"--no-gettext-back-compatible" options. To make the workflow
"l10n.yml" stable, I introduced the "--github-action" option. You can
see I introduced another option "--github-action-event=<push |
pull_request_event>", and the boolean option "--github-action" can be
omitted.

> > +        exit_code=$?
> > +        echo "::set-output name=exit_code::$exit_code"
> > +        has_error_msg=
> > +        has_warning_msg=
> > +        if test $exit_code -ne 0
> > +        then
> > +          has_error_msg=yes
>
> Do we really need this `has_error_msg` variable? It would be much easier
> to test the condition `env.ERROR_MSG != ''` in subsequent steps, and that
> would already do the job, without having to go through output variables.

"env.ERROR_MSG" is an environment variable which contains multiple
lines.  Shell script like `if test ${{ env.ERROR_MSG }} != ""` may
break.

> > +          if test "${{ github.event_name }}" = "pull_request"
> > +          then
> > +            echo "ERROR_MSG<<EOF" >>$GITHUB_ENV
> > +            grep -v -e "^level=warning" -e WARNING git-po-helper.out |
> > +              perl -pe 's/\e\[[0-9;]*m//g' >>$GITHUB_ENV
>
> This is a bit dangerous. First of all, how can you be sure that
> `git-po-helper.out` does not contain lines that consist of the letters
> `EOF` (and would therefore interfere with the here-doc)?

Will replace possible "EOF$" from the output file.

> Second, isn't it conceivable to have an `error:` line which contains the
> characters `WARNING` too? That line would be filtered out...

Will report errors and warnings all together.

> Third, can `git-po-helper` be convinced _not_ to print color codes? The
> output was redirected into a file, after all, and it does not go to a
> terminal...

"git-po-helper" uses the package "logrus" for logging. The default
format of "logrus” to write log to file is for machines, not user
friendly. The only way is provide an additional option "ForceColor" to
it. So I must clear the color code from the output file, if I want to
create a comment for pull request. But the ansi colors are nice to
display in the report of the action workflow.

> > +            echo "EOF" >>$GITHUB_ENV
> > +          fi
> > +        fi
> > +        if grep -q -e "^level=warning" -e WARNING git-po-helper.out
> > +        then
> > +          has_warning_msg=yes
> > +          if test "${{ github.event_name }}" = "pull_request"
> > +          then
> > +            echo "WARNING_MSG<<EOF" >>$GITHUB_ENV
> > +            grep -v -e "^level=error" -e ERROR git-po-helper.out |
> > +              perl -pe 's/\e\[[0-9;]*m//g' >>$GITHUB_ENV
> > +            echo "EOF" >>$GITHUB_ENV
> > +          fi
> > +        fi
> > +        echo "::set-output name=has_error_msg::$has_error_msg"
> > +        echo "::set-output name=has_warning_msg::$has_warning_msg"
> > +    - name: Report errors in comment for pull request
> > +      uses: mshick/add-pr-comment@v1
> > +      if: steps.check-commits.outputs.has_error_msg == 'yes' && github.event_name == 'pull_request'
> > +      continue-on-error: true
> > +      with:
> > +        repo-token: ${{ secrets.GITHUB_TOKEN }}
>
> This requires the `GITHUB_TOKEN` to have write permissions, which I would
> highly recommend not to require.

The action "mshick/add-pr-comment@v1" needs this token. See:
https://github.com/mshick/add-pr-comment .

> Instead, it would probably make more sense to keep the output in the logs
> of the workflow run.

You can see this nice post from ecco:
https://github.community/t/github-actions-are-severely-limited-on-prs/18179

For a successful git-l10n workflow, there are no errors, but may have
warnings for possible typos found in a ".po" file.  There may be lots
of false positives in these warnings.  If I hide these warnings in the
log of a workflow, git-l10n contributors may not notice them. So I
need a way to create comments in pull requests.

See some typical code reviews for git-l10n:

* https://github.com/git-l10n/git-po/pull/541
* https://github.com/git-l10n/git-po/pull/555

> > +        repo-token-user-login: 'github-actions[bot]'
> > +        message: |
> > +          Errors found by git-po-helper in workflow ${{ github.workflow }}:
> > +          ```
> > +          ${{ env.ERROR_MSG }}
> > +          ```
> > +    - name: Report warnings in comment for pull request
> > +      uses: mshick/add-pr-comment@v1
> > +      if: steps.check-commits.outputs.has_warning_msg == 'yes' && github.event_name == 'pull_request'
> > +      continue-on-error: true
> > +      with:
> > +        repo-token: ${{ secrets.GITHUB_TOKEN }}
> > +        repo-token-user-login: 'github-actions[bot]'
> > +        message: |
> > +          Warnings found by git-po-helper in workflow ${{ github.workflow }}:
> > +          ```
> > +          ${{ env.WARNING_MSG }}
> > +          ```
> > +    - name: Report and quit
> > +      run: |
> > +        if test "${{ steps.check-commits.outputs.has_error_msg }}" = "yes"
>
> This would be easier to read and maintain if it was upgraded to an `if:`
> condition:
>
>     - name: Report errors
>       if: step.check-commits.outputs.has_error_msg = "yes"
>       run: |
>         [...]
>
> And then do the same for warnings.

When users check the log of a workflow, they will only expand the
failed step.  So warnings and errors are reported in one step.

> Also, it would be more natural if the `Run git-po-helper` step was allowed
> to fail when `git-po-helper` outputs errors. You would then have to use
> this conditional in the `Report errors` step:
>
>       if: failure() && step.check-commits.outputs.has_error_msg = "yes"
>
> (compare to how Git's `CI/PR` workflow prints errors:
> https://github.com/git/git/blob/v2.33.0/.github/workflows/main.yml#L120).
>
> For the `Report warnings` step, you would however have to use something
> slightly less intuitive:
>
>       if: (success() || failure()) && step.check-commits.outputs.has_warning_msg = "yes"

Will try.

> Finally, I _do_ think that this line would be easier to read like this,
> while basically doing the same thing but with less effort (because the
> outputs would no longer have to be set):
>
>       if: (success() || failure()) && env.WARNING_MSG != ""
>
> Ciao,
> Dscho

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 14471 bytes --]

Hi Xin,

On Tue, 24 Aug 2021, Jiang Xin wrote:

> On Tue, Aug 24, 2021 at 5:03 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > It is a good idea to automate this.
> >
> > I am a bit concerned that the `ci-config` approach, even if we use it in
> > the Git project itself, is quite cumbersome to use, though. So I hope that
> > we can find an alternative solution.
> >
> > One such solution could be to make the `git-po-helper` job contingent on
> > part of the repository name. For example:
> >
> >   git-po-helper:
> >     if: endsWith(github.repository, '/git-po')
> >     [...]
> >
> > would skip the job unless the target repository's name is `git-po`.
>
> Yes, this is a solution I also want to try at the very beginning. But
> some l10n team leaders may fork their repositories directly from
> git/git, and name their repo as "{OWNER}/git".  I want to try another
> solution: check existence of file "ci/config/allow-l10n" in branch
> "ci-config" using a GitHub API, instead of cloning the ci-config
> branch and execute the shell script "ci/config/allow-l10n".

I understood that you were trying to imitate what git/git does.

The problem, in git/git as well as in your patch, is that this is highly
cumbersome to use. Yes, it would be better if there was an easier UI to do
what you want to do, but the current reality is: there isn't.

The main reason why it is so cumbersome to use is that your chosen
strategy scatters the CI configuration so much that it puts a mental
burden on the users. I, for one, have no idea what is currently in my
`ci-config` branch. And new users will be forced to struggle to set up
their fork in such a way that the configuration does what they want it to
do.

And in this instance, there is not even any good reason to make it hard on
the users because most will likely not need that new workflow at all. That
workflow is primarily interesting for the l10n maintainers.

To make it easier on the vast majority of contributors, I therefore
suggested to introduce an easy-to-parse condition that lives not only in
the same branch but in the very same file as the workflow _and_ it does
what most users need it to do: cognitive burden penalty averted!

Even better: the condition I suggested can be _easily_ extended by the few
l10n maintainers to include their particular for as target repository.

> > A couple more questions/suggestions are below:
> >
> > > +  git-po-helper:
> > > +    needs: ci-config
> > > +    if: needs.ci-config.outputs.enabled == 'yes'
> > > +    runs-on: ubuntu-latest
> > > +    steps:
> > > +    - uses: actions/checkout@v2
> > > +      with:
> > > +        fetch-depth: '0'
> >
> > There is code below to specifically fetch the base commit, but setting the
> > `fetch-depth` to zero means to do a full clone anyway.
> >
> > So maybe the `git fetch` code below should be dropped, or the
> > `fetch-depth` override?
> >
> > Since we cannot know how deep we need to fetch, though, I figure that it
> > would be indeed better to have a non-shallow clone (and a code comment
> > would help clarify this).
>
> If we want do a shallow clone, maybe we can try like this:
>
> 1.  Make 'git-po-helper' works with a bare repository, so the initial clone
>     can be a shallow bare repository without checkout.
> 2.  Will get two revisions, base and head revision, when the workflow is
>     triggered. (In patch v1, base and head revision are named as
>     commit_from and commit_to)
> 3. Fetch the base revision and head revision using command:
>     git fetch origin --depth=100 <base> <head>
>
> We used a fixed depth 100, because we don't know how many commits
> these two revisions are diverged.

Okay, but you did not use a fixed depth 100: you used depth 0, which turns
off shallow clones in actions/checkout.

> Will feed the result of "git rev-list <base>..<head>" to
> "git-po-helper", if depth 100 is not deep enough, rev-list will fail,
> and should try to
> run "git rev-list <head> -100".
>
> I think the first version of l10n.yml should use a full clone, and try
> to refactor later to use a shallow or partial clone.

Given how often the workflow will run, I am not sure that it is worth the
effort to micro-optimize this. Just use a full clone and you're done.

> >                         [...]
> >                         ;;
> >                 esac
> >
> > But we might not need it anyway. See the comment on the `git fetch`
> > command below.
> >
> > > +          then
> > > +            if ! git rev-parse "$commit_from^{commit}"
> > > +            then
> > > +              git fetch origin $commit_from
> >
> > As pointed out above, we cannot know how many commits we need to fetch.
> > Therefore, I would suggest to simply drop this `git fetch`.
>
> v2 renamed $commit_from to $base, and renamed $commit_to to $head.
>
> For a force push, the base commit is missing from the initial clone,

Ah, that's the point I was missing.

> and the missing revision can be only accessed (fetched) using a full
> SHA. For a "pull_request_target" event, the "head" revision is also
> missing, because "pull_request_target" only the base commit is
> checkout.

If you truly want to optimize the fetch (which I don't think is worth the
effort, as I mentioned above), you could also start with a shallow clone,
then call

	git config remote.origin.promisor true
	git config remote.origin.partialCloneFilter blob:none
	rm .git/shallow

Subsequent Git operations should transparently fetch whatever is missing.

But again, this strikes me as seriously premature optimization.

> > > +            fi
> > > +          fi
> > > +        fi
> > > +        exit_code=0
> > > +        git-po-helper check-commits --github-action -- \
> > > +          $commit_from..$commit_to >git-po-helper.out 2>&1 ||
> >
> > What does the `--github-action` option do? Might be good to document this
> > here.
>
> The option "--github-action" will enable "--no-gpg" and
> "--no-gettext-back-compatible" options. To make the workflow
> "l10n.yml" stable, I introduced the "--github-action" option. You can
> see I introduced another option "--github-action-event=<push |
> pull_request_event>", and the boolean option "--github-action" can be
> omitted.

Okay, I take your word for it.

> > > +        exit_code=$?
> > > +        echo "::set-output name=exit_code::$exit_code"
> > > +        has_error_msg=
> > > +        has_warning_msg=
> > > +        if test $exit_code -ne 0
> > > +        then
> > > +          has_error_msg=yes
> >
> > Do we really need this `has_error_msg` variable? It would be much easier
> > to test the condition `env.ERROR_MSG != ''` in subsequent steps, and that
> > would already do the job, without having to go through output variables.
>
> "env.ERROR_MSG" is an environment variable which contains multiple
> lines.  Shell script like `if test ${{ env.ERROR_MSG }} != ""` may
> break.

They won't, if you use `if test "$ERROR_MSG" != ""`, which is more
canonical anyway.

> > Third, can `git-po-helper` be convinced _not_ to print color codes? The
> > output was redirected into a file, after all, and it does not go to a
> > terminal...
>
> "git-po-helper" uses the package "logrus" for logging. The default
> format of "logrus” to write log to file is for machines, not user
> friendly. The only way is provide an additional option "ForceColor" to
> it. So I must clear the color code from the output file, if I want to
> create a comment for pull request. But the ansi colors are nice to
> display in the report of the action workflow.

Ah. That makes sense. Maybe it would be useful information for the commit
message?

> > > +            echo "EOF" >>$GITHUB_ENV
> > > +          fi
> > > +        fi
> > > +        if grep -q -e "^level=warning" -e WARNING git-po-helper.out
> > > +        then
> > > +          has_warning_msg=yes
> > > +          if test "${{ github.event_name }}" = "pull_request"
> > > +          then
> > > +            echo "WARNING_MSG<<EOF" >>$GITHUB_ENV
> > > +            grep -v -e "^level=error" -e ERROR git-po-helper.out |
> > > +              perl -pe 's/\e\[[0-9;]*m//g' >>$GITHUB_ENV
> > > +            echo "EOF" >>$GITHUB_ENV
> > > +          fi
> > > +        fi
> > > +        echo "::set-output name=has_error_msg::$has_error_msg"
> > > +        echo "::set-output name=has_warning_msg::$has_warning_msg"
> > > +    - name: Report errors in comment for pull request
> > > +      uses: mshick/add-pr-comment@v1
> > > +      if: steps.check-commits.outputs.has_error_msg == 'yes' && github.event_name == 'pull_request'
> > > +      continue-on-error: true
> > > +      with:
> > > +        repo-token: ${{ secrets.GITHUB_TOKEN }}
> >
> > This requires the `GITHUB_TOKEN` to have write permissions, which I would
> > highly recommend not to require.
>
> The action "mshick/add-pr-comment@v1" needs this token. See:
> https://github.com/mshick/add-pr-comment .

Yes, I am fully aware.

What I tried to point out is that the `GITHUB_TOKEN` you receive _may_
have write permissions (it used to be the default), but these days you can
configure it to be read-only, as a repository admin. For details, see
https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/#setting-the-default-permissions-for-the-organization-or-repository

If GITHUB_TOKEN is configured to be read-only (which I, for one, do with
all repositories I can, for security reasons), then `add-pr-comment` might
fail.

This was the case for the `check-whitespace` workflow on
git-for-windows/git, which is why I fixed that workflow in cc00362125c
(ci(check-whitespace): stop requiring a read/write token, 2021-07-14).

It would not make sense to re-introduce the same issue in a new workflow.

> > Instead, it would probably make more sense to keep the output in the logs
> > of the workflow run.
>
> You can see this nice post from ecco:
> https://github.community/t/github-actions-are-severely-limited-on-prs/18179

Oh, I am aware of the problem. Probably a lot more than you think I am.
After all, I introduced the Azure Pipeline definition which offered not
only a very convenient way to get to the logs of failed test cases, but
also had statistics on flaky tests, and allowed monitoring all kinds of
insights.

And GitHub workflows have none of that.

At least at the moment. If you want to investigate a test failure, you
have to open the failed run, but that won't get you to the right spot yet.
Instead, it opens the log of the `prove` run, which only tells you which
test script(s) failed.

What you _then_ have to do is to expand the `ci/print-test-failures.sh`
step (which _succeeded_ and hence it is not expanded by default) and then
you have to click on the magnifying glass symbol (i.e. _not_ use your
browser's "Find" functionality, that won't work) and search for "not ok"
and then skim over all `# known breakage` entries.

So yes, I do know about the (current) limitations of the GitHub workflows
UI ;-)

> For a successful git-l10n workflow, there are no errors, but may have
> warnings for possible typos found in a ".po" file.  There may be lots
> of false positives in these warnings.  If I hide these warnings in the
> log of a workflow, git-l10n contributors may not notice them. So I
> need a way to create comments in pull requests.

Or the workflow runs need to fail, and PRs need to require those runs to
pass.

> See some typical code reviews for git-l10n:
>
> * https://github.com/git-l10n/git-po/pull/541
> * https://github.com/git-l10n/git-po/pull/555

Thank you for linking those! Now that you said it, I thought about a
different strategy we're using in the Git for Windows project (where we
have a scheduled workflow that monitors a few of the 150 components
bundled in Git for Windows' installer, to get notified if there are new
versions, and the workflow needs write permission in order to open new
tickets). We use an environment secret (and environments can be limited
appropriately, e.g. by requiring approvals from a specific team). For
details, see
https://github.com/git-for-windows/git/commit/1abb4477f462c3d155ec68d40c961a5e0604ddf8

I could imagine that you could make your workflow contingent on the
presence of, say, `GIT_PO_HELPER_GITHUB_TOKEN`. That would not only solve
the "read-only token" problem but also the "don't run everywhere, please!"
problem. You (and other l10n maintainers) only have to create a Personal
Access Token with `repo` permission and add it as a secret.

But ideally, you would test whether an environment of a given name exists,
and I am not aware if such a thing is possible yet with GitHub workflows.

Food for thought?

> > > +        repo-token-user-login: 'github-actions[bot]'
> > > +        message: |
> > > +          Errors found by git-po-helper in workflow ${{ github.workflow }}:
> > > +          ```
> > > +          ${{ env.ERROR_MSG }}
> > > +          ```
> > > +    - name: Report warnings in comment for pull request
> > > +      uses: mshick/add-pr-comment@v1
> > > +      if: steps.check-commits.outputs.has_warning_msg == 'yes' && github.event_name == 'pull_request'
> > > +      continue-on-error: true
> > > +      with:
> > > +        repo-token: ${{ secrets.GITHUB_TOKEN }}
> > > +        repo-token-user-login: 'github-actions[bot]'
> > > +        message: |
> > > +          Warnings found by git-po-helper in workflow ${{ github.workflow }}:
> > > +          ```
> > > +          ${{ env.WARNING_MSG }}
> > > +          ```
> > > +    - name: Report and quit
> > > +      run: |
> > > +        if test "${{ steps.check-commits.outputs.has_error_msg }}" = "yes"
> >
> > This would be easier to read and maintain if it was upgraded to an `if:`
> > condition:
> >
> >     - name: Report errors
> >       if: step.check-commits.outputs.has_error_msg = "yes"
> >       run: |
> >         [...]
> >
> > And then do the same for warnings.
>
> When users check the log of a workflow, they will only expand the
> failed step.  So warnings and errors are reported in one step.

Right. You need to make the step fail that shows the errors/warnings. (In
Git's `main.yml`, we don't...)

Ciao,
Dscho

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Jiang Xin]

Hi Dscho,

On Wed, Aug 25, 2021 at 8:14 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> > Yes, this is a solution I also want to try at the very beginning. But
> > some l10n team leaders may fork their repositories directly from
> > git/git, and name their repo as "{OWNER}/git".  I want to try another
> > solution: check existence of file "ci/config/allow-l10n" in branch
> > "ci-config" using a GitHub API, instead of cloning the ci-config
> > branch and execute the shell script "ci/config/allow-l10n".
>
> I understood that you were trying to imitate what git/git does.
>
> The problem, in git/git as well as in your patch, is that this is highly
> cumbersome to use. Yes, it would be better if there was an easier UI to do
> what you want to do, but the current reality is: there isn't.
>
> The main reason why it is so cumbersome to use is that your chosen
> strategy scatters the CI configuration so much that it puts a mental
> burden on the users. I, for one, have no idea what is currently in my
> `ci-config` branch. And new users will be forced to struggle to set up
> their fork in such a way that the configuration does what they want it to
> do.
>
> And in this instance, there is not even any good reason to make it hard on
> the users because most will likely not need that new workflow at all. That
> workflow is primarily interesting for the l10n maintainers.
>
> To make it easier on the vast majority of contributors, I therefore
> suggested to introduce an easy-to-parse condition that lives not only in
> the same branch but in the very same file as the workflow _and_ it does
> what most users need it to do: cognitive burden penalty averted!
>
> Even better: the condition I suggested can be _easily_ extended by the few
> l10n maintainers to include their particular for as target repository.
>

I agree with you that the "ci-config" job is cumbersome to use, and I
will follow your suggestions. I may also want to try to implement a
github action later to check the existence of a file in a branch using
GitHub API as an supplementary way.

> > > A couple more questions/suggestions are below:
> > >
> > > > +  git-po-helper:
> > > > +    needs: ci-config
> > > > +    if: needs.ci-config.outputs.enabled == 'yes'
> > > > +    runs-on: ubuntu-latest
> > > > +    steps:
> > > > +    - uses: actions/checkout@v2
> > > > +      with:
> > > > +        fetch-depth: '0'
> > >
> > > There is code below to specifically fetch the base commit, but setting the
> > > `fetch-depth` to zero means to do a full clone anyway.
> > >
> > > So maybe the `git fetch` code below should be dropped, or the
> > > `fetch-depth` override?
> > >
> > > Since we cannot know how deep we need to fetch, though, I figure that it
> > > would be indeed better to have a non-shallow clone (and a code comment
> > > would help clarify this).
> >
> > If we want do a shallow clone, maybe we can try like this:
> >
> > 1.  Make 'git-po-helper' works with a bare repository, so the initial clone
> >     can be a shallow bare repository without checkout.
> > 2.  Will get two revisions, base and head revision, when the workflow is
> >     triggered. (In patch v1, base and head revision are named as
> >     commit_from and commit_to)
> > 3. Fetch the base revision and head revision using command:
> >     git fetch origin --depth=100 <base> <head>
> >
> > We used a fixed depth 100, because we don't know how many commits
> > these two revisions are diverged.

Wrong tense.  My English! :sweat:
s/We used a fixed depth 100/I plan to use a fixed depth of 100/

>
> Okay, but you did not use a fixed depth 100: you used depth 0, which turns
> off shallow clones in actions/checkout.
>

>
> If you truly want to optimize the fetch (which I don't think is worth the
> effort, as I mentioned above), you could also start with a shallow clone,
> then call
>
>         git config remote.origin.promisor true
>         git config remote.origin.partialCloneFilter blob:none
>         rm .git/shallow
>
> Subsequent Git operations should transparently fetch whatever is missing.
>
> But again, this strikes me as seriously premature optimization.

I like this. Will try.

And I wonder it would be better if the action "actions/checkout" can
support "fetch-depth: -1". Which means it does not do fetch at all,
but only sets the correct ssh_key and auth token.

> > The action "mshick/add-pr-comment@v1" needs this token. See:
> > https://github.com/mshick/add-pr-comment .
>
> Yes, I am fully aware.
>
> What I tried to point out is that the `GITHUB_TOKEN` you receive _may_
> have write permissions (it used to be the default), but these days you can
> configure it to be read-only, as a repository admin. For details, see
> https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/#setting-the-default-permissions-for-the-organization-or-repository
>
> If GITHUB_TOKEN is configured to be read-only (which I, for one, do with
> all repositories I can, for security reasons), then `add-pr-comment` might
> fail.
>
> This was the case for the `check-whitespace` workflow on
> git-for-windows/git, which is why I fixed that workflow in cc00362125c
> (ci(check-whitespace): stop requiring a read/write token, 2021-07-14).
>
> It would not make sense to re-introduce the same issue in a new workflow.

I understand your concern.  I will setup read only permission for
github action for the repo.

> > > Instead, it would probably make more sense to keep the output in the logs
> > > of the workflow run.
> >
> > You can see this nice post from ecco:
> > https://github.community/t/github-actions-are-severely-limited-on-prs/18179
>
> Oh, I am aware of the problem. Probably a lot more than you think I am.
> After all, I introduced the Azure Pipeline definition which offered not
> only a very convenient way to get to the logs of failed test cases, but
> also had statistics on flaky tests, and allowed monitoring all kinds of
> insights.
>
> And GitHub workflows have none of that.
>
> At least at the moment. If you want to investigate a test failure, you
> have to open the failed run, but that won't get you to the right spot yet.
> Instead, it opens the log of the `prove` run, which only tells you which
> test script(s) failed.
>
> What you _then_ have to do is to expand the `ci/print-test-failures.sh`
> step (which _succeeded_ and hence it is not expanded by default) and then
> you have to click on the magnifying glass symbol (i.e. _not_ use your
> browser's "Find" functionality, that won't work) and search for "not ok"
> and then skim over all `# known breakage` entries.
>
> So yes, I do know about the (current) limitations of the GitHub workflows
> UI ;-)
>
> > For a successful git-l10n workflow, there are no errors, but may have
> > warnings for possible typos found in a ".po" file.  There may be lots
> > of false positives in these warnings.  If I hide these warnings in the
> > log of a workflow, git-l10n contributors may not notice them. So I
> > need a way to create comments in pull requests.
>
> Or the workflow runs need to fail, and PRs need to require those runs to
> pass.
>
> > See some typical code reviews for git-l10n:
> >
> > * https://github.com/git-l10n/git-po/pull/541
> > * https://github.com/git-l10n/git-po/pull/555
>
> Thank you for linking those! Now that you said it, I thought about a
> different strategy we're using in the Git for Windows project (where we
> have a scheduled workflow that monitors a few of the 150 components
> bundled in Git for Windows' installer, to get notified if there are new
> versions, and the workflow needs write permission in order to open new
> tickets). We use an environment secret (and environments can be limited
> appropriately, e.g. by requiring approvals from a specific team). For
> details, see
> https://github.com/git-for-windows/git/commit/1abb4477f462c3d155ec68d40c961a5e0604ddf8
>
> I could imagine that you could make your workflow contingent on the
> presence of, say, `GIT_PO_HELPER_GITHUB_TOKEN`. That would not only solve
> the "read-only token" problem but also the "don't run everywhere, please!"
> problem. You (and other l10n maintainers) only have to create a Personal
> Access Token with `repo` permission and add it as a secret.
>
> But ideally, you would test whether an environment of a given name exists,
> and I am not aware if such a thing is possible yet with GitHub workflows.
>
> Food for thought?

To make it simple, I want to limit the permissions of the token in
"l10n.yml' and use the default `GITHUB_TOKEN`. like this:

          git-po-helper:
            needs: l10n-config
            if: needs.l10n-config.outputs.enabled == 'yes'
            runs-on: ubuntu-latest
    +       permissions:
    +          pull-requests: write
            steps:

It can work even if the administrator turns off
'read-write-permission' for action of the repository.

--
Regards,
Jiang Xin

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Jeff King]

On Wed, Aug 25, 2021 at 02:14:43PM +0200, Johannes Schindelin wrote:

> > Yes, this is a solution I also want to try at the very beginning. But
> > some l10n team leaders may fork their repositories directly from
> > git/git, and name their repo as "{OWNER}/git".  I want to try another
> > solution: check existence of file "ci/config/allow-l10n" in branch
> > "ci-config" using a GitHub API, instead of cloning the ci-config
> > branch and execute the shell script "ci/config/allow-l10n".
> 
> I understood that you were trying to imitate what git/git does.
> 
> The problem, in git/git as well as in your patch, is that this is highly
> cumbersome to use. Yes, it would be better if there was an easier UI to do
> what you want to do, but the current reality is: there isn't.
> 
> The main reason why it is so cumbersome to use is that your chosen
> strategy scatters the CI configuration so much that it puts a mental
> burden on the users. I, for one, have no idea what is currently in my
> `ci-config` branch. And new users will be forced to struggle to set up
> their fork in such a way that the configuration does what they want it to
> do.
> 
> And in this instance, there is not even any good reason to make it hard on
> the users because most will likely not need that new workflow at all. That
> workflow is primarily interesting for the l10n maintainers.

Just adding my two cents as the person who created the "ci-config"
mechanism: yes, it's absolutely horrible, and should be avoided if at
all possible. :)

Your repo-name solution seems like a quite reasonable alternative.

(I'd still love for there to be a way to selectively disable CI on
certain branches, but I didn't find another one, short of enforcing
branch-naming conventions that the whole project must agree on).

-Peff

Re: [PATCH 1/1] ci: new github-action for git-l10n code review

[Jiang Xin]

On Fri, Aug 27, 2021 at 10:10 AM Jeff King <peff@peff.net> wrote:
>
> On Wed, Aug 25, 2021 at 02:14:43PM +0200, Johannes Schindelin wrote:
>
> > > Yes, this is a solution I also want to try at the very beginning. But
> > > some l10n team leaders may fork their repositories directly from
> > > git/git, and name their repo as "{OWNER}/git".  I want to try another
> > > solution: check existence of file "ci/config/allow-l10n" in branch
> > > "ci-config" using a GitHub API, instead of cloning the ci-config
> > > branch and execute the shell script "ci/config/allow-l10n".
> >
> > I understood that you were trying to imitate what git/git does.
> >
> > The problem, in git/git as well as in your patch, is that this is highly
> > cumbersome to use. Yes, it would be better if there was an easier UI to do
> > what you want to do, but the current reality is: there isn't.
> >
> > The main reason why it is so cumbersome to use is that your chosen
> > strategy scatters the CI configuration so much that it puts a mental
> > burden on the users. I, for one, have no idea what is currently in my
> > `ci-config` branch. And new users will be forced to struggle to set up
> > their fork in such a way that the configuration does what they want it to
> > do.
> >
> > And in this instance, there is not even any good reason to make it hard on
> > the users because most will likely not need that new workflow at all. That
> > workflow is primarily interesting for the l10n maintainers.
>
> Just adding my two cents as the person who created the "ci-config"
> mechanism: yes, it's absolutely horrible, and should be avoided if at
> all possible. :)
>
> Your repo-name solution seems like a quite reasonable alternative.
>
> (I'd still love for there to be a way to selectively disable CI on
> certain branches, but I didn't find another one, short of enforcing
> branch-naming conventions that the whole project must agree on).

Yesterday, I created a new github-action:

    https://github.com/marketplace/actions/file-exists-action

And want to use this action in the "ci-config (l10n-config)" job like
this, but it appears slower than before:

-- snip begins --
jobs:
  l10n-config:
    runs-on: ubuntu-latest
    outputs:
      enabled: ${{ steps.check-repo-name.outputs.matched == 'yes' ||
steps.check-file-exists.outputs.exists }}
    steps:
      - id: check-repo-name
        ... ...
      - id: check-file-exists
        name: Check file in branch
        if: steps.check-repo-name.outputs.matched != 'yes'
        uses: jiangxin/file-exists-action@v1
        with:
          ref: ci-config
          path: ci/config/allow-l10n
-- snip ends --

The execution of the "l10n-config" job will take 4 seconds vs 2
seconds before. This may because the new action
"jiangxin/file-exists-action" loads another VM to execute.

So in patch v3, I will remove the entirely "ci-config (l10n-config)"
job, and use an if condition like this:

-- snip begins --
name: git-l10n
on: [push, pull_request_target]
jobs:
  git-po-helper:
    if: endsWith(github.repository, '/git-po') ||
contains(github.head_ref, 'l10n') || contains(github.ref, 'l10n')
    runs-on: ubuntu-latest
    permissions:
      pull-requests: write
    steps:
      ... ...
--snip ends --

Will check the target repository name first.  If l10n leader has a
fork repository with different repo name, push to a special branch
name, such as "l10n/git-2.34", will also trigger the git-l10n
workflow.

--
Jiang Xin

[PATCH v3] ci: new github-action for git-l10n code review

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Repository of git-l10n is a fork from "git/git" on GitHub, and uses
GitHub pull request for code review. A helper program "git-po-helper"
can be used to check typos in ".po" files, validate syntax, and check
commit messages. It would be convenient to integrate this helper program
to CI and add comments in pull request.

The new github-action workflow is added in ".github/workflows/". It will
only be enabled for the following cases:

 * A repository named as "git-po", such as a repository forked from
   "git-l10n/git-po".

 * Push to the branch that contains "l10n" in the name.

 * Pull reqeust from a remote branch which has "l10n" in the name, such
   as: "l10n/fix-fuzzy-translations".

The new l10n workflow listens to two types of github events:

    on: [push, pull_request_target]

The reason we use "pull_request_target" instead of "pull_request" is
that for security reasons, pull requests from forks receive a read-only
GITHUB_TOKEN and workflows cannot write comments back to pull requests.
GitHub provides a "pull_request_target" event to resolve security risks
by checking out the base commit from the target repository, and provide
write permissions for the workflow.

By default, adminstrators can set strict permissions for workflows. The
following code is used to modify the permissions for the GITHUB_TOKEN
and give write permission in order to create comments in pull-requests.

    permissions:
      pull-requests: write

This workflow will scan commits one by one. If a commit does not look
like a l10n commit (no file in "po/" has been changed), the scan process
will stop immediately. For a "push" event, no error will be reported
because it is normal to push non-l10n commits merged from upstream. But
for the "pull_request_target" event, errors will be reported.
For this reason, additional option is provided for "git-po-helper".

    git-po-helper check-commits \
        --github-action-event="${{ github.event_name }}" -- \
        <base>..<head>

The output messages of "git-po-helper" contain color codes not only for
console, but also for logfile. This is because "git-po-helper" uses a
package named "logrus" for logging, and I use an additional option
"ForceColor" to initialize "logrus" to print messages in a user-friendly
format in logfile output. These color codes help produce beautiful
output for logs of workflow, but they must be stripped off when creating
comments for pull requests. E.g.:

    perl -pe 's/\e\[[0-9;]*m//g' git-po-helper.out

"git-po-helper" may generate two kinds of suggestions, errors and
warnings. All the errors and warnings will be reported in the log
the l10n workflow. For a "pull_request_target" event, an additional
comment will be created in the pull request to report the result.
A l10n contributor should try to fix all the errors, and should pay
attention to the warnings.

Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 .github/workflows/l10n.yml | 91 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 .github/workflows/l10n.yml

diff --git a/.github/workflows/l10n.yml b/.github/workflows/l10n.yml
new file mode 100644
index 0000000..4129fa3
--- /dev/null
+++ b/.github/workflows/l10n.yml
@@ -0,0 +1,91 @@
+name: git-l10n
+
+on: [push, pull_request_target]
+
+jobs:
+  git-po-helper:
+    if: endsWith(github.repository, '/git-po') || contains(github.head_ref, 'l10n') || contains(github.ref, 'l10n')
+    runs-on: ubuntu-latest
+    permissions:
+      pull-requests: write
+    steps:
+      - uses: actions/checkout@v2
+        with:
+          fetch-depth: '1'
+      - name: Fetch missing commits
+        id: fetch-commits
+        run: |
+          # Setup partial clone.
+          git config remote.origin.promisor true
+          git config remote.origin.partialCloneFilter blob:none
+          if test "${{ github.event_name }}" = "pull_request_target"
+          then
+            base=${{ github.event.pull_request.base.sha }}
+            head=${{ github.event.pull_request.head.sha }}
+          else
+            base=${{ github.event.before }}
+            head=${{ github.event.after }}
+          fi
+          args=
+          for commit in $base $head
+          do
+            case $commit in
+            *[^0]*)
+              args="$args $commit"
+              ;;
+            *)
+              # Ignore ZERO-OID.
+              ;;
+            esac
+          done
+          # Unshallow the repository, and fetch missing commits.
+          # "$base" may be missing due to forced push, and "$head"
+          # may be missing due to the "pull_request_target" event.
+          git fetch --unshallow origin $args
+          echo "::set-output name=base::$base"
+          echo "::set-output name=head::$head"
+      - uses: actions/setup-go@v2
+        with:
+          go-version: '>=1.16'
+      - name: Install git-po-helper
+        run: go install github.com/git-l10n/git-po-helper@main
+      - name: Install other dependencies
+        run: |
+          sudo apt-get update -q &&
+          sudo apt-get install -q -y gettext
+      - name: Run git-po-helper
+        id: check-commits
+        run: |
+          exit_code=0
+          create_comment=
+          git-po-helper check-commits \
+              --github-action-event="${{ github.event_name }}" -- \
+              ${{ steps.fetch-commits.outputs.base }}..${{ steps.fetch-commits.outputs.head }} \
+              >git-po-helper.out 2>&1 ||
+            exit_code=$?
+          if test $exit_code -ne 0 ||
+            grep -q -e "^level=warning" -e WARNING git-po-helper.out
+          then
+            create_comment=yes
+            # Remove ANSI colors which are proper for console logs but not
+            # proper for PR comment.
+            echo "COMMENT_BODY<<EOF" >>$GITHUB_ENV
+            perl -pe 's/\e\[[0-9;]*m//g; s/\bEOF$//g' git-po-helper.out >>$GITHUB_ENV
+            echo "EOF" >>$GITHUB_ENV
+          fi
+          echo "::set-output name=create_comment::$create_comment"
+          cat git-po-helper.out
+          exit $exit_code
+      - name: Report in comment for pull request
+        uses: mshick/add-pr-comment@v1
+        if: always() && steps.check-commits.outputs.create_comment== 'yes' && github.event_name == 'pull_request_target'
+        continue-on-error: true
+        with:
+          repo-token: ${{ secrets.GITHUB_TOKEN }}
+          repo-token-user-login: 'github-actions[bot]'
+          message: |
+            ${{ steps.check-commits.outcome == 'failure' && 'Errors and warnings' || 'Warnings' }} found by [git-po-helper](https://github.com/git-l10n/git-po-helper#readme) in workflow [#${{ github.run_number }}](${{ env.GITHUB_SERVER_URL }}/${{ github.repository }}/actions/runs/${{ github.run_id }}):
+
+            ```
+            ${{ env.COMMENT_BODY }}
+            ```
-- 
2.33.0

[PATCH v4 0/1] ci: new github-action for git-l10n code review

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

## Changes since v3

* Update commit message.

* Wrap the long if conditions to make them easier to read. See:
  https://yaml-multiline.info/

* Create partial clone using one command by adding "--filter=blob:none"
  option to "git fetch".

* In the if condition of the last step, use `env.COMMENT_BODY != ''`,
  and do not need "steps.check-commits.outputs.create_comment".


## range-diff v3...v4

1:  06a6c1c ! 1:  0067ccc ci: new github-action for git-l10n code review
    @@ Metadata
      ## Commit message ##
         ci: new github-action for git-l10n code review
     
    -    Repository of git-l10n is a fork from "git/git" on GitHub, and uses
    +    The repository of git-l10n is a fork of "git/git" on GitHub, and uses
         GitHub pull request for code review. A helper program "git-po-helper"
         can be used to check typos in ".po" files, validate syntax, and check
         commit messages. It would be convenient to integrate this helper program
         to CI and add comments in pull request.
     
    -    The new github-action workflow is added in ".github/workflows/". It will
    -    only be enabled for the following cases:
    +    The new github-action workflow will be enabled for l10n related
    +    operations, such as:
     
    -     * A repository named as "git-po", such as a repository forked from
    -       "git-l10n/git-po".
    +     * Operations on a repository named as "git-po", such as a repository
    +       forked from "git-l10n/git-po".
     
    -     * Push to the branch that contains "l10n" in the name.
    +     * Push to a branch that contains "l10n" in the name.
     
    -     * Pull reqeust from a remote branch which has "l10n" in the name, such
    +     * Pull request from a remote branch which has "l10n" in the name, such
            as: "l10n/fix-fuzzy-translations".
     
         The new l10n workflow listens to two types of github events:
    @@ Commit message
             on: [push, pull_request_target]
     
         The reason we use "pull_request_target" instead of "pull_request" is
    -    that for security reasons, pull requests from forks receive a read-only
    -    GITHUB_TOKEN and workflows cannot write comments back to pull requests.
    -    GitHub provides a "pull_request_target" event to resolve security risks
    -    by checking out the base commit from the target repository, and provide
    -    write permissions for the workflow.
    +    that pull requests from forks receive a read-only GITHUB_TOKEN and
    +    workflows cannot write comments back to pull requests for security
    +    reasons. GitHub provides a "pull_request_target" event to resolve
    +    security risks by checking out the base commit from the target
    +    repository, and provide write permissions for the workflow.
     
    -    By default, adminstrators can set strict permissions for workflows. The
    +    By default, administrators can set strict permissions for workflows. The
         following code is used to modify the permissions for the GITHUB_TOKEN
    -    and give write permission in order to create comments in pull-requests.
    +    and grant write permission in order to create comments in pull-requests.
     
             permissions:
               pull-requests: write
    @@ Commit message
         like a l10n commit (no file in "po/" has been changed), the scan process
         will stop immediately. For a "push" event, no error will be reported
         because it is normal to push non-l10n commits merged from upstream. But
    -    for the "pull_request_target" event, errors will be reported.
    -    For this reason, additional option is provided for "git-po-helper".
    +    for the "pull_request_target" event, errors will be reported. For this
    +    reason, additional option is provided for "git-po-helper".
     
             git-po-helper check-commits \
                 --github-action-event="${{ github.event_name }}" -- \
    @@ Commit message
         package named "logrus" for logging, and I use an additional option
         "ForceColor" to initialize "logrus" to print messages in a user-friendly
         format in logfile output. These color codes help produce beautiful
    -    output for logs of workflow, but they must be stripped off when creating
    -    comments for pull requests. E.g.:
    +    output for the log of workflow, but they must be stripped off when
    +    creating comments for pull requests. E.g.:
     
             perl -pe 's/\e\[[0-9;]*m//g' git-po-helper.out
     
         "git-po-helper" may generate two kinds of suggestions, errors and
    -    warnings. All the errors and warnings will be reported in the log
    -    the l10n workflow. For a "pull_request_target" event, an additional
    -    comment will be created in the pull request to report the result.
    -    A l10n contributor should try to fix all the errors, and should pay
    -    attention to the warnings.
    +    warnings. All the errors and warnings will be reported in the log of the
    +    l10n workflow. However, warnings in the log of the workflow for a
    +    successfully running "git-po-helper" can easily be ignored by users.
    +    For the "pull_request_target" event, this issue is resolved by creating
    +    an additional comment in the pull request. A l10n contributor should try
    +    to fix all the errors, and should pay attention to the warnings.
     
         Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
    @@ .github/workflows/l10n.yml (new)
     +
     +jobs:
     +  git-po-helper:
    -+    if: endsWith(github.repository, '/git-po') || contains(github.head_ref, 'l10n') || contains(github.ref, 'l10n')
    ++    if: >-
    ++      endsWith(github.repository, '/git-po') ||
    ++      contains(github.head_ref, 'l10n') ||
    ++      contains(github.ref, 'l10n')
     +    runs-on: ubuntu-latest
     +    permissions:
     +      pull-requests: write
    @@ .github/workflows/l10n.yml (new)
     +      - name: Fetch missing commits
     +        id: fetch-commits
     +        run: |
    -+          # Setup partial clone.
    -+          git config remote.origin.promisor true
    -+          git config remote.origin.partialCloneFilter blob:none
     +          if test "${{ github.event_name }}" = "pull_request_target"
     +          then
     +            base=${{ github.event.pull_request.base.sha }}
    @@ .github/workflows/l10n.yml (new)
     +              ;;
     +            esac
     +          done
    -+          # Unshallow the repository, and fetch missing commits.
    -+          # "$base" may be missing due to forced push, and "$head"
    ++          # Unshallow the repository, and fetch missing commits using partial
    ++          # clone.  "$base" may be missing due to forced push, and "$head"
     +          # may be missing due to the "pull_request_target" event.
    -+          git fetch --unshallow origin $args
    ++          git fetch --unshallow --filter=blob:none origin $args
     +          echo "::set-output name=base::$base"
     +          echo "::set-output name=head::$head"
     +      - uses: actions/setup-go@v2
    @@ .github/workflows/l10n.yml (new)
     +        id: check-commits
     +        run: |
     +          exit_code=0
    -+          create_comment=
     +          git-po-helper check-commits \
     +              --github-action-event="${{ github.event_name }}" -- \
     +              ${{ steps.fetch-commits.outputs.base }}..${{ steps.fetch-commits.outputs.head }} \
     +              >git-po-helper.out 2>&1 ||
     +            exit_code=$?
    -+          if test $exit_code -ne 0 ||
    -+            grep -q -e "^level=warning" -e WARNING git-po-helper.out
    ++          if test $exit_code -ne 0 || grep -q WARNING git-po-helper.out
     +          then
    -+            create_comment=yes
     +            # Remove ANSI colors which are proper for console logs but not
     +            # proper for PR comment.
     +            echo "COMMENT_BODY<<EOF" >>$GITHUB_ENV
     +            perl -pe 's/\e\[[0-9;]*m//g; s/\bEOF$//g' git-po-helper.out >>$GITHUB_ENV
     +            echo "EOF" >>$GITHUB_ENV
     +          fi
    -+          echo "::set-output name=create_comment::$create_comment"
     +          cat git-po-helper.out
     +          exit $exit_code
    -+      - name: Report in comment for pull request
    ++      - name: Create comment in pull request for report
     +        uses: mshick/add-pr-comment@v1
    -+        if: always() && steps.check-commits.outputs.create_comment== 'yes' && github.event_name == 'pull_request_target'
    -+        continue-on-error: true
    ++        if: >-
    ++          always() &&
    ++          github.event_name == 'pull_request_target' &&
    ++          env.COMMENT_BODY != ''
     +        with:
     +          repo-token: ${{ secrets.GITHUB_TOKEN }}
     +          repo-token-user-login: 'github-actions[bot]'
    -+          message: |
    -+            ${{ steps.check-commits.outcome == 'failure' && 'Errors and warnings' || 'Warnings' }} found by [git-po-helper](https://github.com/git-l10n/git-po-helper#readme) in workflow [#${{ github.run_number }}](${{ env.GITHUB_SERVER_URL }}/${{ github.repository }}/actions/runs/${{ github.run_id }}):
    ++          message: >
    ++            ${{ steps.check-commits.outcome == 'failure' && 'Errors and warnings' || 'Warnings' }}
    ++            found by [git-po-helper](https://github.com/git-l10n/git-po-helper#readme) in workflow
    ++            [#${{ github.run_number }}](${{ env.GITHUB_SERVER_URL }}/${{ github.repository }}/actions/runs/${{ github.run_id }}):
     +
     +            ```
    ++
     +            ${{ env.COMMENT_BODY }}
    ++
     +            ```

## diff stat

Jiang Xin (1):
  ci: new github-action for git-l10n code review

 .github/workflows/l10n.yml | 93 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)
 create mode 100644 .github/workflows/l10n.yml

-- 
2.33.0

[PATCH v5 0/1] ci: new github-action for git-l10n code review

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

## Changes since v4

* "git-po-helper check-commits" can work with bare repository now. So do not use
  "actions/checkout@v2" to prepare workspace any more.

* "git-po-helper learned how to fetch missing blobs in a batch. So do a blobless
  partial clone.


## range-diff v4...v5

1:  0067ccc ! 1:  1c43d6b ci: new github-action for git-l10n code review
    @@ .github/workflows/l10n.yml (new)
     +    permissions:
     +      pull-requests: write
     +    steps:
    -+      - uses: actions/checkout@v2
    -+        with:
    -+          fetch-depth: '1'
    -+      - name: Fetch missing commits
    -+        id: fetch-commits
    ++      - name: Setup base and head objects
    ++        id: setup-tips
     +        run: |
     +          if test "${{ github.event_name }}" = "pull_request_target"
     +          then
    @@ .github/workflows/l10n.yml (new)
     +            base=${{ github.event.before }}
     +            head=${{ github.event.after }}
     +          fi
    ++          echo "::set-output name=base::$base"
    ++          echo "::set-output name=head::$head"
    ++      - name: Run partial clone
    ++        run: |
    ++          git -c init.defaultBranch=master init --bare .
    ++          git remote add \
    ++            --mirror=fetch \
    ++            origin \
    ++            https://github.com/${{ github.repository }}
    ++          # Fetch tips that may be unreachable from github.ref:
    ++          # - For a forced push, "$base" may be unreachable.
    ++          # - For a "pull_request_target" event, "$head" may be unreachable.
     +          args=
    -+          for commit in $base $head
    ++          for commit in \
    ++            ${{ steps.setup-tips.outputs.base }} \
    ++            ${{ steps.setup-tips.outputs.head }}
     +          do
     +            case $commit in
     +            *[^0]*)
     +              args="$args $commit"
     +              ;;
     +            *)
    -+              # Ignore ZERO-OID.
    ++              # Should not fetch ZERO-OID.
     +              ;;
     +            esac
     +          done
    -+          # Unshallow the repository, and fetch missing commits using partial
    -+          # clone.  "$base" may be missing due to forced push, and "$head"
    -+          # may be missing due to the "pull_request_target" event.
    -+          git fetch --unshallow --filter=blob:none origin $args
    -+          echo "::set-output name=base::$base"
    -+          echo "::set-output name=head::$head"
    ++          git -c protocol.version=2 fetch \
    ++            --progress \
    ++            --no-tags \
    ++            --no-write-fetch-head \
    ++            --filter=blob:none \
    ++            origin \
    ++            ${{ github.ref }} \
    ++            $args
     +      - uses: actions/setup-go@v2
     +        with:
     +          go-version: '>=1.16'
    @@ .github/workflows/l10n.yml (new)
     +        run: |
     +          exit_code=0
     +          git-po-helper check-commits \
    -+              --github-action-event="${{ github.event_name }}" -- \
    -+              ${{ steps.fetch-commits.outputs.base }}..${{ steps.fetch-commits.outputs.head }} \
    -+              >git-po-helper.out 2>&1 ||
    -+            exit_code=$?
    ++            --github-action-event="${{ github.event_name }}" -- \
    ++            ${{ steps.setup-tips.outputs.base }}..${{ steps.setup-tips.outputs.head }} \
    ++            >git-po-helper.out 2>&1 || exit_code=$?
     +          if test $exit_code -ne 0 || grep -q WARNING git-po-helper.out
     +          then
     +            # Remove ANSI colors which are proper for console logs but not

----

Jiang Xin (1):
  ci: new github-action for git-l10n code review

 .github/workflows/l10n.yml | 105 +++++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)
 create mode 100644 .github/workflows/l10n.yml

-- 
2.33.0

[PATCH v5 1/1] ci: new github-action for git-l10n code review

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

The repository of git-l10n is a fork of "git/git" on GitHub, and uses
GitHub pull request for code review. A helper program "git-po-helper"
can be used to check typos in ".po" files, validate syntax, and check
commit messages. It would be convenient to integrate this helper program
to CI and add comments in pull request.

The new github-action workflow will be enabled for l10n related
operations, such as:

 * Operations on a repository named as "git-po", such as a repository
   forked from "git-l10n/git-po".

 * Push to a branch that contains "l10n" in the name.

 * Pull request from a remote branch which has "l10n" in the name, such
   as: "l10n/fix-fuzzy-translations".

The new l10n workflow listens to two types of github events:

    on: [push, pull_request_target]

The reason we use "pull_request_target" instead of "pull_request" is
that pull requests from forks receive a read-only GITHUB_TOKEN and
workflows cannot write comments back to pull requests for security
reasons. GitHub provides a "pull_request_target" event to resolve
security risks by checking out the base commit from the target
repository, and provide write permissions for the workflow.

By default, administrators can set strict permissions for workflows. The
following code is used to modify the permissions for the GITHUB_TOKEN
and grant write permission in order to create comments in pull-requests.

    permissions:
      pull-requests: write

This workflow will scan commits one by one. If a commit does not look
like a l10n commit (no file in "po/" has been changed), the scan process
will stop immediately. For a "push" event, no error will be reported
because it is normal to push non-l10n commits merged from upstream. But
for the "pull_request_target" event, errors will be reported. For this
reason, additional option is provided for "git-po-helper".

    git-po-helper check-commits \
        --github-action-event="${{ github.event_name }}" -- \
        <base>..<head>

The output messages of "git-po-helper" contain color codes not only for
console, but also for logfile. This is because "git-po-helper" uses a
package named "logrus" for logging, and I use an additional option
"ForceColor" to initialize "logrus" to print messages in a user-friendly
format in logfile output. These color codes help produce beautiful
output for the log of workflow, but they must be stripped off when
creating comments for pull requests. E.g.:

    perl -pe 's/\e\[[0-9;]*m//g' git-po-helper.out

"git-po-helper" may generate two kinds of suggestions, errors and
warnings. All the errors and warnings will be reported in the log of the
l10n workflow. However, warnings in the log of the workflow for a
successfully running "git-po-helper" can easily be ignored by users.
For the "pull_request_target" event, this issue is resolved by creating
an additional comment in the pull request. A l10n contributor should try
to fix all the errors, and should pay attention to the warnings.

Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 .github/workflows/l10n.yml | 105 +++++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)
 create mode 100644 .github/workflows/l10n.yml

diff --git a/.github/workflows/l10n.yml b/.github/workflows/l10n.yml
new file mode 100644
index 0000000..27f72f0
--- /dev/null
+++ b/.github/workflows/l10n.yml
@@ -0,0 +1,105 @@
+name: git-l10n
+
+on: [push, pull_request_target]
+
+jobs:
+  git-po-helper:
+    if: >-
+      endsWith(github.repository, '/git-po') ||
+      contains(github.head_ref, 'l10n') ||
+      contains(github.ref, 'l10n')
+    runs-on: ubuntu-latest
+    permissions:
+      pull-requests: write
+    steps:
+      - name: Setup base and head objects
+        id: setup-tips
+        run: |
+          if test "${{ github.event_name }}" = "pull_request_target"
+          then
+            base=${{ github.event.pull_request.base.sha }}
+            head=${{ github.event.pull_request.head.sha }}
+          else
+            base=${{ github.event.before }}
+            head=${{ github.event.after }}
+          fi
+          echo "::set-output name=base::$base"
+          echo "::set-output name=head::$head"
+      - name: Run partial clone
+        run: |
+          git -c init.defaultBranch=master init --bare .
+          git remote add \
+            --mirror=fetch \
+            origin \
+            https://github.com/${{ github.repository }}
+          # Fetch tips that may be unreachable from github.ref:
+          # - For a forced push, "$base" may be unreachable.
+          # - For a "pull_request_target" event, "$head" may be unreachable.
+          args=
+          for commit in \
+            ${{ steps.setup-tips.outputs.base }} \
+            ${{ steps.setup-tips.outputs.head }}
+          do
+            case $commit in
+            *[^0]*)
+              args="$args $commit"
+              ;;
+            *)
+              # Should not fetch ZERO-OID.
+              ;;
+            esac
+          done
+          git -c protocol.version=2 fetch \
+            --progress \
+            --no-tags \
+            --no-write-fetch-head \
+            --filter=blob:none \
+            origin \
+            ${{ github.ref }} \
+            $args
+      - uses: actions/setup-go@v2
+        with:
+          go-version: '>=1.16'
+      - name: Install git-po-helper
+        run: go install github.com/git-l10n/git-po-helper@main
+      - name: Install other dependencies
+        run: |
+          sudo apt-get update -q &&
+          sudo apt-get install -q -y gettext
+      - name: Run git-po-helper
+        id: check-commits
+        run: |
+          exit_code=0
+          git-po-helper check-commits \
+            --github-action-event="${{ github.event_name }}" -- \
+            ${{ steps.setup-tips.outputs.base }}..${{ steps.setup-tips.outputs.head }} \
+            >git-po-helper.out 2>&1 || exit_code=$?
+          if test $exit_code -ne 0 || grep -q WARNING git-po-helper.out
+          then
+            # Remove ANSI colors which are proper for console logs but not
+            # proper for PR comment.
+            echo "COMMENT_BODY<<EOF" >>$GITHUB_ENV
+            perl -pe 's/\e\[[0-9;]*m//g; s/\bEOF$//g' git-po-helper.out >>$GITHUB_ENV
+            echo "EOF" >>$GITHUB_ENV
+          fi
+          cat git-po-helper.out
+          exit $exit_code
+      - name: Create comment in pull request for report
+        uses: mshick/add-pr-comment@v1
+        if: >-
+          always() &&
+          github.event_name == 'pull_request_target' &&
+          env.COMMENT_BODY != ''
+        with:
+          repo-token: ${{ secrets.GITHUB_TOKEN }}
+          repo-token-user-login: 'github-actions[bot]'
+          message: >
+            ${{ steps.check-commits.outcome == 'failure' && 'Errors and warnings' || 'Warnings' }}
+            found by [git-po-helper](https://github.com/git-l10n/git-po-helper#readme) in workflow
+            [#${{ github.run_number }}](${{ env.GITHUB_SERVER_URL }}/${{ github.repository }}/actions/runs/${{ github.run_id }}):
+
+            ```
+
+            ${{ env.COMMENT_BODY }}
+
+            ```
-- 
2.33.0

[PATCH v4 1/1] ci: new github-action for git-l10n code review

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

The repository of git-l10n is a fork of "git/git" on GitHub, and uses
GitHub pull request for code review. A helper program "git-po-helper"
can be used to check typos in ".po" files, validate syntax, and check
commit messages. It would be convenient to integrate this helper program
to CI and add comments in pull request.

The new github-action workflow will be enabled for l10n related
operations, such as:

 * Operations on a repository named as "git-po", such as a repository
   forked from "git-l10n/git-po".

 * Push to a branch that contains "l10n" in the name.

 * Pull request from a remote branch which has "l10n" in the name, such
   as: "l10n/fix-fuzzy-translations".

The new l10n workflow listens to two types of github events:

    on: [push, pull_request_target]

The reason we use "pull_request_target" instead of "pull_request" is
that pull requests from forks receive a read-only GITHUB_TOKEN and
workflows cannot write comments back to pull requests for security
reasons. GitHub provides a "pull_request_target" event to resolve
security risks by checking out the base commit from the target
repository, and provide write permissions for the workflow.

By default, administrators can set strict permissions for workflows. The
following code is used to modify the permissions for the GITHUB_TOKEN
and grant write permission in order to create comments in pull-requests.

    permissions:
      pull-requests: write

This workflow will scan commits one by one. If a commit does not look
like a l10n commit (no file in "po/" has been changed), the scan process
will stop immediately. For a "push" event, no error will be reported
because it is normal to push non-l10n commits merged from upstream. But
for the "pull_request_target" event, errors will be reported. For this
reason, additional option is provided for "git-po-helper".

    git-po-helper check-commits \
        --github-action-event="${{ github.event_name }}" -- \
        <base>..<head>

The output messages of "git-po-helper" contain color codes not only for
console, but also for logfile. This is because "git-po-helper" uses a
package named "logrus" for logging, and I use an additional option
"ForceColor" to initialize "logrus" to print messages in a user-friendly
format in logfile output. These color codes help produce beautiful
output for the log of workflow, but they must be stripped off when
creating comments for pull requests. E.g.:

    perl -pe 's/\e\[[0-9;]*m//g' git-po-helper.out

"git-po-helper" may generate two kinds of suggestions, errors and
warnings. All the errors and warnings will be reported in the log of the
l10n workflow. However, warnings in the log of the workflow for a
successfully running "git-po-helper" can easily be ignored by users.
For the "pull_request_target" event, this issue is resolved by creating
an additional comment in the pull request. A l10n contributor should try
to fix all the errors, and should pay attention to the warnings.

Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 .github/workflows/l10n.yml | 93 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)
 create mode 100644 .github/workflows/l10n.yml

diff --git a/.github/workflows/l10n.yml b/.github/workflows/l10n.yml
new file mode 100644
index 0000000..7e2385d
--- /dev/null
+++ b/.github/workflows/l10n.yml
@@ -0,0 +1,93 @@
+name: git-l10n
+
+on: [push, pull_request_target]
+
+jobs:
+  git-po-helper:
+    if: >-
+      endsWith(github.repository, '/git-po') ||
+      contains(github.head_ref, 'l10n') ||
+      contains(github.ref, 'l10n')
+    runs-on: ubuntu-latest
+    permissions:
+      pull-requests: write
+    steps:
+      - uses: actions/checkout@v2
+        with:
+          fetch-depth: '1'
+      - name: Fetch missing commits
+        id: fetch-commits
+        run: |
+          if test "${{ github.event_name }}" = "pull_request_target"
+          then
+            base=${{ github.event.pull_request.base.sha }}
+            head=${{ github.event.pull_request.head.sha }}
+          else
+            base=${{ github.event.before }}
+            head=${{ github.event.after }}
+          fi
+          args=
+          for commit in $base $head
+          do
+            case $commit in
+            *[^0]*)
+              args="$args $commit"
+              ;;
+            *)
+              # Ignore ZERO-OID.
+              ;;
+            esac
+          done
+          # Unshallow the repository, and fetch missing commits using partial
+          # clone.  "$base" may be missing due to forced push, and "$head"
+          # may be missing due to the "pull_request_target" event.
+          git fetch --unshallow --filter=blob:none origin $args
+          echo "::set-output name=base::$base"
+          echo "::set-output name=head::$head"
+      - uses: actions/setup-go@v2
+        with:
+          go-version: '>=1.16'
+      - name: Install git-po-helper
+        run: go install github.com/git-l10n/git-po-helper@main
+      - name: Install other dependencies
+        run: |
+          sudo apt-get update -q &&
+          sudo apt-get install -q -y gettext
+      - name: Run git-po-helper
+        id: check-commits
+        run: |
+          exit_code=0
+          git-po-helper check-commits \
+              --github-action-event="${{ github.event_name }}" -- \
+              ${{ steps.fetch-commits.outputs.base }}..${{ steps.fetch-commits.outputs.head }} \
+              >git-po-helper.out 2>&1 ||
+            exit_code=$?
+          if test $exit_code -ne 0 || grep -q WARNING git-po-helper.out
+          then
+            # Remove ANSI colors which are proper for console logs but not
+            # proper for PR comment.
+            echo "COMMENT_BODY<<EOF" >>$GITHUB_ENV
+            perl -pe 's/\e\[[0-9;]*m//g; s/\bEOF$//g' git-po-helper.out >>$GITHUB_ENV
+            echo "EOF" >>$GITHUB_ENV
+          fi
+          cat git-po-helper.out
+          exit $exit_code
+      - name: Create comment in pull request for report
+        uses: mshick/add-pr-comment@v1
+        if: >-
+          always() &&
+          github.event_name == 'pull_request_target' &&
+          env.COMMENT_BODY != ''
+        with:
+          repo-token: ${{ secrets.GITHUB_TOKEN }}
+          repo-token-user-login: 'github-actions[bot]'
+          message: >
+            ${{ steps.check-commits.outcome == 'failure' && 'Errors and warnings' || 'Warnings' }}
+            found by [git-po-helper](https://github.com/git-l10n/git-po-helper#readme) in workflow
+            [#${{ github.run_number }}](${{ env.GITHUB_SERVER_URL }}/${{ github.repository }}/actions/runs/${{ github.run_id }}):
+
+            ```
+
+            ${{ env.COMMENT_BODY }}
+
+            ```
-- 
2.33.0

[PATCH v2 0/1] ci: new github-action for git-l10n code review

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Git l10n uses github pull request for code review. A helper program
"git-po-helper" can be used to check typos in ".po" files, validate
syntax, and check commit message. It would be convenient to integrate
this helper program to CI and add comments in pull request.

A repository is created for testing git-l10n CI workflow. L10n
contributors can fork and try.

- https://github.com/jiangxin/github-action-test


## Changes since v1

+ Listen to event "pull_request_target" instead of event "pull_request".
  Event "pull_request_target" provides write permissions for
  GITHUB_TOKEN, and the workflow triggered by a pull request from a fork
  repository can create new comment in the pull request.
 
+ Because for "pull_request_target", only checkout base commit of the
  target repository, add a new step "Fetch missing commits".

+ Add new option "--github-action-event <event>" for git-po-helper.

+ Add "--end-of-options" for "git rev-parse" command.


## Range diff v1...v2

1:  25c5645 ! 1:  c2618b9 ci: new github-action for git-l10n code review
    @@ Commit message
         "ci-config" and create a simple shell script at "ci/config/allow-l10n"
         in this branch.
     
    -    The new l10n workflow listens to two types of github events: push and
    -    pull_request.
    +    The new l10n workflow listens to two types of github events: "push" and
    +    "pull_request_target". The "pull_request_target" event is just like the
    +    "pull_request" event, but provides write permission to create comments
    +    for pull request.
     
    -    For a push event, it will scan commits one by one. If a commit does not
    -    look like a l10n commit (no file in "po/" has been changed), it will
    -    immediately fail without checking for further commits. While for a
    -    pull_request event, all new introduced commits will be scanned.
    +    For a "push" event, it will scan commits one by one. If a commit does
    +    not look like a l10n commit (no file in "po/" has been changed), it
    +    will immediately fail without checking for further commits. While for a
    +    "pull_request_target" event, all new introduced commits will be scanned.
     
         "git-po-helper" will generate two kinds of suggestions, errors and
         warnings. A l10n contributor should try to fix all the errors, and
         should pay attention to the warnings. All the errors and warnings will
    -    be reported in the last step of the l10n workflow as two message groups.
    -    For a pull_request event, will create additional comments in pull
    -    request to report the result.
    +    be reported in the last step of the l10n workflow with two message
    +    groups. For a "pull_request_target" event, will create additional
    +    comments in the pull request to report the result.
     
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
    @@ .github/workflows/l10n.yml (new)
     @@
     +name: git-l10n
     +
    -+on: [push, pull_request]
    ++on: [push, pull_request_target]
     +
     +jobs:
     +  ci-config:
    @@ .github/workflows/l10n.yml (new)
     +    - uses: actions/checkout@v2
     +      with:
     +        fetch-depth: '0'
    ++    - name: Fetch missing commits
    ++      id: fetch-commits
    ++      run: |
    ++        if test "${{ github.event_name }}" = "pull_request_target"
    ++        then
    ++          base=${{ github.event.pull_request.base.sha }}
    ++          head=${{ github.event.pull_request.head.sha }}
    ++        else
    ++          base=${{ github.event.before }}
    ++          head=${{ github.event.after }}
    ++        fi
    ++        for commit in $base $head
    ++        do
    ++          if echo $commit | grep -q "^00*$"
    ++          then
    ++            continue
    ++          fi
    ++          if ! git rev-parse --verify --end-of-options "$commit^{commit}" --
    ++          then
    ++            git fetch origin $commit
    ++          fi
    ++        done
    ++        echo "::set-output name=base::$base"
    ++        echo "::set-output name=head::$head"
    ++
     +    - uses: actions/setup-go@v2
     +      with:
     +        go-version: ">=1.16"
    @@ .github/workflows/l10n.yml (new)
     +      run: |
     +        sudo apt-get update -q &&
     +        sudo apt-get install -q -y gettext
    -+    - id: check-commits
    -+      name: Run git-po-helper
    ++    - name: Run git-po-helper
    ++      id: check-commits
     +      run: |
    -+        if test "${{ github.event_name }}" = "pull_request"
    -+        then
    -+          commit_from=${{ github.event.pull_request.base.sha }}
    -+          commit_to=${{ github.event.pull_request.head.sha }}
    -+        else
    -+          commit_from=${{ github.event.before }}
    -+          commit_to=${{ github.event.after }}
    -+          if ! echo $commit_from | grep -q "^00*$"
    -+          then
    -+            if ! git rev-parse "$commit_from^{commit}"
    -+            then
    -+              git fetch origin $commit_from
    -+            fi
    -+          fi
    -+        fi
     +        exit_code=0
    -+        git-po-helper check-commits --github-action -- \
    -+          $commit_from..$commit_to >git-po-helper.out 2>&1 ||
    -+        exit_code=$?
    ++        git-po-helper check-commits \
    ++            --github-action \
    ++            --github-action-event "${{ github.event_name }}" -- \
    ++            ${{ steps.fetch-commits.outputs.base }}..${{ steps.fetch-commits.outputs.head }} \
    ++            >git-po-helper.out 2>&1 ||
    ++          exit_code=$?
     +        echo "::set-output name=exit_code::$exit_code"
     +        has_error_msg=
     +        has_warning_msg=
     +        if test $exit_code -ne 0
     +        then
     +          has_error_msg=yes
    -+          if test "${{ github.event_name }}" = "pull_request"
    ++          if test "${{ github.event_name }}" = "pull_request_target"
     +          then
     +            echo "ERROR_MSG<<EOF" >>$GITHUB_ENV
     +            grep -v -e "^level=warning" -e WARNING git-po-helper.out |
    @@ .github/workflows/l10n.yml (new)
     +        if grep -q -e "^level=warning" -e WARNING git-po-helper.out
     +        then
     +          has_warning_msg=yes
    -+          if test "${{ github.event_name }}" = "pull_request"
    ++          if test "${{ github.event_name }}" = "pull_request_target"
     +          then
     +            echo "WARNING_MSG<<EOF" >>$GITHUB_ENV
     +            grep -v -e "^level=error" -e ERROR git-po-helper.out |
    @@ .github/workflows/l10n.yml (new)
     +        echo "::set-output name=has_warning_msg::$has_warning_msg"
     +    - name: Report errors in comment for pull request
     +      uses: mshick/add-pr-comment@v1
    -+      if: steps.check-commits.outputs.has_error_msg == 'yes' && github.event_name == 'pull_request'
    ++      if: steps.check-commits.outputs.has_error_msg == 'yes' && github.event_name == 'pull_request_target'
     +      continue-on-error: true
     +      with:
     +        repo-token: ${{ secrets.GITHUB_TOKEN }}
    @@ .github/workflows/l10n.yml (new)
     +          ```
     +    - name: Report warnings in comment for pull request
     +      uses: mshick/add-pr-comment@v1
    -+      if: steps.check-commits.outputs.has_warning_msg == 'yes' && github.event_name == 'pull_request'
    ++      if: steps.check-commits.outputs.has_warning_msg == 'yes' && github.event_name == 'pull_request_target'
     +      continue-on-error: true
     +      with:
     +        repo-token: ${{ secrets.GITHUB_TOKEN }}
    @@ .github/workflows/l10n.yml (new)
     +          ```
     +          ${{ env.WARNING_MSG }}
     +          ```
    -+    - name: Report and quit
    ++    - name: Final report
     +      run: |
     +        if test "${{ steps.check-commits.outputs.has_error_msg }}" = "yes"
     +        then

[PATCH v2 1/1] ci: new github-action for git-l10n code review

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Git l10n uses github pull request for code review. A helper program
"git-po-helper" can be used to check typos in ".po" files, validate
syntax, and check commit message. It would be convenient to integrate
this helper program to CI and add comments in pull request.

The new github-action workflow is added in ".github/workflows/l10n.yml",
which is disabled by default. To turn it on for the git-l10n related
repositories, such as "git-l10n/git-po", we can add a new branch named
"ci-config" and create a simple shell script at "ci/config/allow-l10n"
in this branch.

The new l10n workflow listens to two types of github events: "push" and
"pull_request_target". The "pull_request_target" event is just like the
"pull_request" event, but provides write permission to create comments
for pull request.

For a "push" event, it will scan commits one by one. If a commit does
not look like a l10n commit (no file in "po/" has been changed), it
will immediately fail without checking for further commits. While for a
"pull_request_target" event, all new introduced commits will be scanned.

"git-po-helper" will generate two kinds of suggestions, errors and
warnings. A l10n contributor should try to fix all the errors, and
should pay attention to the warnings. All the errors and warnings will
be reported in the last step of the l10n workflow with two message
groups. For a "pull_request_target" event, will create additional
comments in the pull request to report the result.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 .github/workflows/l10n.yml | 156 +++++++++++++++++++++++++++++++++++++
 1 file changed, 156 insertions(+)
 create mode 100644 .github/workflows/l10n.yml

diff --git a/.github/workflows/l10n.yml b/.github/workflows/l10n.yml
new file mode 100644
index 0000000..bb4d8b3
--- /dev/null
+++ b/.github/workflows/l10n.yml
@@ -0,0 +1,156 @@
+name: git-l10n
+
+on: [push, pull_request_target]
+
+jobs:
+  ci-config:
+    runs-on: ubuntu-latest
+    outputs:
+      enabled: ${{ steps.check-l10n.outputs.enabled }}
+    steps:
+      - name: try to clone ci-config branch
+        run: |
+          git -c protocol.version=2 clone \
+            --no-tags \
+            --single-branch \
+            -b ci-config \
+            --depth 1 \
+            --no-checkout \
+            --filter=blob:none \
+            https://github.com/${{ github.repository }} \
+            config-repo &&
+          cd config-repo &&
+          git checkout HEAD -- ci/config || : ignore
+      - id: check-l10n
+        name: check whether CI is enabled for l10n
+        run: |
+          enabled=no
+          if test -x config-repo/ci/config/allow-l10n &&
+             config-repo/ci/config/allow-l10n '${{ github.ref }}'
+          then
+            enabled=yes
+          fi
+          echo "::set-output name=enabled::$enabled"
+
+  git-po-helper:
+    needs: ci-config
+    if: needs.ci-config.outputs.enabled == 'yes'
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
+      with:
+        fetch-depth: '0'
+    - name: Fetch missing commits
+      id: fetch-commits
+      run: |
+        if test "${{ github.event_name }}" = "pull_request_target"
+        then
+          base=${{ github.event.pull_request.base.sha }}
+          head=${{ github.event.pull_request.head.sha }}
+        else
+          base=${{ github.event.before }}
+          head=${{ github.event.after }}
+        fi
+        for commit in $base $head
+        do
+          if echo $commit | grep -q "^00*$"
+          then
+            continue
+          fi
+          if ! git rev-parse --verify --end-of-options "$commit^{commit}" --
+          then
+            git fetch origin $commit
+          fi
+        done
+        echo "::set-output name=base::$base"
+        echo "::set-output name=head::$head"
+
+    - uses: actions/setup-go@v2
+      with:
+        go-version: ">=1.16"
+    - name: Install git-po-helper
+      run: |
+        go install github.com/git-l10n/git-po-helper@main
+    - name: Install other dependencies
+      run: |
+        sudo apt-get update -q &&
+        sudo apt-get install -q -y gettext
+    - name: Run git-po-helper
+      id: check-commits
+      run: |
+        exit_code=0
+        git-po-helper check-commits \
+            --github-action \
+            --github-action-event "${{ github.event_name }}" -- \
+            ${{ steps.fetch-commits.outputs.base }}..${{ steps.fetch-commits.outputs.head }} \
+            >git-po-helper.out 2>&1 ||
+          exit_code=$?
+        echo "::set-output name=exit_code::$exit_code"
+        has_error_msg=
+        has_warning_msg=
+        if test $exit_code -ne 0
+        then
+          has_error_msg=yes
+          if test "${{ github.event_name }}" = "pull_request_target"
+          then
+            echo "ERROR_MSG<<EOF" >>$GITHUB_ENV
+            grep -v -e "^level=warning" -e WARNING git-po-helper.out |
+              perl -pe 's/\e\[[0-9;]*m//g' >>$GITHUB_ENV
+            echo "EOF" >>$GITHUB_ENV
+          fi
+        fi
+        if grep -q -e "^level=warning" -e WARNING git-po-helper.out
+        then
+          has_warning_msg=yes
+          if test "${{ github.event_name }}" = "pull_request_target"
+          then
+            echo "WARNING_MSG<<EOF" >>$GITHUB_ENV
+            grep -v -e "^level=error" -e ERROR git-po-helper.out |
+              perl -pe 's/\e\[[0-9;]*m//g' >>$GITHUB_ENV
+            echo "EOF" >>$GITHUB_ENV
+          fi
+        fi
+        echo "::set-output name=has_error_msg::$has_error_msg"
+        echo "::set-output name=has_warning_msg::$has_warning_msg"
+    - name: Report errors in comment for pull request
+      uses: mshick/add-pr-comment@v1
+      if: steps.check-commits.outputs.has_error_msg == 'yes' && github.event_name == 'pull_request_target'
+      continue-on-error: true
+      with:
+        repo-token: ${{ secrets.GITHUB_TOKEN }}
+        repo-token-user-login: 'github-actions[bot]'
+        message: |
+          Errors found by git-po-helper in workflow ${{ github.workflow }}:
+          ```
+          ${{ env.ERROR_MSG }}
+          ```
+    - name: Report warnings in comment for pull request
+      uses: mshick/add-pr-comment@v1
+      if: steps.check-commits.outputs.has_warning_msg == 'yes' && github.event_name == 'pull_request_target'
+      continue-on-error: true
+      with:
+        repo-token: ${{ secrets.GITHUB_TOKEN }}
+        repo-token-user-login: 'github-actions[bot]'
+        message: |
+          Warnings found by git-po-helper in workflow ${{ github.workflow }}:
+          ```
+          ${{ env.WARNING_MSG }}
+          ```
+    - name: Final report
+      run: |
+        if test "${{ steps.check-commits.outputs.has_error_msg }}" = "yes"
+        then
+          echo "::group::Errors found by git-po-helper"
+          grep -v -e "^level=warning" -e WARNING git-po-helper.out
+          echo "::endgroup::"
+        fi
+        if test "${{ steps.check-commits.outputs.has_warning_msg }}" = "yes"
+        then
+          echo "::group::Warnings found by git-po-helper"
+          grep -v -e "^level=error" -e ERROR git-po-helper.out
+          echo "::endgroup::"
+        fi
+        if test ${{ steps.check-commits.outputs.exit_code }} -ne 0
+        then
+          exit ${{ steps.check-commits.outputs.exit_code }}
+        fi
-- 
2.33.0