github/workflows/format-comment: init

[Synthetica9]

Motivation for this change

Workflow to format the changes made in a PR when someone with write access
comments /format. Uses git-filter-tree to not insert any unneeded commits
but rather rewrite such that all commits appear as if they've always been
properly formatted. Based on the /rebase workflow.

Can be seen in action here: Synthetica9/nixpkgs-format-testbed#13

Related: #120832 #121306

cc @domenkozar

[Synthetica9]

There should probably also be a second workflow that checks if the changes in every commit are properly formatted. I'm willing to build this, but don't have time today.

[zowoq]

Hmm, I wasn't expecting that we'd end up with another /command action so soon.

I'm planning to rewrite the rebase action to use repo dispatch via another action. This would allow us to drop the boilerplate and have more reliable commands. (would also cut down on GHA CI usage as the currently each command would run it's own CI job for a few seconds on every comment)

It would be much easier to only need to rewrite and test one action rather than two but I'm not going to have time to deal with it at least for a couple of weeks.

[zowoq]

There should probably also be a second workflow that checks if the changes in every commit are properly formatted.

It won't run automatically after the /format action pushes as it would be from the default github actions token which won't start another action (the other PR actions wouldn't run either). It would need to use the same close/reopen hack from the rebase action or switch to another token which I'm planning to do as part of the rewrite.

[Synthetica9]

Hmm, I wasn't expecting that we'd end up with another /command action so soon.

Originally I wanted to add an action that pushes a format commit if it isn't properly formatted, but this seemed like the less intrusive option. I'm still open to suggestions for other approaches tho!

It won't run automatically after the /format action pushes as it would be from the default github actions token which won't start another action (the other PR actions wouldn't run either). It would need to use the same close/reopen hack from the rebase action or switch to another token which I'm planning to do as part of the rewrite.

This shouldn't alter the semantics, so if we trust nixpkgs-fmt enough that wouldn't be necessary. Whether we do is something that's above my pay grade.

(would also cut down on GHA CI usage as the currently each command would run it's own CI job for a few seconds on every comment)

Oh hmm, I assumed the if prevented that. Are we currently running into those limits?

I'm planning to rewrite the rebase action to use repo dispatch via another action.

Give me a ping when you do, I'll see if I can rewrite this based on that.

[asymmetric]

I wonder if this goes a step too far. If there's a CI build failure, then whoever submitted the PR should deal with fixing the issue.

Maintainers can then simply ignore PRs with a ❎, rather than being tasked with fixing errors on behalf of someone else - it's not like they aren't overloaded with work anyway ;)

[stale]

I marked this as stale due to inactivity. → More info

[Mic92]

This one should have less false-positive:

Suggested change

	git show $CURRENT_BASE..$PR_BRANCH --name-status \
	| egrep ^[AM] | grep .nix | cut -f2 \
	| sort | uniq > /tmp/changed_files
	git show $CURRENT_BASE..$PR_BRANCH --name-status \
	| awk '/^(M|A)\t.*\.nix$/ { print $2 }' | sort -u \
	> /tmp/changed_files

[Mic92]

null-terminated handling is slightly more robust here:

Suggested change

	FILTER_SCRIPT="cat /tmp/changed_files | perl -ne 'chomp(); if (-e \$_) {print \"\$_\\n\"}' | xargs -d '\n' -- nixpkgs-fmt"
	FILTER_SCRIPT="perl -ne 'chomp; print \"\$_\\0\" if (-e \$_)' < /tmp/changed_files | xargs -0 nixpkgs-fmt"

[Mic92]

on the other hands we already use perl to read in newline separated files...

[Synthetica9]

I think this should also use on:dispatch or whatever it's called.

[stale]

I marked this as stale due to inactivity. → More info

[wolfgangwalther]

I don't think we need this anymore with the full tree formatted soon and nixfmt ci checks in place. Closing, feel free to re-open, if you disagree.