{"thread":{"id":"60144","subject":"[PATCH] .github/workflows: add coverity action","startedAt":"2023-08-22T03:34:37Z","lastAt":"2023-09-23T07:09:08Z","messageCount":13,"participants":["Taylor Blau","Jeff King","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"480914","messageId":"4590e1381feb8962cadf2b40b22086531d662ef8.1692675172.git.me@ttaylorr.com","threadId":"60144","inReplyTo":null,"subject":"[PATCH] .github/workflows: add coverity action","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-22T03:34:32Z","receivedAt":"2023-08-22T03:34:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Coverity is a static analysis tool that detects and generates reports on\nvarious security and code quality issues.\n\nIt is particularly useful when diagnosing memory safety issues which may\nbe used as part of exploiting a security vulnerability.\n\nCoverity's website provides a service accepts \"builds\" (which is more or\nless the collection of '*.o' files generated during a standard build\nwith \"make\") as input and generates reports as output. In order to\ngenerate a report, we have to first compile Git and then upload the\nbuild archive to Coverity.\n\nThis Action generates and uploads a build archive to Coverity when a\nGitHub repository has configured the \"COVERITY_SCAN_EMAIL\" and\n\"COVERITY_SCAN_TOKEN\" repository secrets, respectively. This enables\nCoverity to automatically report on new changes pushed to 'master', as\nwell as any newly created tags.\n\nA couple of implementation notes:\n\n  - In order to successfully build 'git', we (ab-)use the\n    ci/install-dependencies.sh script by faking in some of the\n    environment variables to take on values specific to the main.yml\n    workflow file in order to install the correct set of dependencies.\n\n  - We could upload the build archive to Coverity directly with a\n    straightforward curl request. But using the vapier/coverity-scan\n    Action comes with some additional niceties, such as caching the\n    (rather large) Coverity tool download between runs.\n\nIf the repository is missing either of the two secret tokens mentioned\nearlier, this Action is a no-op.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n .github/workflows/coverity.yml | 35 ++++++++++++++++++++++++++++++++++\n 1 file changed, 35 insertions(+)\n create mode 100644 .github/workflows/coverity.yml\n\ndiff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml\nnew file mode 100644\nindex 0000000000..26b9145d9e\n--- /dev/null\n+++ b/.github/workflows/coverity.yml\n@@ -0,0 +1,35 @@\n+name: Coverity\n+\n+on:\n+  push:\n+    branches:\n+      - master\n+    tags:\n+      - '*'\n+\n+jobs:\n+  coverity:\n+    runs-on: ubuntu-latest\n+    env:\n+      HAVE_COVERITY_TOKEN: ${{ secrets.COVERITY_SCAN_EMAIL != '' && secrets.COVERITY_SCAN_TOKEN != '' }}\n+    steps:\n+      - id: check-coverity\n+        name: check whether Coverity token is configured\n+        run: |\n+          echo \"enabled=$HAVE_COVERITY_TOKEN\" >>$GITHUB_OUTPUT\n+      - uses: actions/checkout@v3\n+        if: steps.check-coverity.outputs.enabled == 'true'\n+      - run: ci/install-dependencies.sh\n+        env:\n+          CC: gcc\n+          CC_PACKAGE: gcc-9\n+          jobname: linux-gcc-default\n+          runs_on_pool: ubuntu-latest\n+        if: steps.check-coverity.outputs.enabled == 'true'\n+      - uses: vapier/coverity-scan-action@v1\n+        if: steps.check-coverity.outputs.enabled == 'true'\n+        with:\n+          email: ${{ secrets.COVERITY_SCAN_EMAIL }}\n+          token: ${{ secrets.COVERITY_SCAN_TOKEN }}\n+          command: make -j8\n+\n-- \n2.42.0.15.geed1e1a32a\n"},{"id":"481038","messageId":"20230825213201.GD2382334@coredump.intra.peff.net","threadId":"60144","inReplyTo":"4590e1381feb8962cadf2b40b22086531d662ef8.1692675172.git.me@ttaylorr.com","subject":"Re: [PATCH] .github/workflows: add coverity action","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-25T21:32:01Z","receivedAt":"2023-08-25T21:32:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 21, 2023 at 11:34:32PM -0400, Taylor Blau wrote:\n\n> Coverity is a static analysis tool that detects and generates reports on\n> various security and code quality issues.\n> \n> It is particularly useful when diagnosing memory safety issues which may\n> be used as part of exploiting a security vulnerability.\n\nI've been running a variant of this on my private repo for the last year\nor two. The biggest issue IMHO is that we don't have a good way to\nsurface problems publicly. The CI \"passes\" in the sense that the build\nis uploaded to coverity, but the interesting bit is whether any new\ndefects were found (because there are a ton of existing false positives\nthat I haven't figured out how to silence). So my flow is something\nlike:\n\n  1. Push to my fork.\n\n  2. Eventually get an email that says \"N new defects found\".\n\n  3. If N > 0, log into the coverity site and look at them.\n\nWhat's the plan for adding this to the main git.git repo? Is it to run\non Junio's tree, and then have interested folks sign up for coverity\naccess and look at it there? If so, how does step 2 work?\n\nOr is it just to have it, so interested parties can do their own\ncoverity analysis? I'd be happy enough with that, as I am carrying a\nsimilar patch locally.\n\n> Coverity's website provides a service accepts \"builds\" (which is more or\n> less the collection of '*.o' files generated during a standard build\n> with \"make\") as input and generates reports as output. In order to\n> generate a report, we have to first compile Git and then upload the\n> build archive to Coverity.\n\nI don't think this is quite right. You run a build with their\n\"cov-build\" wrapper, which does some magic. It _does_ invoke the actual\ncompiler, but does some extra magic, too (I don't know the details, but\nit is running cov-translate and cov-emit on the actual source file).\n\nThis maybe seems like nit-picking, but it will play into some questions\nI have on the patch itself.\n\n>   - We could upload the build archive to Coverity directly with a\n>     straightforward curl request. But using the vapier/coverity-scan\n>     Action comes with some additional niceties, such as caching the\n>     (rather large) Coverity tool download between runs.\n\nYeah, that is a nice bonus over what I've been using. The tool download\nis 800MB!\n\n> diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml\n> new file mode 100644\n> index 0000000000..26b9145d9e\n> --- /dev/null\n> +++ b/.github/workflows/coverity.yml\n> @@ -0,0 +1,35 @@\n> +name: Coverity\n> +\n> +on:\n> +  push:\n> +    branches:\n> +      - master\n> +    tags:\n> +      - '*'\n\nIt's probably a good idea not to run this against every single topic\nbranch. But it's nice if we can catch bugs _before_ they hit master.\nSince this isn't a blocking CI job, that's aspirational (it depends on\nsomebody looking at the results and patching in a timely manner). But I\nroutinely do my runs against \"next\" (actually an integration branch that\nI base on \"next\") and investigate the results within a day or two.\n\nRunning against \"seen\" might be even better (though personally I usually\navoid looking at it too closely, as sometimes it has too much of other\npeople's broken stuff. I have plenty of my own).\n\nI'm not sure what workflow you're envisioning here.\n\n> +jobs:\n> +  coverity:\n> +    runs-on: ubuntu-latest\n> +    env:\n> +      HAVE_COVERITY_TOKEN: ${{ secrets.COVERITY_SCAN_EMAIL != '' && secrets.COVERITY_SCAN_TOKEN != '' }}\n> +    steps:\n> +      - id: check-coverity\n> +        name: check whether Coverity token is configured\n> +        run: |\n> +          echo \"enabled=$HAVE_COVERITY_TOKEN\" >>$GITHUB_OUTPUT\n\nThis check-coverity step is good, as obviously most people's forks won't\nbe set up to run it. It is a shame we'll have to kick off an actions vm\njust to echo \"enabled=0\". Is there any way to embed that logic directly\nin the yaml (I won't be surprised if the answer is \"no\"; I've been\nlooking for years for something similar for check-config in the main\nworkflow).\n\n> +      - uses: actions/checkout@v3\n> +        if: steps.check-coverity.outputs.enabled == 'true'\n> +      - run: ci/install-dependencies.sh\n> +        env:\n> +          CC: gcc\n> +          CC_PACKAGE: gcc-9\n> +          jobname: linux-gcc-default\n> +          runs_on_pool: ubuntu-latest\n> +        if: steps.check-coverity.outputs.enabled == 'true'\n\nIn the workflow file I've been using, I didn't need to do any of this CC\nmagic. I do run install-dependencies, but it seems to work fine without\nextra variables. I do set runs_on_pool=ubuntu-latest in mine. Maybe that\nmatters (it does hit UBUNTU_COMMON_PKGS)?\n\n> +      - uses: vapier/coverity-scan-action@v1\n> +        if: steps.check-coverity.outputs.enabled == 'true'\n> +        with:\n> +          email: ${{ secrets.COVERITY_SCAN_EMAIL }}\n> +          token: ${{ secrets.COVERITY_SCAN_TOKEN }}\n> +          command: make -j8\n\nOK, this is presumably doing the cov-build stuff behind the scenes, as\nwell as the curl upload. In my file, we have to feed a \"project\" field.\nI guess the action just figures this out from whichever repo we're\nusing. Do GitHub repo names always correspond to coverity project names?\n(It does seem kind of superfluous; I think the SCAN_TOKEN is unique to\nthe project).\n\n-Peff\n"},{"id":"481039","messageId":"20230825215603.GA2400192@coredump.intra.peff.net","threadId":"60144","inReplyTo":"20230825213201.GD2382334@coredump.intra.peff.net","subject":"Re: [PATCH] .github/workflows: add coverity action","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-25T21:56:03Z","receivedAt":"2023-08-25T21:57:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 25, 2023 at 05:32:01PM -0400, Jeff King wrote:\n\n> > Coverity's website provides a service accepts \"builds\" (which is more or\n> > less the collection of '*.o' files generated during a standard build\n> > with \"make\") as input and generates reports as output. In order to\n> > generate a report, we have to first compile Git and then upload the\n> > build archive to Coverity.\n> \n> I don't think this is quite right. You run a build with their\n> \"cov-build\" wrapper, which does some magic. It _does_ invoke the actual\n> compiler, but does some extra magic, too (I don't know the details, but\n> it is running cov-translate and cov-emit on the actual source file).\n> \n> This maybe seems like nit-picking, but it will play into some questions\n> I have on the patch itself.\n\nI guess I never actually asked those questions, but they were: why are\nyou asking for some specific version of gcc since we don't really care\nmuch about the actual compiler.\n\nBut I think you don't care about the compiler version; you were just\ntrying to feed enough data to install-dependencies to get it to run.\n\n-Peff\n"},{"id":"481110","messageId":"6534ba5d-f4a6-71e6-5b0f-9cba2be8426e@gmx.de","threadId":"60144","inReplyTo":"4590e1381feb8962cadf2b40b22086531d662ef8.1692675172.git.me@ttaylorr.com","subject":"Re: [PATCH] .github/workflows: add coverity action","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-08-29T08:18:24Z","receivedAt":"2023-08-29T08:19:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Taylor,\n\nOn Mon, 21 Aug 2023, Taylor Blau wrote:\n\n> diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml\n> new file mode 100644\n> index 0000000000..26b9145d9e\n> --- /dev/null\n> +++ b/.github/workflows/coverity.yml\n> @@ -0,0 +1,35 @@\n> +name: Coverity\n> +\n> +on:\n> +  push:\n> +    branches:\n> +      - master\n> +    tags:\n> +      - '*'\n\nI would consider it too late to do a Coverity analysis when the version\nis already tagged. Therefore, I would like to see this `tags: *' part go.\n\nAnd I agree with Peff that we should activate this build for `next`, too.\nAnd for good measure for `maint` (and maybe `maint-*`, although those\nbranches are typically not pushed to `git/git`).\n\n> +\n> +jobs:\n> +  coverity:\n> +    runs-on: ubuntu-latest\n> +    env:\n> +      HAVE_COVERITY_TOKEN: ${{ secrets.COVERITY_SCAN_EMAIL != '' && secrets.COVERITY_SCAN_TOKEN != '' }}\n> +    steps:\n> +      - id: check-coverity\n> +        name: check whether Coverity token is configured\n> +        run: |\n> +          echo \"enabled=$HAVE_COVERITY_TOKEN\" >>$GITHUB_OUTPUT\n\nThe canonical output for a step that has no other outputs is called\n`result`. That output name is handled by linters such as\nhttps://rhysd.github.io/actionlint/, too, in contrast to outputs with\ndifferent names that may be set in a free-form `run:` step that is\nimpossible to parse by `actionlint`.\n\nBut I see a bigger problem: Contrary to what the commit message claims,\nthe design makes this workflow far from a no-op: There will always be the\nneed for a VM to be spun up, to receive the orchestration script, to\ndownload the necessary Actions (in this instance,\n`vapier/coverity-scan-action`), only to check environment variables, see\nthat actually nothing needs to be done, update the job's and the run's\nstatus (claiming \"success\" even if nothing was sent to Coverity) and then\nshut down.\n\nWe discussed this in the past, as it is a great waste of resources (even\nif other people pay for it, it still costs money [*1*] and electricity),\nand for some of us even more relevant: it is a great waste of time because\nwhen you are already waiting for 20 queued jobs to be picked up, having to\nwait for the next build agent to become available only to spend, I don't\nknow, half a minute (wall clock time)? on getting this workflow run\nqueued, the next build agent to become available, the job to be set up,\nthe first step to be run, the others to be skipped, and then to wrap it\nall up by updating the run as completed. I would not fault anyone for\nbeing frustrated with this.\n\nTherefore, I would strongly suggest looking for alternative ways to skip\nthis workflow run, before applying this patch.\n\nI see these viable alternatives, all pulling the `if:` directive from the\nstep level to the job level, just like the `l10n.yml` workflow does [*2*],\nto avoid even starting the job unless really needed:\n\n- Limit it to the primary repository, `git/git`:\n\n\tif: github.event.repository.fork == false\n\n  This is what Git for Windows does in the build that validates the\n  revision to use in the `git-for-windows/setup-git-for-windows-sdk`\n  Action that is used in Git's CI runs to build and test Git on Windows:\n  https://github.com/git-for-windows/git-sdk-64/blob/894191270a78/.github/workflows/ci-artifacts.yml#L14\n\n  While this still clutters the list of workflow runs with plenty of\n  skipped runs (see https://github.com/git/git/actions/workflows/l10n.yml\n  for an example how this looks), at least it avoids wasting a build agent\n  on essentially only checking for the presence of two secrets.\n\n  Developers wishing to run this workflow in their fork could always\n  create a branch, edit the workflow to remove this `if:` directive, and\n  push the branch to their fork.\n\n- Limit it to `git/git` explicitly, by name:\n\n\tif: github.event.repository.owner.login == 'git'\n\n  This is how we run the `vs-build`/`vs-test` combo only in the\n  `git-for-windows` fork:\n  https://github.com/git/git/blob/v2.42.0/.github/workflows/main.yml#L150\n\n  The advantage here is that it would be slightly more obvious what we are\n  doing here than checking `fork == false`, as the org name is mentioned\n  explicitly.\n\n- Limit it to `git/git` but also allow it to be triggered manually:\n\n\tif: github.event.repository.fork == false || github.event_name == 'workflow_dispatch'\n\n  This is what Git for Windows does in the test verifying that it works in\n  the \"Nano Server\" container image that allows for kind of a minimal\n  Windows system to be run via Docker:\n  https://github.com/git-for-windows/git-sdk-64/blob/894191270a78/.github/workflows/nano-server.yml#L7\n\n  The advantage of allowing the workflow to be triggered via a\n  `workflow_dispatch` is that developers wishing to run this workflow in\n  their fork would not need to edit the GitHub workflow but could run it\n  unmodified. For details, see\n  https://docs.github.com/en/actions/using-workflows/manually-running-a-workflow\n\n- Limit it by repository \"topics\" (think: \"repository tags\"):\n\n\tif: contains(github.event.repository.topics, 'has-coverity-secrets')\n\n  On GitHub, every repository can be annotated with \"topics\", i.e. a list\n  of labels (which in other scenarios would be called \"tags\"). These\n  topics can be seen by the GitHub workflow run, _before_ even starting a\n  job, via the `github.event.repository` attribute.\n\n  This is how InnoSetup does it:\n  https://github.com/jrsoftware/issrc/blob/main/.github/workflows/build.yml#L12\n  The primary repository does not have the secrets required to run this\n  workflow, and hence does not have the \"has-issrc-build-env\" topic as\n  can be seen here: https://github.com/jrsoftware/issrc (look below the\n  \"About\" section on the right side, where you see the topics \"installer\"\n  and \"inno-setup\"). My personal fork does have them secrets, and the\n  topic \"has-issrc-build-env\" says as much: https://github.com/dscho/issrc\n\n  The advantage of this solution is that it is very easy to \"turn on and\n  off\" the workflow on a repository level, simply by adding or removing\n  the \"has-coverity-secrets\" topic. See:\n  https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/classifying-your-repository-with-topics\n\n  The downside is that this method is somewhat unintuitive and needs the\n  developer to read documentation (*shudder*) to know what they need to\n  do.\n\nI have no strong preference as long as the job is skipped whenever the\nCoverity build (read: the `coverity` GitHub workflow run) should be\nskipped.\n\n> +      - uses: actions/checkout@v3\n> +        if: steps.check-coverity.outputs.enabled == 'true'\n> +      - run: ci/install-dependencies.sh\n> +        env:\n> +          CC: gcc\n> +          CC_PACKAGE: gcc-9\n> +          jobname: linux-gcc-default\n\nHow about modifying `ci/install-dependencies.sh` to introduce a new job\nname, `coverity`, that is handled like `linux-gcc-default`? I.e. something\nlike this:\n\n-- snip --\ndiff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh\nindex 107757a1fea..e6a894e507d 100755\n--- a/ci/install-dependencies.sh\n+++ b/ci/install-dependencies.sh\n@@ -74,7 +74,7 @@ Documentation)\n \ttest -n \"$ALREADY_HAVE_ASCIIDOCTOR\" ||\n \tsudo gem install --version 1.5.8 asciidoctor\n \t;;\n-linux-gcc-default)\n+linux-gcc-default|coverity)\n \tsudo apt-get -q update\n \tsudo apt-get -q -y install $UBUNTU_COMMON_PKGS\n \t;;\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex 1b0cc2b57db..7a196aa0d17 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -227,7 +227,7 @@ export SKIP_DASHED_BUILT_INS=YesPlease\n\n case \"$runs_on_pool\" in\n ubuntu-latest)\n-\tif test \"$jobname\" = \"linux-gcc-default\"\n+\tif test \"$jobname\" = \"linux-gcc-default\" ||  test \"$jobname\" = \"coverity\"\n \tthen\n \t\tbreak\n \tfi\n-- snap --\n\nThat would be a lot cleaner and would also allow for Coverity-specific\nthings in this step that won't affect `linux-gcc-default` at the same\ntime.\n\n> +          runs_on_pool: ubuntu-latest\n> +        if: steps.check-coverity.outputs.enabled == 'true'\n> +      - uses: vapier/coverity-scan-action@v1\n\nIn contrast to the other Actions we use in Git, this one is neither\nprovided by GitHub nor by the Git (or Git for Windows) project [*3*].\n\nThe recommended way to handle such third-party Actions is to pin them by\nfull length commit SHA:\nhttps://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-third-party-actions\n\nThis is particularly important because we're passing secret values to that\nAction.\n\n> +        if: steps.check-coverity.outputs.enabled == 'true'\n> +        with:\n> +          email: ${{ secrets.COVERITY_SCAN_EMAIL }}\n> +          token: ${{ secrets.COVERITY_SCAN_TOKEN }}\n> +          command: make -j8\n> +\n\nThe commit message would make for a fine place to describe the role of\nthese `COVERITY_SCAN_*` secrets, how to obtain them, and how to configure\nthem (i.e. providing a link to\nhttps://docs.github.com/en/actions/security-guides/encrypted-secrets#creating-encrypted-secrets-for-a-repository).\n\nThe reason I am suggesting that is that many a time, I have authored\ncommits like this one, only to puzzle about the exact steps to configure\nthe the thing properly when I needed it six months later, and regretting\ndearly not having had the foresight to help my future self while writing\nthe commit message.\n\nAnother important issue that should be discussed in the commit message is\nthe matter of Coverity's false positives. I regularly dread going through\nthe new reports in Coverity (I am doing this for git-for-windows) because\nof the many falsely pointed out \"buffer overruns\" when working e.g. with\n`strvec` where Coverity assumes that its `empty_strvec` is ever written\nto.\n\nIn Git for Windows, I have not had any luck with suppressing these false\nreports. For what it's worth, this is the \"model file\" I am currently\nusing:\n\n-- snip --\n/* modelfile for git */\n\nchar strbuf_slopbuf[64];\n\nvoid *malloc(size_t);\nvoid *calloc(size_t, size_t);\nvoid *realloc(void *, size_t);\nvoid free(void *);\n\nvoid *xrealloc(void *ptr, size_t size)\n{\n\tvoid *ret = realloc(ptr, size);\n\tif (!ret) __coverity_panic__();\n\treturn ret;\n}\n\nvoid *xmalloc(size_t size)\n{\n\tvoid *mem = malloc(size);\n\tif (!mem) __coverity_panic__();\n\treturn mem;\n}\n\nvoid xcalloc(size_t num, size_t size)\n{\n\tvoid *ret = calloc(num, size);\n\tif (!ret)  __coverity_panic__();\n\treturn ret;\n}\n\nvoid usage(const char *err) {\n  __coverity_panic__();\n}\n\nvoid usagef(const char *err, ...) {\n  __coverity_panic__();\n}\n\nvoid die(const char *err, ...)  {\n  __coverity_panic__();\n}\n\nvoid die_errno(const char *err, ...) {\n  __coverity_panic__();\n}\n\nvoid BUG_fl(const char *file, int line, const char *fmt, ...) {\n  __coverity_panic__();\n}\n\nstruct strbuf {\n\tsize_t alloc;\n\tsize_t len;\n\tchar *buf;\n};\n\nvoid strbuf_setlen(struct strbuf *sb, size_t len)\n{\n        sb->len = len;\n\tsb->buf[len] = '\\0';\n}\n\nvoid strbuf_grow(struct strbuf *sb, size_t amount);\n\nvoid *memcpy(void *dest, const void *src, size_t n);\n\nvoid strbuf_add(struct strbuf *sb, const void *data, size_t len)\n{\n\tstrbuf_grow(sb, len);\n\tmemcpy(sb->buf + sb->len, data, len);\n\tsb->len = len;\n\tsb->buf[len] = '\\0';\n}\n\nvoid *memmove(void *dest, const void *src, size_t n);\n\nvoid strbuf_splice(struct strbuf *sb, size_t pos, size_t len,\n\t\t\t\t   const void *data, size_t dlen)\n{\n\tif (pos > sb->len)\n\t\tdie(\"`pos' is too far after the end of the buffer\");\n\tif (pos + len > sb->len)\n\t\tdie(\"`pos + len' is too far after the end of the buffer\");\n\n\tif (dlen >= len)\n\t\tstrbuf_grow(sb, dlen - len);\n\tmemmove(sb->buf + pos + dlen,\n\t\t\tsb->buf + pos + len,\n\t\t\tsb->len - pos - len);\n\tmemcpy(sb->buf + pos, data, dlen);\n\tsb->len = sb->len + dlen - len;\n\tsb->buf[sb->len + dlen - len] = '\\0';\n}\n-- snap --\n\nIt is getting a bit ridiculous at this point, the size of it and the\nmeager benefit it reaps. But it is better than nothing, I guess.\n\nThe reason I would really like to see an extensive discussion about this\nin the commit message is that this place would be _the_ most logical\nhabitat for such a description, to help developers who want to set up\ntheir own Coverity coverage (either in their own Git fork, or even in a\nseparate project) trying to learn from the Git project.\n\nCiao,\nJohannes\n\nFootnote *1*: In case it was unclear: Build agents are essentially virtual\nmachines. Those virtual machines are destroyed after every run, and\nrecreated to a pristine state before picking up the next job. To avoid\nhaving to wait for a virtual machine to be created whenever a job needs to\nbe run, there is a pool of virtual machines ready to run. There have been\ntimes when we ran so many jobs in the Git project that we drained that\npool and had to wait, painfully, for the virtual machines to be created\nwhich, let me tell you, is not a matter of a mere couple of seconds.\n\nThis complexity is reflected in the pricing you can see at\nhttps://docs.github.com/en/billing/managing-billing-for-github-actions/about-billing-for-github-actions\n\nJust to put things into perspective: In our security work, we have to run\nour GitHub workflow in a private repository, where GitHub Actions are not\nprovided free of cost by GitHub. We have a very nice donation from GitHub\nin the form of 50,000 build minutes we can use every month. If we spent\nall of those minutes running on Linux (i.e. the cheapest option), GitHub\nwould essentially donate $400 to us. That's what we usually spend in a\nmatter of days preparing for security releases: The workflow run to\nvalidate v2.39.3, for example, while taking \"only\" 36 minutes and 1 second\nwall clock time, used up over 12 hours of those 50,000 build minutes, and\nevery time we iterated, we pushed 11 branches and 11 tags. Three\niterations in, we were out of build minutes.\n\nAll this is to say: We, the Git project, are not exactly cautious when it\ncomes to spending GitHub Actions build minutes. I really would love to see\nus all to become a lot more mindful on this matter.\n\nFootnote *2*:\nhttps://github.com/git/git/blob/v2.42.0/.github/workflows/l10n.yml#L13\n\nFootnote *3*: We do already use a third-party Action in `l10n.yml`:\n`mshick/add-pr-comment@v1`. As you can verify easily, this is not pinned\nby full length commit SHA. But then, this step only gets the\n`GITHUB_TOKEN` secret, which only has the permission to write to the Pull\nRequest (i.e. add a comment), not to write to the repository.\n\nStill, we will probably want to pin that Action at some stage, better safe\n'n sorry. #leftoverbits\n"},{"id":"481165","messageId":"20230830001844.GA227310@coredump.intra.peff.net","threadId":"60144","inReplyTo":"6534ba5d-f4a6-71e6-5b0f-9cba2be8426e@gmx.de","subject":"Re: [PATCH] .github/workflows: add coverity action","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-30T00:18:44Z","receivedAt":"2023-08-30T00:19:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 29, 2023 at 10:18:24AM +0200, Johannes Schindelin wrote:\n\n> - Limit it by repository \"topics\" (think: \"repository tags\"):\n> \n> \tif: contains(github.event.repository.topics, 'has-coverity-secrets')\n\nFWIW, I like this approach the most. As you note, it's not exactly\nobvious to discover, but I think it is the most flexible. And setting up\nCoverity runs already requires a lot of non-obvious steps (like creating\nan account with them and getting a token to paste into the GitHub\nsecrets area).\n\nMy gut feeling is that we should be able to do something with env\nvariables to avoid the (ab)use of repository tags, but when I looked\ninto this in the past, it wasn't possible.  However, poking at it again\nnow, it seems that the \"vars\" context (but not \"secrets\") is available\nto \"jobs.*.if\". I'm not sure if I missed before, or if that's a new\nfeature since the last time I checked.\n\nAt any rate, it seems to work to do:\n\n  if: vars.ENABLE_COVERITY != ''\n\nor even make COVERITY_SCAN_EMAIL a \"var\" instead of a \"secret\", and use\nthat. Likewise, I think we could do:\n\n  if: contains(vars.COVERITY_BRANCHES, github.ref_name)\n\nto allow individual repositories to do their own branch selection (that\nis matching by substring, which is probably good enough, but if you want\nto get fancy, I think we can use fromJSON to interpret the contents of\nthe variable).\n\n(I had mostly looked into this in the context of branch selection for\nour ci-config job, and I think we could do something similar there).\n\n-Peff\n"},{"id":"481186","messageId":"36143de8-8343-b75d-4ba6-d7241535ba91@gmx.de","threadId":"60144","inReplyTo":"20230830001844.GA227310@coredump.intra.peff.net","subject":"Re: [PATCH] .github/workflows: add coverity action","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-08-30T08:07:35Z","receivedAt":"2023-08-30T18:29:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 29 Aug 2023, Jeff King wrote:\n\n> On Tue, Aug 29, 2023 at 10:18:24AM +0200, Johannes Schindelin wrote:\n>\n> > - Limit it by repository \"topics\" (think: \"repository tags\"):\n> >\n> > \tif: contains(github.event.repository.topics, 'has-coverity-secrets')\n>\n> FWIW, I like this approach the most. [...]\n>\n> My gut feeling is that we should be able to do something with env\n> variables [...]\n\nEnvironment variables need an environment, i.e. a running build agent.\nThat's why they aren't available in our use case, but only inside a step\n(which is too late for our purposes).\n\nI am unsure why secrets aren't available in job-level `if:` expressions,\nbut they aren't, and that's that, for now.\n\n> [...] it seems that the \"vars\" context (but not \"secrets\") is available\n> to \"jobs.*.if\". I'm not sure if I missed before, or if that's a new\n> feature since the last time I checked.\n\nI had missed that, too. It was announced here:\nhttps://github.blog/2023-01-10-introducing-required-workflows-and-configuration-variables-to-github-actions/#configuration-variables\n(I must have glanced over that post when I saw it because it talked about\nrequired workflows, which are currently irrelevant to my interests).\n\nFWIW the feature is documented here:\nhttps://docs.github.com/en/actions/learn-github-actions/variables\n\nAnd\nhttps://docs.github.com/en/actions/learn-github-actions/variables#using-the-vars-context-to-access-configuration-variable-values\nspecifically says:\n\n\tConfiguration variables can be accessed across the workflow using\n\t`vars` context.\n\nI.e. it suggests that the context can be used even in the `run-name`\nattribute of any workflow. Nice.\n\nFWIW I was unable to deduce any authoritative information as to where the\n`secrets` context can be accessed from\nhttps://docs.github.com/en/actions/learn-github-actions/contexts#secrets-context,\nbut I must assume that access to that context is highly restricted and\nprobably cannot be used outside the `steps:` attribute, explaining why a\njob-level (and in my previous tests, even step-level) `if:` condition\ncannot access them.\n\n> (I had mostly looked into this in the context of branch selection for\n> our ci-config job, and I think we could do something similar there).\n\nFWIW I concur.\n\nCiao,\nJohannes\n"},{"id":"481201","messageId":"20230830190309.GA1707474@coredump.intra.peff.net","threadId":"60144","inReplyTo":"36143de8-8343-b75d-4ba6-d7241535ba91@gmx.de","subject":"Re: [PATCH] .github/workflows: add coverity action","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-30T19:03:09Z","receivedAt":"2023-08-30T23:04:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 30, 2023 at 10:07:35AM +0200, Johannes Schindelin wrote:\n\n> > My gut feeling is that we should be able to do something with env\n> > variables [...]\n> \n> Environment variables need an environment, i.e. a running build agent.\n> That's why they aren't available in our use case, but only inside a step\n> (which is too late for our purposes).\n\nYeah, sorry I was a bit loose with my language. I meant \"variables set\nfrom the GitHub UI that are often used as env variables\". :)\n\n> > [...] it seems that the \"vars\" context (but not \"secrets\") is available\n> > to \"jobs.*.if\". I'm not sure if I missed before, or if that's a new\n> > feature since the last time I checked.\n> \n> I had missed that, too. It was announced here:\n> https://github.blog/2023-01-10-introducing-required-workflows-and-configuration-variables-to-github-actions/#configuration-variables\n> (I must have glanced over that post when I saw it because it talked about\n> required workflows, which are currently irrelevant to my interests).\n\nYeah, I found that, too (after sending my email). So it looks like this\nis a good, supported path for going forward.\n\nI have a working patch that can replace the ci/config/allow-ref script.\nWe can't totally ditch the \"config\" job with it, as there are other\nthings it does, but it's possible those can be migrated, too. IMHO the\nallow-ref one is the _most_ important, though, because it addresses the\ncase of \"everything should be skipped\".  Taking that from spinning up\none VM per ref to zero is very good.\n\nI'll post that patch later today.\n\n> FWIW I was unable to deduce any authoritative information as to where the\n> `secrets` context can be accessed from\n> https://docs.github.com/en/actions/learn-github-actions/contexts#secrets-context,\n> but I must assume that access to that context is highly restricted and\n> probably cannot be used outside the `steps:` attribute, explaining why a\n> job-level (and in my previous tests, even step-level) `if:` condition\n> cannot access them.\n\nI found the \"vars\" context through this table:\n\n  https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability\n\nwhich does show where \"secrets\" is available.\n\n-Peff\n"},{"id":"482119","messageId":"b23951c569660e1891a7fb3ad2c2ea1952897bd7.1695332105.git.me@ttaylorr.com","threadId":"60144","inReplyTo":"4590e1381feb8962cadf2b40b22086531d662ef8.1692675172.git.me@ttaylorr.com","subject":"[PATCH v2] .github/workflows: add coverity action","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-21T21:53:31Z","receivedAt":"2023-09-21T22:08:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Coverity is a static analysis tool that detects and generates reports on\nvarious security and code quality issues.\n\nIt is particularly useful when diagnosing memory safety issues which may\nbe used as part of exploiting a security vulnerability.\n\nCoverity's website provides a service accepts \"builds\" (which is the\noutput of running `cov-build` on your project, which invokes `make` with\nadditional magic) as input and generates reports as output. In order to\ngenerate a report, we have to first compile Git and then upload the\nbuild archive to Coverity.\n\nThis Action generates and uploads a build archive to Coverity when a\nGitHub repository has done the following:\n\n- Configured the \"COVERITY_SCAN_EMAIL\" and \"COVERITY_SCAN_TOKEN\"\n  repository secrets. Tokens are found on the \"Project Settings\" page at\n  [1]. Tokens may be added as repository secrets on GitHub repositories\n  by following the guide at [2].\n\n- Enabled Coverity builds by (in addition to the above) creating a\n  repository variable called `ENABLE_COVERITY`. Repository variables\n  (which are different than secrets) can be added according to the guide\n  at [3].\n\nThis enables Coverity to automatically report on new changes pushed to\nthe configured branch set, which is specified via the\n`COVERITY_BRANCHES` repository variable.\n\nThe implementation is mostly straightforward. Though note that we could\nupload the build archive to Coverity directly with a straightforward\ncurl request. But using the vapier/coverity-scan Action comes with some\nadditional niceties, such as caching the (rather large) Coverity tool\ndownload between runs.\n\nIf the repository does not have the `ENABLE_COVERITY` variable set, or\nthe list of branches specified by `COVERITY_BRANCHES` does not contain\nthe branch being pushed to, this Action is a no-op.\n\n[1]: https://scan.coverity.com/projects/NAME?tab=project_settings\n[2]: https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions\n[3]: https://docs.github.com/en/actions/learn-github-actions/variables\n\nHelped-by: Jeff King <peff@peff.net>\nHelped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nThis fell to the bottom of my queue, but I got back to it today while\ndoing some ~~spring~~ fall inbox cleaning :-). Thanks Peff and Johannes\nfor helpful review in the first round. Range-diff is below:\n\nRange-diff against v1:\n1:  f74ae75ddb < -:  ---------- .github/workflows: add coverity action\n-:  ---------- > 1:  b23951c569 .github/workflows: add coverity action\n\n .github/workflows/coverity.yml | 22 ++++++++++++++++++++++\n ci/install-dependencies.sh     |  2 +-\n ci/lib.sh                      |  2 +-\n 3 files changed, 24 insertions(+), 2 deletions(-)\n create mode 100644 .github/workflows/coverity.yml\n\ndiff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml\nnew file mode 100644\nindex 0000000000..3ba00b3929\n--- /dev/null\n+++ b/.github/workflows/coverity.yml\n@@ -0,0 +1,22 @@\n+name: Coverity\n+\n+on: [push, pull_request]\n+\n+jobs:\n+  coverity:\n+    if: (vars.ENABLE_COVERITY == 'true') &&\n+      (vars.COVERITY_BRANCHES == '' ||\n+       contains(vars.COVERITY_BRANCHES, github.ref_name) ||\n+       contains(vars.COVERITY_BRANCHES, '*'))\n+    runs-on: ubuntu-latest\n+    steps:\n+      - uses: actions/checkout@v3\n+      - run: ci/install-dependencies.sh\n+        env:\n+          jobname: coverity\n+      - uses: vapier/coverity-scan-action@cae3c096a2eb21c431961a49375ac17aea2670ce\n+        with:\n+          email: ${{ secrets.COVERITY_SCAN_EMAIL }}\n+          token: ${{ secrets.COVERITY_SCAN_TOKEN }}\n+          command: make -j8\n+\ndiff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh\nindex 4f407530d3..7e100ee63f 100755\n--- a/ci/install-dependencies.sh\n+++ b/ci/install-dependencies.sh\n@@ -74,7 +74,7 @@ Documentation)\n \ttest -n \"$ALREADY_HAVE_ASCIIDOCTOR\" ||\n \tsudo gem install --version 1.5.8 asciidoctor\n \t;;\n-linux-gcc-default)\n+linux-gcc-default|coverity)\n \tsudo apt-get -q update\n \tsudo apt-get -q -y install $UBUNTU_COMMON_PKGS\n \t;;\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex 6fbb5bade1..2ad0ae340e 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -227,7 +227,7 @@ export SKIP_DASHED_BUILT_INS=YesPlease\n\n case \"$runs_on_pool\" in\n ubuntu-*)\n-\tif test \"$jobname\" = \"linux-gcc-default\"\n+\tif test \"$jobname\" = \"linux-gcc-default\" || test \"$jobname\" = \"coverity\"\n \tthen\n \t\tbreak\n \tfi\n--\n2.42.0.242.gc844f407a1\n"},{"id":"482121","messageId":"xmqqo7hv3y11.fsf@gitster.g","threadId":"60144","inReplyTo":"b23951c569660e1891a7fb3ad2c2ea1952897bd7.1695332105.git.me@ttaylorr.com","subject":"Re: [PATCH v2] .github/workflows: add coverity action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-21T23:30:02Z","receivedAt":"2023-09-21T23:30:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> This fell to the bottom of my queue, but I got back to it today while\n> doing some ~~spring~~ fall inbox cleaning :-). Thanks Peff and Johannes\n> for helpful review in the first round. Range-diff is below:\n>\n> Range-diff against v1:\n> 1:  f74ae75ddb < -:  ---------- .github/workflows: add coverity action\n> -:  ---------- > 1:  b23951c569 .github/workflows: add coverity action\n\nThat's a useful range-diff ;-).  Even with --word-diff, range-diff does\nnot notice that they correspond to each other, without an absurd setting\nlike --creation-factor=999.\n\n>  .github/workflows/coverity.yml | 22 ++++++++++++++++++++++\n>  ci/install-dependencies.sh     |  2 +-\n>  ci/lib.sh                      |  2 +-\n>  3 files changed, 24 insertions(+), 2 deletions(-)\n>  create mode 100644 .github/workflows/coverity.yml\n>\n> diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml\n> new file mode 100644\n> index 0000000000..3ba00b3929\n> --- /dev/null\n> +++ b/.github/workflows/coverity.yml\n> @@ -0,0 +1,22 @@\n> +name: Coverity\n> +\n> +on: [push, pull_request]\n\nThis no longer hardcocdes the condition to master and tagged ones,\nas ...\n\n> +jobs:\n> +  coverity:\n> +    if: (vars.ENABLE_COVERITY == 'true') &&\n> +      (vars.COVERITY_BRANCHES == '' ||\n> +       contains(vars.COVERITY_BRANCHES, github.ref_name) ||\n> +       contains(vars.COVERITY_BRANCHES, '*'))\n\n... this lets you control when to run it via the \"vars\".  This round\nalso can act on pull-requests in addition to pushes.\n\n> +    runs-on: ubuntu-latest\n> +    steps:\n> +      - uses: actions/checkout@v3\n> +      - run: ci/install-dependencies.sh\n> +        env:\n> +          jobname: coverity\n> +      - uses: vapier/coverity-scan-action@cae3c096a2eb21c431961a49375ac17aea2670ce\n> +        with:\n> +          email: ${{ secrets.COVERITY_SCAN_EMAIL }}\n> +          token: ${{ secrets.COVERITY_SCAN_TOKEN }}\n> +          command: make -j8\n\nAnd the actual implementation is vastly different by just using a\ncanned one, which requires less maintenance on our end, which is\nnice.\n\n> diff --git a/ci/lib.sh b/ci/lib.sh\n> index 6fbb5bade1..2ad0ae340e 100755\n> --- a/ci/lib.sh\n> +++ b/ci/lib.sh\n> @@ -227,7 +227,7 @@ export SKIP_DASHED_BUILT_INS=YesPlease\n>\n>  case \"$runs_on_pool\" in\n>  ubuntu-*)\n> -\tif test \"$jobname\" = \"linux-gcc-default\"\n> +\tif test \"$jobname\" = \"linux-gcc-default\" || test \"$jobname\" = \"coverity\"\n>  \tthen\n>  \t\tbreak\n>  \tfi\n\nThis part is new in this iteration, to avoid further customization\nthat enables more exotic features, which makes sense.\n\n\n"},{"id":"482134","messageId":"d29b5fd3-3b44-89ca-62b4-cf93b23031fd@gmx.de","threadId":"60144","inReplyTo":"b23951c569660e1891a7fb3ad2c2ea1952897bd7.1695332105.git.me@ttaylorr.com","subject":"Re: [PATCH v2] .github/workflows: add coverity action","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-09-22T11:09:26Z","receivedAt":"2023-09-22T11:09:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Taylor,\n\nthank you for continuing your work on this. The timing is a bit\nunfortunate, though: As I had not heard back from you in this thread, I\nhad started work yesterday morning on converting Git for Windows'\ncorresponding Azure Pipeline to a GitHub workflow as a consequence. I had\nto leave some polishing for this morning because the validation that the\nworkflow actually runs correctly and is versatile enough to address both\nthe needs of the Git project as well as of the Git for Windows project\ntook longer than anticipated and revealed a couple of problems on which I\nwill elaborate below.\n\nOn Thu, 21 Sep 2023, Taylor Blau wrote:\n\n> [...] using the vapier/coverity-scan Action comes with some additional\n> niceties, such as caching the (rather large) Coverity tool download\n> between runs.\n\nWhile it is true that that Action caches the Coverity Tool, this GitHub\nAction comes with a few non-niceties, too, one of them being that it\nactually fails to do its job. Looking at\nhttps://github.com/vapier/coverity-scan-action/blob/main/action.yml it can\nbe very easily verified that `cov-configure` is not called. But as\nhttps://github.com/git-for-windows/git/actions/runs/6267593979/job/17020975444#step:9:12\ndemonstrates, this command now _needs_ to be called:\n\n\tError:  Could not find file coverity_config.xml in directory D:/a/_temp/cov-analysis/config\n\n\tThe cov-build.exe command needs a configuration file, generated by\n\tcov-configure, to know how to capture analysis inputs based on\n\tcompiler invocations and/or scanning the filesystem.\n\tExamples of running cov-configure:\n\n\tcov-configure --cs          Microsoft C# compiler (csc)\n\tcov-configure --gcc         GNU C/C++ compiler (gcc/g++)\n\tcov-configure --java        Oracle Java compiler (javac)\n\tcov-configure --javascript  *.js JavaScript source code\n\tcov-configure --msvc        Microsoft C/C++ compiler (cl)\n\n\tSee cov-configure documentation for more information.\n\nI had encountered this issue already in April last year and had addressed\nit in https://github.com/git-for-windows/build-extra/commit/cec961d2536c,\nso it came as an unwelcome surprise to me that this GitHub Action leaves\nthis problem unaddressed.\n\nAnother issue I ran into: That Action defaults to using the URL-encoded\nname of the repository (in our case, `git%2Fgit`) as Coverity project\nname. That project does not exist, though, and the GitHub Action therefore\nsilently assumes an empty MD5 and attempts to extract a tar file that is\nactually a text file instead, with the contents \"Not found\".\n\nFurther, when I tried to specify `win64` as this GitHub Action's\n`platform` input variable (whose existence suggests that platforms other\nthan `linux64` are allowed), it totally fell over, trying to untar a\n`.zip` file.\n\nFor this and for various other reasons detailed in the cover letter of\nhttps://lore.kernel.org/git/pull.1588.git.1695379323.gitgitgadget@gmail.com,\nit would appear that unfortunately that GitHub Action won't be helpful\nhere.\n\nCiao,\nJohannes\n"},{"id":"482197","messageId":"20230923061053.GA1469962@coredump.intra.peff.net","threadId":"60144","inReplyTo":"b23951c569660e1891a7fb3ad2c2ea1952897bd7.1695332105.git.me@ttaylorr.com","subject":"Re: [PATCH v2] .github/workflows: add coverity action","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-23T06:10:53Z","receivedAt":"2023-09-23T06:10:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 21, 2023 at 05:53:31PM -0400, Taylor Blau wrote:\n\n> diff --git a/.github/workflows/coverity.yml b/.github/workflows/coverity.yml\n> new file mode 100644\n> index 0000000000..3ba00b3929\n> --- /dev/null\n> +++ b/.github/workflows/coverity.yml\n> @@ -0,0 +1,22 @@\n> +name: Coverity\n> +\n> +on: [push, pull_request]\n> +\n> +jobs:\n> +  coverity:\n> +    if: (vars.ENABLE_COVERITY == 'true') &&\n> +      (vars.COVERITY_BRANCHES == '' ||\n> +       contains(vars.COVERITY_BRANCHES, github.ref_name) ||\n> +       contains(vars.COVERITY_BRANCHES, '*'))\n\nI wonder if we really need this ENABLE_COVERITY flag. It's not _too_\nhard to set up, but it feels like we can eliminate a setup step and just\ninfer it from other variables.  Either:\n\n  1. Treat unset COVERITY_BRANCHES as \"do not run\". Unlike the CI job,\n     there's not much useful signal in running this for every PR. A\n     human has to go periodically look at the coverity output and make\n     sense of it. So I think it's something that would get run\n     periodically (say, on updates of \"next\") just for one or two\n     branches.\n\n  2. Use COVERITY_SCAN_EMAIL as a clue that the feature it is enabled. I\n     don't think we can do that as-is because it's a secret, not a var.\n     But is there a reason for the EMAIL to be a secret? I don't think\n     repository vars are fully public; they're just not hidden as deeply\n     as secrets. It seems like the right level of privacy for an email.\n\n> +    runs-on: ubuntu-latest\n> +    steps:\n> +      - uses: actions/checkout@v3\n> +      - run: ci/install-dependencies.sh\n> +        env:\n> +          jobname: coverity\n> +      - uses: vapier/coverity-scan-action@cae3c096a2eb21c431961a49375ac17aea2670ce\n> +        with:\n> +          email: ${{ secrets.COVERITY_SCAN_EMAIL }}\n> +          token: ${{ secrets.COVERITY_SCAN_TOKEN }}\n> +          command: make -j8\n\nI ran the action and it worked out of the box for me (I have everything\nset up on the coverity side already from my custom workflow), modulo one\nhiccup. I initially had COVERITY_SCAN_EMAIL as a var, so the upload\nfailed, complaining of an empty email.\n\nBut much to my surprise, the Action still succeeded. It didn't record\nthe HTTP result code, so I'm not sure if we could detect this with \"curl\n--fail-with-body\" or if we'd have to scrape the output. It may not be\nthat big a deal either way, though, since the coverity output really is\nuseless until a human periodically scans over it (but it would be nice\nto know if it was routinely failing; I might not notice if it didn't).\n\n-Peff\n"},{"id":"482198","messageId":"20230923062137.GB1469962@coredump.intra.peff.net","threadId":"60144","inReplyTo":"d29b5fd3-3b44-89ca-62b4-cf93b23031fd@gmx.de","subject":"Re: [PATCH v2] .github/workflows: add coverity action","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-23T06:21:37Z","receivedAt":"2023-09-23T06:21:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 22, 2023 at 01:09:26PM +0200, Johannes Schindelin wrote:\n\n> While it is true that that Action caches the Coverity Tool, this GitHub\n> Action comes with a few non-niceties, too, one of them being that it\n> actually fails to do its job. Looking at\n> https://github.com/vapier/coverity-scan-action/blob/main/action.yml it can\n> be very easily verified that `cov-configure` is not called. But as\n> https://github.com/git-for-windows/git/actions/runs/6267593979/job/17020975444#step:9:12\n> demonstrates, this command now _needs_ to be called:\n\nHmm. I have been running Coverity for years without running\ncov-configure (using my own workflow), and I just tested Taylor's patch,\nwhich worked fine. I wonder what is different about our setups; maybe it\nis only an issue on the Windows version of Coverity?\n\nOr maybe it has to do with the project setup on Coverity's server side,\nsince we have to provide the project name as part of the tool download\n(though I don't know if that is purely for logging/auditing on their\nside or if the delivered download is customized in any way).\n\n> Another issue I ran into: That Action defaults to using the URL-encoded\n> name of the repository (in our case, `git%2Fgit`) as Coverity project\n> name. That project does not exist, though, and the GitHub Action therefore\n> silently assumes an empty MD5 and attempts to extract a tar file that is\n> actually a text file instead, with the contents \"Not found\".\n\nIt took me a minute to understand what you meant here, since the main\nuse of the project name is for uploading our tarball. But we also must\nprovide it (along with the token) just to download their scan software,\nso that step failed.\n\nAnd yes, I agree that the error checking in the Action could be better\n(I also mentioned elsewhere in the thread that it did not detect or\nreport a failed upload). I'm not sure if it is easier to submit a patch\nthere, or if we are better off just doing it all inline (it is not that\nmuch code either way).\n\n> Further, when I tried to specify `win64` as this GitHub Action's\n> `platform` input variable (whose existence suggests that platforms other\n> than `linux64` are allowed), it totally fell over, trying to untar a\n> `.zip` file.\n\nHmm, yes, I hadn't really considered running Coverity with different\nbuild configurations. But I guess it needs to run code through the\ncompiler to do its magic, so you'd want to be able to run it on a\nWindows build to hit any platform-specific code.\n\nFor general git/git use, I'm not sure if we'd want to run it on multiple\nplatforms, but I guess your plan would be to share the workflow code\nbetween git/ and git-for-windows/, and use a repo variable to select the\nplatform for the latter. And for that case we can't rely on an Action\nwhich doesn't support Windows.\n\nI'll take a look at the patches you posted.\n\n-Peff\n"},{"id":"482206","messageId":"20230923070903.GD1471672@coredump.intra.peff.net","threadId":"60144","inReplyTo":"20230923062137.GB1469962@coredump.intra.peff.net","subject":"Re: [PATCH v2] .github/workflows: add coverity action","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-23T07:09:03Z","receivedAt":"2023-09-23T07:09:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 23, 2023 at 02:21:37AM -0400, Jeff King wrote:\n\n> I'll take a look at the patches you posted.\n\nOK, I just read over them. I do think the result is not much more\ncomplicated than using the vapier Action, and it fits our multi-platform\nneeds a bit better.\n\nI had a few small comments about curl usage, but it mostly looks like a\ngood direction to me.\n\n-Peff\n"}]}