Make check-whitespace failures more helpful

[webstech]

Add the errors to the job summary along with suggested commands to fix the problem. The commits and filenames are links.

This is for issue #1395. Sample job output:

❌ A whitespace issue was found in one or more of the commits.

Run these commands to correct the problem:

1. git rebase --whitespace=fix aaa04a9
2. git push --force

Errors:

1. --- 5cd37f6 Remove annotations
   trailing.txt:4: trailing whitespace.
   +
   trailing.txt:2: new blank line at EOF.

cc: Đoàn Trần Công Danh
congdanhqx@gmail.com

[webstech]

/preview

[gitgitgadget]

Preview email sent as pull.1444.git.1670978180450.gitgitgadget@gmail.com

[webstech]

@dscho Would you like any changes to the wording before I submit this?

[dscho]

Thank you so much for working on this @webstech!

I just left a couple comments, hopefully they are helpful.

[gitgitgadget]

There are issues in commit 97300b4:
Improve check-whitespace output
Commit not signed off

[webstech]

Commit not signed off

Have I missed finding a config option to always sign off commits in a repo?

[dscho]

This will be so nice for contributors who run into white-space problems!

[webstech]

/submit

[gitgitgadget]

Submitted as pull.1444.git.1671179520.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1444/webstech/whitespace-v1

To fetch this version to local tag pr-1444/webstech/whitespace-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1444/webstech/whitespace-v1

[dscho]

Commit not signed off

Have I missed finding a config option to always sign off commits in a repo?

Sadly, no. There is indeed no config option for that yet.

[webstech]

@dscho fyi, there are missing reply emails to the /submit. I am looking into it but it may be more obvious to you. Wanted to get this corrected before replying to the emails and resubmitting this. The two jobs processing the emails had errors but did not fail (guess I will open an issue for that). A search hit on the error says it can happen if the commit is missing or incorrect on the request.

Jobs in run #164801

Handling commit range 169963b92048bda2e7174480de66fdcc344a0c56..1915614b0caf9410edd6a9f6ee2169d0aa1818a2
Message-ID xmqqo7s3mzlt.fsf@gitster.g (length 5962) for PR https://github.com/gitgitgadget/git/pull/1444, commit 67f60e4e5cbb470bbf3f556f962403af5dd5938c, comment ID: undefined
HttpError: Validation Failed: {"resource":"PullRequestReviewComment","code":"custom","field":"pull_request_review_thread.line","message":"pull_request_review_thread.line must be part of the diff"}, {"resource":"PullRequestReviewComment","code":"missing_field","field":"pull_request_review_thread.diff_hunk"}: skipping

Jobs in run #164803

Handling commit range 1915614b0caf9410edd6a9f6ee2169d0aa1818a2..02c40851755370245718165d7fd6781980748a03
Message-ID xmqqh6xvmzb8.fsf@gitster.g (length 5409) for PR https://github.com/gitgitgadget/git/pull/1444, commit cdc2b1aae81f8c37b4e71cb3e0e382cf82de2272, comment ID: undefined
HttpError: Validation Failed: {"resource":"PullRequestReviewComment","code":"custom","field":"pull_request_review_thread.line","message":"pull_request_review_thread.line must be part of the diff"}, {"resource":"PullRequestReviewComment","code":"missing_field","field":"pull_request_review_thread.diff_hunk"}: skipping

[webstech]

Wanted to get this corrected

But I may have made that impossible with the push I did yesterday. Sorry about that.

[dscho]

{
  "resource":"PullRequestReviewComment",
  "code":"custom",
  "field":"pull_request_review_thread.line",
  "message":"pull_request_review_thread.line must be part of the diff"
}, {
  "resource":"PullRequestReviewComment",
  "code":"missing_field",
  "field":"pull_request_review_thread.diff_hunk"
}

Hmm. It would appear that the line attribute is now required in https://docs.github.com/en/rest/pulls/comments?apiVersion=2022-11-28#create-a-review-comment-for-a-pull-request (and the diff_hunk message might just be a secondary effect of the missing line attribute).

Having said that, it looks as if we already specify the line, hard-coding it to 1?

[dscho]

So, I just experimented a bit with the REST API calls and it would appear that the API got a lot stricter. Not only does the commit have to be part of the PR, the side is apparently now also mandatory, and the line must be part of the diff.

To prove that that works, I hard-coded a few values and created a2b5f3e#discussion_r1052695200 (which is bogus, of course, as the comments target a different commit, one that is no longer part of the PR's commit range).

@webstech honestly, I do not really know yet how to proceed from here.

[webstech]

the line must be part of the diff

It worked when I changed position to line but they could have 'fixed' it. Isn't it still working on git/git PRs?

My initial thought is to grab the line number from the /submit email in PatchSeries and save it in the IMailMetadata notes.

[webstech]

/submit

[gitgitgadget]

Submitted as pull.1444.v2.git.1671496548.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1444/webstech/whitespace-v2

To fetch this version to local tag pr-1444/webstech/whitespace-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1444/webstech/whitespace-v2

[webstech]

Isn't it still working on git/git PRs?

From job 165116, git/git notes do not have the commit?

Handling commit range 5e50a029e045dc35a46487997c8b9d95fc092c8a..d34a04ac1aadce71d5fdc77e7777adce689ba86f
Message-ID 221219.86ili7xmd5.gmgdl@evledraar.gmail.com (length 7224) for PR https://github.com/git/git/pull/1406, commit undefined, comment ID: undefined

[gitgitgadget]

This branch is now known as cw/ci-whitespace.

[gitgitgadget]

This patch series was integrated into seen via git@b82d87b.

[dscho]

It worked when I changed position to line but they could have 'fixed' it.

@webstech the position attribute is deprecated, though: https://docs.github.com/en/rest/pulls/comments?apiVersion=2022-11-28#create-a-review-comment-for-a-pull-request

My initial thought is to grab the line number from the /submit email in PatchSeries and save it in the IMailMetadata notes.

Makes sense, but what if an email comes in that comments on a commit that has hence been force-pushed out of the PR range?

My current thinking is that we should fall back to addPRComment(), mentioning the commit to which the comment is attached, but that would cause more problems down the line when somebody replies to that comment and we might erroneously try to reply to a review comment attached to a commit that does not exist. We would have to detect that the originalCommit is out of range here and avoid this code block and instead go to this code block but carefully assigning originalCommit = undefined there lest we record incorrect metadata.

This sounds slightly involved, and I will sadly not have time to implement any of it.

From job 165116, git/git notes do not have the commit?

You mean this? It worked...

The reason why this was not attached to a commit is that the reply was to a single-patch contribution whose Message-Id starts with pull., which is mistaken for a cover letter without a commit to attach the comment to. I am inclined to leave the logic as-is, though, it's a single-patch contribution so there is little sense in attaching the comment to a commit (because there is no danger of getting confused as to which commit the comment is about).

[webstech]

For future reference, there was some discussion on the git mailing list.

[gitgitgadget]

On the Git mailing list, Đoàn Trần Công Danh
wrote (reply to this), regarding a2b5f3e:

On 2022-12-20 22:08:58-0800, Chris Webster <chris@webstech.net> wrote:
> On Tue, Dec 20, 2022 at 5:53 PM Đoàn Trần Công Danh
> <congdanhqx@gmail.com> wrote:
> > Yes, I think, a patch to move the whole block into a script, maybe in
> > ci/ folder.
> 
> Maybe before the next patch or someone could create a check-whitespace
> workflow action.  Can this patch move forward?  A script would involve
> validating parameters or env variables that are just workflow context
> expressions now (ie more complexity).

I would say, we can just check an environment variables specific to
GitHub Action, and print a warning if it's missing. Other than that,
just process as normal.

> > > I am not sure what you mean.
> >
> > I mean we can write:
> >
> >         echo 'Run `git rebase ...` to correct the problem'
> >
> > With single quote, we need less escape.
> 
> What about ${lastcommit}?  Yes, there is more than one way to do it.

Ah, I misread that part. Sorry.

-- 
Danh

[gitgitgadget]

User Đoàn Trần Công Danh <congdanhqx@gmail.com> has been added to the cc: list.

[webstech]

> Maybe before the next patch or someone could create a check-whitespace
> workflow action.

@dscho What do you think about moving this to a git-for-windows owned workflow action? Moves the code out of the git repo. Would that be acceptable or is it too unmanaged for the git owners?

[gitgitgadget]

This patch series was integrated into seen via git@9e7fbaf.

[gitgitgadget]

There was a status update in the "New Topics" section about the branch cw/ci-whitespace on the Git mailing list:

CI updates.

Will merge to 'next'?
source: <pull.1444.v2.git.1671496548.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@4dab7fb.

[gitgitgadget]

This patch series was integrated into seen via git@11d56af.

[gitgitgadget]

This patch series was integrated into seen via git@3eaccb1.

[dscho]

What do you think about moving this to a git-for-windows owned workflow action? Moves the code out of the git repo. Would that be acceptable or is it too unmanaged for the git owners?

@webstech If they concretely asked to move it out, sure, let's do it. If they haven't been clear about that request, they might not accept the patch to move it out, and then we will have wasted a whole lot of our precious time. So it really depends on whether you get a clear answer to that question whether it would be acceptable or whether it would be too unmanaged.

[gitgitgadget]

This patch series was integrated into seen via git@ea98484.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch cw/ci-whitespace on the Git mailing list:

CI updates.  We probably want a clean-up to move the long shell
script embedded in yaml file into a separate file, but that can
come later.

Will merge to 'next'.
source: <pull.1444.v2.git.1671496548.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@37f8928.

[gitgitgadget]

This patch series was integrated into next via git@d3dc35f.

[webstech]

If they concretely asked to move it out, sure, let's do it.

Thanks for the input. The feeling so far appears to be to move it to a separate script. I asked because some other workflows in the git repo are using non-local scripts.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch cw/ci-whitespace on the Git mailing list:

CI updates.  We probably want a clean-up to move the long shell
script embedded in yaml file into a separate file, but that can
come later.

Will merge to 'master'.
source: <pull.1444.v2.git.1671496548.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@7ec4ccc.

[gitgitgadget]

This patch series was integrated into master via git@7ec4ccc.

[gitgitgadget]

This patch series was integrated into next via git@7ec4ccc.

[gitgitgadget]

Closed via 7ec4ccc.