{"thread":{"id":"54271","subject":"[PATCH] ci: github action - add check for whitespace errors","startedAt":"2020-09-22T07:28:09Z","lastAt":"2020-10-10T06:30:02Z","messageCount":17,"participants":["Chris. Webster via GitGitGadget","Jeff King","Junio C Hamano","Chris Webster","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"406114","messageId":"pull.709.git.1600759684548.gitgitgadget@gmail.com","threadId":"54271","inReplyTo":null,"subject":"[PATCH] ci: github action - add check for whitespace errors","fromName":"Chris. Webster via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-22T07:28:04Z","receivedAt":"2020-09-22T07:28:09Z","isPatch":true,"sender":{"key":"name:Chris. Webster","avatar":null},"body":"From: \"Chris. Webster\" \n\nNot all developers are aware of `git diff --check` to warn\nabout whitespace issues. Running a check when a pull request is\nopened or updated can save time for reviewers and the submitter.\n\nA GitHub workflow will run when a pull request is created or the\ncontents are updated to check the patch series. A pull request\nprovides the necessary information (number of commits) to only\ncheck the patch series.\n\nTo ensure the developer is aware of any issues, a comment will be\nadded to the pull request with the check errors.\n\nSigned-off-by: Chris. Webster \n---\n ci: GitHub Action - add check for whitespace errors\n \n Not all developers are aware of git diff --check to warn about\n whitespace issues. Running a check when a pull request is opened or\n updated can save time for reviewers and the submitter.\n \n A GitHub workflow will run when a pull request is created or the\n contents are updated to check the patch series. A pull request provides\n the necessary information (number of commits) to only check the patch\n series.\n \n To ensure the developer is aware of any issues, a comment will be added\n to the pull request with the check errors.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-709%2Fwebstech%2Fcw%2Fdiffcheck-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-709/webstech/cw/diffcheck-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/709\n\n .github/workflows/check-whitespace.yml | 69 ++++++++++++++++++++++++++\n 1 file changed, 69 insertions(+)\n create mode 100644 .github/workflows/check-whitespace.yml\n\ndiff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml\nnew file mode 100644\nindex 0000000000..9d070b9cdf\n--- /dev/null\n+++ b/.github/workflows/check-whitespace.yml\n@@ -0,0 +1,69 @@\n+name: check-whitespace\n+\n+# Get the repo with the commits(+1) in the series.\n+# Process `git log --check` output to extract just the check errors.\n+# Add a comment to the pull request with the check errors.\n+\n+on:\n+ pull_request:\n+ types: [opened, synchronize]\n+\n+jobs:\n+ check-whitespace:\n+ runs-on: ubuntu-latest\n+ steps:\n+ - name: Set commit count\n+ shell: bash\n+ run: echo \"::set-env name=COMMIT_DEPTH::$((1+$COMMITS))\"\n+ env:\n+ COMMITS: ${{ github.event.pull_request.commits }}\n+\n+ - uses: actions/checkout@v2\n+ with:\n+ fetch-depth: ${{ env.COMMIT_DEPTH }}\n+\n+ - name: git log --check\n+ id: check_out\n+ run: |\n+ log=\n+ commit=\n+ while read dash etc\n+ do\n+ case \"${dash}\" in\n+ \"---\")\n+ commit=\"${etc}\"\n+ ;;\n+ \"\")\n+ ;;\n+ *)\n+ if test -n \"${commit}\"\n+ then\n+ log=\"${log}\\n${commit}\"\n+ echo \"\"\n+ echo \"--- ${commit}\"\n+ fi\n+ commit=\n+ log=\"${log}\\n${dash} ${etc}\"\n+ echo \"${dash} ${etc}\"\n+ ;;\n+ esac\n+ done <<< $(git log --check --pretty=format:\"---% h% s\" -${{github.event.pull_request.commits}})\n+\n+ if test -n \"${log}\"\n+ then\n+ echo \"::set-output name=checkout::\"${log}\"\"\n+ exit 2\n+ fi\n+\n+ - name: Add Check Output as Comment\n+ uses: actions/github-script@v3\n+ id: add-comment\n+ with:\n+ script: |\n+ github.issues.createComment({\n+ issue_number: context.issue.number,\n+ owner: context.repo.owner,\n+ repo: context.repo.repo,\n+ body: \"Whitespace errors found in workflow ${{ github.workflow }}:\\n\\n${{ steps.check_out.outputs.checkout }}\"\n+ })\n+ if: ${{ failure() }}\n\nbase-commit: 675a4aaf3b226c0089108221b96559e0baae5de9\n-- \ngitgitgadget\n"},{"id":"406134","messageId":"20200922170745.GA541915@coredump.intra.peff.net","threadId":"54271","inReplyTo":"pull.709.git.1600759684548.gitgitgadget@gmail.com","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-22T17:07:45Z","receivedAt":"2020-09-22T17:07:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 22, 2020 at 07:28:04AM +0000, Chris. Webster via GitGitGadget wrote:\n\n> From: \"Chris. Webster\" \n> \n> Not all developers are aware of `git diff --check` to warn\n> about whitespace issues. Running a check when a pull request is\n> opened or updated can save time for reviewers and the submitter.\n\nSounds like a useful thing to have.\n\n> A GitHub workflow will run when a pull request is created or the\n> contents are updated to check the patch series. A pull request\n> provides the necessary information (number of commits) to only\n> check the patch series.\n\nI think this will work OK in practice, but a few thoughts:\n\n - for a linear branch on top of master, using the commit count will\n work reliably. But I suspect it would run into problems if there were\n ever a merge on a PR (e.g., back-merging from master), where we'd be\n subject to how `git log` linearizes the commits. That's not really a\n workflow I'd expect people to use with git.git, but it would probably\n be easy to make it more robust. Does the PR object provide the \"base\"\n oid, so we could do \"git log $base..$head\"?\n\n - this will run only on PRs. That's helpful for people using\n GitGitGadget, but it might also be useful for people just running the\n CI by pushing branches, or looking at CI builds of Junio's next or\n seen branches. Could we make it work there? Obviously we wouldn't be\n able to rely on having PR data, but I wonder if \"git log\n HEAD..$branch\" would be sufficient.\n\n-Peff\n"},{"id":"406135","messageId":"xmqq1ritlmrk.fsf@gitster.c.googlers.com","threadId":"54271","inReplyTo":"20200922170745.GA541915@coredump.intra.peff.net","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-22T17:55:11Z","receivedAt":"2020-09-22T17:55:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> - this will run only on PRs. That's helpful for people using\n> GitGitGadget, but it might also be useful for people just running the\n> CI by pushing branches, or looking at CI builds of Junio's next or\n> seen branches. Could we make it work there? Obviously we wouldn't be\n> able to rely on having PR data, but I wonder if \"git log\n> HEAD..$branch\" would be sufficient.\n\nYes, I like that very much. If a push triggers a CI run to notice\nwhitespace breakage and other mechanically detectable errors, that\nwould prevent embarrassment before even a pull request is opened.\n\nFor me, 'next' is way too late to catch mechanically detectable\nerrors, but an extra set of eyes, even mechanical ones, on the tip\nof 'seen' is always appreciated.\n\nThanks.\n"},{"id":"406140","messageId":"CAGT1KpVmeT+nT1-Pfwa_M8BptFYwRTL4ofM0k6UOOzkYh0kucw@mail.gmail.com","threadId":"54271","inReplyTo":"20200922170745.GA541915@coredump.intra.peff.net","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Chris Webster","fromEmail":"chris@webstech.net","sentAt":"2020-09-22T22:17:54Z","receivedAt":"2020-09-22T22:17:43Z","isPatch":true,"sender":{"key":"chris@webstech.net","avatar":"https://avatars.githubusercontent.com/u/7956947?v=4"},"body":"On Tue, Sep 22, 2020 at 10:07 AM Jeff King wrote:\n> - for a linear branch on top of master, using the commit count will\n> work reliably. But I suspect it would run into problems if there were\n> ever a merge on a PR (e.g., back-merging from master), where we'd be\n> subject to how `git log` linearizes the commits. That's not really a\n> workflow I'd expect people to use with git.git, but it would probably\n> be easy to make it more robust. Does the PR object provide the \"base\"\n> oid, so we could do \"git log $base..$head\"?\n\nGitGitGadget PR linting is going to flag merges in the PR and request\na rebase. If I understand correctly, that means back-merging is not\npart of the workflow. The checkout is limited to improve performance\nand reduce resources. In the PR object, the base is the branch. The\ngithub api would need to be used to get more detailed information.\nThe \"base\" is not really part of the checkout so it can not be\nreferenced in the git log command (without doing a larger checkout).\n\n...chris.\n"},{"id":"406141","messageId":"CAGT1KpU4Kjv2PEAA7-bNbGp2DFvfsKqABuUK68128xkLjdcEhA@mail.gmail.com","threadId":"54271","inReplyTo":"xmqq1ritlmrk.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Chris Webster","fromEmail":"chris@webstech.net","sentAt":"2020-09-22T22:41:57Z","receivedAt":"2020-09-22T22:41:43Z","isPatch":true,"sender":{"key":"chris@webstech.net","avatar":"https://avatars.githubusercontent.com/u/7956947?v=4"},"body":"On Tue, Sep 22, 2020 at 10:55 AM Junio C Hamano wrote:\n>\n> Jeff King writes:\n>\n> > - this will run only on PRs. That's helpful for people using\n> > GitGitGadget, but it might also be useful for people just running the\n> > CI by pushing branches, or looking at CI builds of Junio's next or\n> > seen branches. Could we make it work there? Obviously we wouldn't be\n> > able to rely on having PR data, but I wonder if \"git log\n> > HEAD..$branch\" would be sufficient.\n>\n> Yes, I like that very much. If a push triggers a CI run to notice\n> whitespace breakage and other mechanically detectable errors, that\n> would prevent embarrassment before even a pull request is opened.\n\nThis was originally started as a push action which was expected to be\nin the GitGitGadget workflow. It was changed to run on PRs using the\nmore helpful PR data. The original GitGitGadget issue suggested a\nbuild target to run the check, which could have been part of the CI\nbuild (or a local build). Doing the check later as part of the PR\nprocess is consistent with GitGitGadget performing linting on the PR\nrequest, with similar opportunities for embarrassment.\n\n...chris.\n"},{"id":"406240","messageId":"20200924065129.GB1851751@coredump.intra.peff.net","threadId":"54271","inReplyTo":"CAGT1KpVmeT+nT1-Pfwa_M8BptFYwRTL4ofM0k6UOOzkYh0kucw@mail.gmail.com","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-24T06:51:29Z","receivedAt":"2020-09-24T06:51:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 22, 2020 at 03:17:54PM -0700, Chris Webster wrote:\n\n> On Tue, Sep 22, 2020 at 10:07 AM Jeff King wrote:\n> > - for a linear branch on top of master, using the commit count will\n> > work reliably. But I suspect it would run into problems if there were\n> > ever a merge on a PR (e.g., back-merging from master), where we'd be\n> > subject to how `git log` linearizes the commits. That's not really a\n> > workflow I'd expect people to use with git.git, but it would probably\n> > be easy to make it more robust. Does the PR object provide the \"base\"\n> > oid, so we could do \"git log $base..$head\"?\n> \n> GitGitGadget PR linting is going to flag merges in the PR and request\n> a rebase. If I understand correctly, that means back-merging is not\n> part of the workflow.\n\nYeah, I would definitely be surprised to see it used with a git PR, but\nI didn't realize there was other linting that would actually complain\nabout it.\n\n> The checkout is limited to improve performance\n> and reduce resources. In the PR object, the base is the branch. The\n> github api would need to be used to get more detailed information.\n> The \"base\" is not really part of the checkout so it can not be\n> referenced in the git log command (without doing a larger checkout).\n\nHmm.\n\n git clone --shallow-exclude=HEAD --single-branch -b $branch\n git log --check\n\n_almost_ works. The problem is that the shallow graft means that the\nbottom commit looks like it introduces every file. We really want to\ngraft at HEAD^, but the server side only accepts exact refnames. You\ncould work around it with a followup:\n\n git fetch --deepen 1\n\nwhich is getting a bit convoluted. I suspect you may also have to\nabandon the \"checkout\" action and do this manually. Definitely not worth\nit compared to your solution for a PR, but maybe worth it if it lets us\ndo the same thing for arbitrary branches.\n\n-Peff\n"},{"id":"406296","messageId":"CAGT1KpU6zkvr9QQQqez6x1cVsvhxmkU9gG5T0QYC8yCgnOPN1Q@mail.gmail.com","threadId":"54271","inReplyTo":"20200924065129.GB1851751@coredump.intra.peff.net","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Chris Webster","fromEmail":"chris@webstech.net","sentAt":"2020-09-25T05:10:16Z","receivedAt":"2020-09-25T05:10:11Z","isPatch":true,"sender":{"key":"chris@webstech.net","avatar":"https://avatars.githubusercontent.com/u/7956947?v=4"},"body":"On Wed, Sep 23, 2020 at 11:51 PM Jeff King wrote:\n> git clone --shallow-exclude=HEAD --single-branch -b $branch\n> git log --check\n>\n> _almost_ works. The problem is that the shallow graft means that the\n> bottom commit looks like it introduces every file. We really want to\n> graft at HEAD^, but the server side only accepts exact refnames. You\n> could work around it with a followup:\n>\n> git fetch --deepen 1\n\nThanks for the other possibilities (I have much to learn). The first\nstep to increase the commit count is addressing this very problem.\n\n> Definitely not worth\n> it compared to your solution for a PR, but maybe worth it if it lets us\n> do the same thing for arbitrary branches.\n\nThe PR solution works because fixed values are available from GitHub\n(both repos are present and accounted for). A push action for\nbranches could have issues with the state of the GitHub repo versus\nthe local repo. What happens if the base branch is not current on\nGitHub? Is HEAD reliable? What if the branch has been re-used with a\nback-merge? How do you limit the check in this case? Based on my\ndemonstrated lack of knowledge these concerns may be addressable.\n\nThe original push solution pulled an arbitrary depth knowing\nGitGitGadget had a limit of 30 commits and then limited the check to\npost merge commits, again knowing merges were flagged. Not pretty but\nworkable in the confines of the GitGitGadget workflow.\n\nA generic push solution (with an opt out?) could be a separate file or\nreplace this (yea).\n\nI appreciate the feedback,\n...chris.\n"},{"id":"406304","messageId":"20200925064459.GA3179383@coredump.intra.peff.net","threadId":"54271","inReplyTo":"CAGT1KpU6zkvr9QQQqez6x1cVsvhxmkU9gG5T0QYC8yCgnOPN1Q@mail.gmail.com","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-25T06:44:59Z","receivedAt":"2020-09-25T06:45:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 24, 2020 at 10:10:16PM -0700, Chris Webster wrote:\n\n> > Definitely not worth\n> > it compared to your solution for a PR, but maybe worth it if it lets us\n> > do the same thing for arbitrary branches.\n> \n> The PR solution works because fixed values are available from GitHub\n> (both repos are present and accounted for). A push action for\n> branches could have issues with the state of the GitHub repo versus\n> the local repo. What happens if the base branch is not current on\n> GitHub? Is HEAD reliable? What if the branch has been re-used with a\n> back-merge? How do you limit the check in this case? Based on my\n> demonstrated lack of knowledge these concerns may be addressable.\n\nHmm, good points. The case I was most worried about was branches based\non older points in history, but as long as master keeps moving forward,\nwe'd be OK there (at least in the local case where we have all of the\ncommits; not sure about the shallow-exclude I mentioned above).\n\nAnd in the case of git.git, I think we're pretty safe. \"master\" gets\npushed along with \"seen\". But not necessarily so in other repositories.\nIf I base a new topic on Junio's \"master\" and then push it up, it may be\nfar ahead of my \"master\" (and in fact, I don't even have a \"master\" in\nmy personal repo).\n\nGGG PRs figure this out because that repo is a fork of git/git, and it\nlooks at the master of the parent repo as the base for the PR. So\nprobably we could do something similar, but this is starting to get\nrather tricky.\n\nI think you've convinced me that it's not easy to just adapt this to\nhandle any branch. Let's punt on that idea for now (unless somebody\nfeels like digging further on it, of course) and move forward with doing\nthis for the PR case as your patch does.\n\n-Peff\n"},{"id":"407196","messageId":"CAGT1KpXz4nFBu2xkVSaoW4DgXc_5oB69MQRQW=365gfgd_R-mQ@mail.gmail.com","threadId":"54271","inReplyTo":"CAGT1KpU4Kjv2PEAA7-bNbGp2DFvfsKqABuUK68128xkLjdcEhA@mail.gmail.com","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Chris Webster","fromEmail":"chris@webstech.net","sentAt":"2020-10-09T05:00:25Z","receivedAt":"2020-10-09T05:00:32Z","isPatch":true,"sender":{"key":"chris@webstech.net","avatar":"https://avatars.githubusercontent.com/u/7956947?v=4"},"body":"Is this waiting for some action on my part? I thought the question of\nrunning on push vs pull had been resolved (in favour of pull).\n\nthanks,\n...chris.\n"},{"id":"407212","messageId":"nycvar.QRO.7.76.6.2010091519460.50@tvgsbejvaqbjf.bet","threadId":"54271","inReplyTo":"CAGT1KpXz4nFBu2xkVSaoW4DgXc_5oB69MQRQW=365gfgd_R-mQ@mail.gmail.com","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-09T13:20:35Z","receivedAt":"2020-10-09T15:38:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Thu, 8 Oct 2020, Chris Webster wrote:\n\n> Is this waiting for some action on my part? I thought the question of\n> running on push vs pull had been resolved (in favour of pull).\n\nFWIW I agree that the current shape is the best we can do for now (and of\ncourse, full disclosure: I was the one suggesting to restrict this to Pull\nRequests because we know exactly the commit range to check in that case).\n\nThanks,\nDscho\n"},{"id":"407218","messageId":"xmqqtuv3tlkv.fsf@gitster.c.googlers.com","threadId":"54271","inReplyTo":"nycvar.QRO.7.76.6.2010091519460.50@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-09T16:23:28Z","receivedAt":"2020-10-09T16:23:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Chris,\n>\n> On Thu, 8 Oct 2020, Chris Webster wrote:\n>\n>> Is this waiting for some action on my part? I thought the question of\n>> running on push vs pull had been resolved (in favour of pull).\n>\n> FWIW I agree that the current shape is the best we can do for now (and of\n> course, full disclosure: I was the one suggesting to restrict this to Pull\n> Requests because we know exactly the commit range to check in that case).\n\nI think this is exactly the use case that\n\n After the list reached a consensus that it is a good idea to apply the\n patch, re-send it with \"To:\" set to the maintainer{current-maintainer}\n and \"cc:\" the list{git-ml} for inclusion.\n\nin Documentation/SubmittingPatches was written to address.\n\nI usually pay attention to majority of topics and have them on my\nradar by getting involved in _some_ way in the discussion thread, so\nI often know when the patch(es) matured enough to be picked up\nwithout such a \"this is the version after our discussion and it is\nas close to perfect as we can possibly make\" resend.\n\nBut for some topics, I have no strong opinion on the exact shape of\nthe final patch(es), and/or I have no expertise to offer to help the\ndiscussion to reach the final product. In such a case, I'd be just\nwaiting, without getting involved in the discussion, for trusted\nothers to bring the posted patch to a completed form. I think this\nis such a case.\n\nThanks.\n\n\n\n\n"},{"id":"407227","messageId":"20201009175917.GA963340@coredump.intra.peff.net","threadId":"54271","inReplyTo":"xmqqtuv3tlkv.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-09T17:59:17Z","receivedAt":"2020-10-09T17:59:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 09, 2020 at 09:23:28AM -0700, Junio C Hamano wrote:\n\n> I think this is exactly the use case that\n> \n> After the list reached a consensus that it is a good idea to apply the\n> patch, re-send it with \"To:\" set to the maintainer{current-maintainer}\n> and \"cc:\" the list{git-ml} for inclusion.\n> \n> in Documentation/SubmittingPatches was written to address.\n> \n> I usually pay attention to majority of topics and have them on my\n> radar by getting involved in _some_ way in the discussion thread, so\n> I often know when the patch(es) matured enough to be picked up\n> without such a \"this is the version after our discussion and it is\n> as close to perfect as we can possibly make\" resend.\n> \n> But for some topics, I have no strong opinion on the exact shape of\n> the final patch(es), and/or I have no expertise to offer to help the\n> discussion to reach the final product. In such a case, I'd be just\n> waiting, without getting involved in the discussion, for trusted\n> others to bring the posted patch to a completed form. I think this\n> is such a case.\n\nAs the other person in the discussion, I'm sufficiently convinced that\ndoing this just for PRs is a good step for now. I.e., I think the\n\"completed form\" is just what was posted already (though I agree it is\noften convenient to the maintainer to re-post the patch as part of the\nping).\n\n-Peff\n"},{"id":"407228","messageId":"xmqqeem7tgh4.fsf@gitster.c.googlers.com","threadId":"54271","inReplyTo":"20201009175917.GA963340@coredump.intra.peff.net","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-09T18:13:43Z","receivedAt":"2020-10-09T18:13:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Oct 09, 2020 at 09:23:28AM -0700, Junio C Hamano wrote:\n>\n>> I think this is exactly the use case that\n>> \n>> After the list reached a consensus that it is a good idea to apply the\n>> patch, re-send it with \"To:\" set to the maintainer{current-maintainer}\n>> and \"cc:\" the list{git-ml} for inclusion.\n>> \n>> in Documentation/SubmittingPatches was written to address.\n>> \n>> I usually pay attention to majority of topics and have them on my\n>> radar by getting involved in _some_ way in the discussion thread, so\n>> I often know when the patch(es) matured enough to be picked up\n>> without such a \"this is the version after our discussion and it is\n>> as close to perfect as we can possibly make\" resend.\n>> \n>> But for some topics, I have no strong opinion on the exact shape of\n>> the final patch(es), and/or I have no expertise to offer to help the\n>> discussion to reach the final product. In such a case, I'd be just\n>> waiting, without getting involved in the discussion, for trusted\n>> others to bring the posted patch to a completed form. I think this\n>> is such a case.\n>\n> As the other person in the discussion, I'm sufficiently convinced that\n> doing this just for PRs is a good step for now. I.e., I think the\n> \"completed form\" is just what was posted already (though I agree it is\n> often convenient to the maintainer to re-post the patch as part of the\n> ping).\n\nYes, and CC'ing those who were involved in the review would give\nthem the last chance to say \"oh, no, that extra change you added\nfor this final submission was not something I meant to suggest!\",\netc.\n\nSo, is as-is the\none we should take?\n\nThanks.\n\n"},{"id":"407229","messageId":"20201009181827.GA965760@coredump.intra.peff.net","threadId":"54271","inReplyTo":"xmqqeem7tgh4.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-09T18:18:27Z","receivedAt":"2020-10-09T18:18:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 09, 2020 at 11:13:43AM -0700, Junio C Hamano wrote:\n\n> > As the other person in the discussion, I'm sufficiently convinced that\n> > doing this just for PRs is a good step for now. I.e., I think the\n> > \"completed form\" is just what was posted already (though I agree it is\n> > often convenient to the maintainer to re-post the patch as part of the\n> > ping).\n> \n> Yes, and CC'ing those who were involved in the review would give\n> them the last chance to say \"oh, no, that extra change you added\n> for this final submission was not something I meant to suggest!\",\n> etc.\n> \n> So, is as-is the\n> one we should take?\n\nAFAIK it's the only one on the list. :) So yes, that one is fine with\nme.\n\n-Peff\n"},{"id":"407234","messageId":"xmqq8scfteh7.fsf@gitster.c.googlers.com","threadId":"54271","inReplyTo":"20201009181827.GA965760@coredump.intra.peff.net","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-09T18:56:52Z","receivedAt":"2020-10-09T18:57:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Oct 09, 2020 at 11:13:43AM -0700, Junio C Hamano wrote:\n>\n>> > As the other person in the discussion, I'm sufficiently convinced that\n>> > doing this just for PRs is a good step for now. I.e., I think the\n>> > \"completed form\" is just what was posted already (though I agree it is\n>> > often convenient to the maintainer to re-post the patch as part of the\n>> > ping).\n>> \n>> Yes, and CC'ing those who were involved in the review would give\n>> them the last chance to say \"oh, no, that extra change you added\n>> for this final submission was not something I meant to suggest!\",\n>> etc.\n>> \n>> So, is as-is the\n>> one we should take?\n>\n> AFAIK it's the only one on the list. :) So yes, that one is fine with\n> me.\n\nThanks.\n\nAnother thing the resending does is that it can credit who helped\nthe patch into the final shape with Reviewed-by/Helped-by etc. If\nthe maintainer must hunt for the names of those who had input to the\ndiscussion and judge the degree of contribution for a topic whose\nreview has been delegated to trusted others, that defeats the whole\npoint of delegation (I think the attached clarification may help).\n\nFor this particular patch, I added Reviewed-by: naming you before\napplying.\n\nThanks.\n\n Documentation/SubmittingPatches | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches\nindex 291b61e262..87089654ae 100644\n--- c/Documentation/SubmittingPatches\n+++ w/Documentation/SubmittingPatches\n@@ -290,12 +290,14 @@ identify them), to solicit comments and reviews.\n :git-ml: footnote:[The mailing list: git@vger.kernel.org]\n \n After the list reached a consensus that it is a good idea to apply the\n-patch, re-send it with \"To:\" set to the maintainer{current-maintainer} and \"cc:\" the\n-list{git-ml} for inclusion.\n+patch, re-send it with \"To:\" set to the maintainer{current-maintainer}\n+and \"cc:\" the list{git-ml} for inclusion. This is especially relevant\n+when the maintainer did not heavily participate in the discussion and\n+instead left the review to trusted others.\n \n Do not forget to add trailers such as `Acked-by:`, `Reviewed-by:` and\n `Tested-by:` lines as necessary to credit people who helped your\n-patch.\n+patch, and \"cc:\" them when sending such a final version for inclusion.\n \n [[sign-off]]\n === Certify your work by adding your \"Signed-off-by: \" line\n"},{"id":"407259","messageId":"CAGT1KpV+vcD59W2qWBsgg2qfSSLaJ37aVi__y5u=wHjsSDiiOQ@mail.gmail.com","threadId":"54271","inReplyTo":"xmqq8scfteh7.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Chris Webster","fromEmail":"chris@webstech.net","sentAt":"2020-10-10T05:26:35Z","receivedAt":"2020-10-10T05:27:12Z","isPatch":true,"sender":{"key":"chris@webstech.net","avatar":"https://avatars.githubusercontent.com/u/7956947?v=4"},"body":"On Fri, Oct 9, 2020 at 11:57 AM Junio C Hamano wrote:\n\n> Thanks.\n>\n> Another thing the resending does is that it can credit who helped\n> the patch into the final shape with Reviewed-by/Helped-by etc. If\n> the maintainer must hunt for the names of those who had input to the\n> discussion and judge the degree of contribution for a topic whose\n> review has been delegated to trusted others, that defeats the whole\n> point of delegation (I think the attached clarification may help).\n>\n> For this particular patch, I added Reviewed-by: naming you before\n> applying.\n>\n> Thanks.\n>\n\nThank you for moving forward with this. I apologize for not\nre-reviewing the SubmittingPatches doc. I should have done that.\n\nThanks to Johannes for the input on this before it was submitted.\nWill work on improving the commit messages with more credits.\n\nIs there an opportunity here for a gitgitgadget command to send the\n'consensus reached' email? The value may be in deciding who is\ngetting the email and trying to select content from the PR comments.\n\n...chris.\n"},{"id":"407260","messageId":"xmqq7drysiei.fsf@gitster.c.googlers.com","threadId":"54271","inReplyTo":"CAGT1KpV+vcD59W2qWBsgg2qfSSLaJ37aVi__y5u=wHjsSDiiOQ@mail.gmail.com","subject":"Re: [PATCH] ci: github action - add check for whitespace errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-10T06:29:41Z","receivedAt":"2020-10-10T06:30:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Webster writes:\n\n> Thank you for moving forward with this. I apologize for not\n> re-reviewing the SubmittingPatches doc. I should have done that.\n>\n> Thanks to Johannes for the input on this before it was submitted.\n> Will work on improving the commit messages with more credits.\n\nThank you to all, and especially to you Chris for pinging to revive\nthe thread. Applied and pushed out.\n\n> Is there an opportunity here for a gitgitgadget command to send the\n> 'consensus reached' email? The value may be in deciding who is\n> getting the email and trying to select content from the PR comments.\n"}]}