{"thread":{"id":"56888","subject":"[PATCH] ci(check-whitespace): update stale file top comments","startedAt":"2021-11-13T17:00:43Z","lastAt":"2021-11-17T06:59:14Z","messageCount":3,"participants":["hakre via GitGitGadget","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"441069","messageId":"pull.1138.git.git.1636822837587.gitgitgadget@gmail.com","threadId":"56888","inReplyTo":null,"subject":"[PATCH] ci(check-whitespace): update stale file top comments","fromName":"hakre via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-13T17:00:37Z","receivedAt":"2021-11-13T17:00:43Z","isPatch":true,"sender":{"key":"name:hakre","avatar":null},"body":"From: hakre <hanskrentel@yahoo.de>\n\nPart of these two recent commits\n\n1. a066a90db6 (ci(check-whitespace): restrict to the intended commits,\n   2021-07-15)\n2. cc00362125 (ci(check-whitespace): stop requiring a read/write token,\n   2021-07-15)\n\nare well written messages that reflect the changes (compare: [1]).\n\nUnfortunately those commits left the description in top file comments\nunchanged which are still showing the previous picture.\n\nTo better display the current workflow upfront, those comments now\nreflect that:\n\n1. full (not shallow) clone to steadily check the intended commits\n2. communicated result is the exit status (not a comment in the PR)\n\n[1]: https://git-scm.com/docs/SubmittingPatches#describe-changes\nCC: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: hakre <hanskrentel@yahoo.de>\n---\n    ci(check-whitespace): update stale file top comments\n    \n    Part of these two recent commits\n    \n     1. a066a90db6 (ci(check-whitespace): restrict to the intended commits,\n        2021-07-15)\n     2. cc00362125 (ci(check-whitespace): stop requiring a read/write token,\n        2021-07-15)\n    \n    are well written messages that reflect the changes (compare: 1\n    [https://git-scm.com/docs/SubmittingPatches#describe-changes]).\n    \n    Unfortunately those commits left the description in top file comments\n    unchanged which are still showing the previous picture.\n    \n    To better display the current workflow upfront, those comments now\n    reflect that:\n    \n     1. full (not shallow) clone to steadily check the intended commits\n     2. communicated result is the exit status (not a comment in the PR)\n    \n    Signed-off-by: hakre hanskrentel@yahoo.de\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1138%2Fhakre%2Fpatch-1-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1138/hakre/patch-1-v1\nPull-Request: https://github.com/git/git/pull/1138\n\n .github/workflows/check-whitespace.yml | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml\nindex 8c4358d805c..2dce03bc479 100644\n--- a/.github/workflows/check-whitespace.yml\n+++ b/.github/workflows/check-whitespace.yml\n@@ -1,8 +1,8 @@\n name: check-whitespace\n \n-# Get the repo with the commits(+1) in the series.\n+# Get the repo with all commits to steady catch 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+# Give status 2 on check errors.\n \n on:\n   pull_request:\n\nbase-commit: 5fbd2fc5997dfa4d4593a862fe729b1e7a89bcf8\n-- \ngitgitgadget\n"},{"id":"441322","messageId":"nycvar.QRO.7.76.6.2111161321380.21127@tvgsbejvaqbjf.bet","threadId":"56888","inReplyTo":"pull.1138.git.git.1636822837587.gitgitgadget@gmail.com","subject":"Re: [PATCH] ci(check-whitespace): update stale file top comments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-16T12:26:26Z","receivedAt":"2021-11-16T12:26:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hans,\n\nOn Sat, 13 Nov 2021, hakre via GitGitGadget wrote:\n\n> From: hakre <hanskrentel@yahoo.de>\n\nAs per https://git-scm.com/docs/SubmittingPatches#sign-off:\n\n\tPlease don’t hide your real name.\n\nI strongly suspect your real name to be Hans Krentel, not hakre.\n\n> Part of these two recent commits\n>\n> 1. a066a90db6 (ci(check-whitespace): restrict to the intended commits,\n>    2021-07-15)\n> 2. cc00362125 (ci(check-whitespace): stop requiring a read/write token,\n>    2021-07-15)\n>\n> are well written messages that reflect the changes (compare: [1]).\n>\n> Unfortunately those commits left the description in top file comments\n> unchanged which are still showing the previous picture.\n>\n> To better display the current workflow upfront, those comments now\n> reflect that:\n>\n> 1. full (not shallow) clone to steadily check the intended commits\n> 2. communicated result is the exit status (not a comment in the PR)\n>\n> [1]: https://git-scm.com/docs/SubmittingPatches#describe-changes\n> CC: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Signed-off-by: hakre <hanskrentel@yahoo.de>\n> ---\n>     ci(check-whitespace): update stale file top comments\n>\n>     Part of these two recent commits\n>\n>      1. a066a90db6 (ci(check-whitespace): restrict to the intended commits,\n>         2021-07-15)\n>      2. cc00362125 (ci(check-whitespace): stop requiring a read/write token,\n>         2021-07-15)\n>\n>     are well written messages that reflect the changes (compare: 1\n>     [https://git-scm.com/docs/SubmittingPatches#describe-changes]).\n>\n>     Unfortunately those commits left the description in top file comments\n>     unchanged which are still showing the previous picture.\n>\n>     To better display the current workflow upfront, those comments now\n>     reflect that:\n>\n>      1. full (not shallow) clone to steadily check the intended commits\n>      2. communicated result is the exit status (not a comment in the PR)\n>\n>     Signed-off-by: hakre hanskrentel@yahoo.de\n\nIf you send a new iteration, please replace the first comment on your PR\nby a cover letter. You can also delete the comment's contents instead.\n\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1138%2Fhakre%2Fpatch-1-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1138/hakre/patch-1-v1\n> Pull-Request: https://github.com/git/git/pull/1138\n>\n>  .github/workflows/check-whitespace.yml | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml\n> index 8c4358d805c..2dce03bc479 100644\n> --- a/.github/workflows/check-whitespace.yml\n> +++ b/.github/workflows/check-whitespace.yml\n> @@ -1,8 +1,8 @@\n>  name: check-whitespace\n>\n> -# Get the repo with the commits(+1) in the series.\n> +# Get the repo with all commits to steady catch the series.\n\nI am not a native English speaker, but \"to steady catch\" strikes me as not\nquite English. I would suggest something like this instead:\n\n\tGet the repository with all commits to ensure that we can analyze\n\tall of the commits contributed via the Pull Request.\n\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> +# Give status 2 on check errors.\n\nIs it really interesting that the exit code 2 is used? Or is it more\ninteresting that the job will exit with failure if the check produces\nerrors? I think it would sound better as:\n\n\tExit with failure upon white-space issues.\n\nCiao,\nJohannes\n\n>\n>  on:\n>    pull_request:\n>\n> base-commit: 5fbd2fc5997dfa4d4593a862fe729b1e7a89bcf8\n> --\n> gitgitgadget\n>\n"},{"id":"441405","messageId":"xmqq7dd75kub.fsf@gitster.g","threadId":"56888","inReplyTo":"nycvar.QRO.7.76.6.2111161321380.21127@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] ci(check-whitespace): update stale file top comments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-17T06:59:08Z","receivedAt":"2021-11-17T06:59:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi Hans,\n>\n> On Sat, 13 Nov 2021, hakre via GitGitGadget wrote:\n>\n>> From: hakre <hanskrentel@yahoo.de>\n>\n> As per https://git-scm.com/docs/SubmittingPatches#sign-off:\n>\n> \tPlease don’t hide your real name.\n>\n> I strongly suspect your real name to be Hans Krentel, not hakre.\n\nThanks for pointing it out.  I have a feeling that we see more\nsubmissions with this problem via GGG than other avenues, and I do\nnot want to be the only one who is enforcing that rule.  Very much\nappreciated.\n\n>> Part of these two recent commits\n>>\n>> 1. a066a90db6 (ci(check-whitespace): restrict to the intended commits,\n>>    2021-07-15)\n>> 2. cc00362125 (ci(check-whitespace): stop requiring a read/write token,\n>>    2021-07-15)\n>>\n>> are well written messages that reflect the changes (compare: [1]).\n\nThe above may not be incorrect per-se, but other than the fact that\nwhat this patch fixes came from these two commits, I find it largely\nirrelevant.\n\n>> Unfortunately those commits left the description in top file comments\n>> unchanged which are still showing the previous picture.\n\n    Earlier a066a90d (ci(check-whitespace): restrict to the intended\n    commits, 2021-07-14) changed the check-whitespace task to stop\n    using a shallow clone, and cc003621 (ci(check-whitespace): stop\n    requiring a read/write token, 2021-07-14) changed the way how\n    the errors the task discovered is signaled back to the user.\n\n    They however forgot to update the comment that outlines what is\n    done in the task.  Correct them.\n\nwould perhaps be sufficient?\n\n>>  name: check-whitespace\n>>\n>> -# Get the repo with the commits(+1) in the series.\n>> +# Get the repo with all commits to steady catch the series.\n>\n> I am not a native English speaker, but \"to steady catch\" strikes me as not\n> quite English. I would suggest something like this instead:\n>\n> \tGet the repository with all commits to ensure that we can analyze\n> \tall of the commits contributed via the Pull Request.\n\nSounds good (I am not native, either).\n\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>> +# Give status 2 on check errors.\n>\n> Is it really interesting that the exit code 2 is used? Or is it more\n> interesting that the job will exit with failure if the check produces\n> errors? I think it would sound better as:\n>\n> \tExit with failure upon white-space issues.\n\nSounds fine, too.\n\nOr \"exit with status 2 to signal white-space issues\" if we really\nwanted to say \"2\" somewhere, but I do not think it is needed,\nbecause I do not offhand see other non-zero exits in the script.\n\nThanks.\n\n>\n> Ciao,\n> Johannes\n>\n>>\n>>  on:\n>>    pull_request:\n>>\n>> base-commit: 5fbd2fc5997dfa4d4593a862fe729b1e7a89bcf8\n>> --\n>> gitgitgadget\n>>\n"}]}