Skip to content

ci: Format nixfmt command prominently#373939

Merged
infinisil merged 2 commits intoNixOS:masterfrom
roberth:ci-format-format
Jan 19, 2025
Merged

ci: Format nixfmt command prominently#373939
infinisil merged 2 commits intoNixOS:masterfrom
roberth:ci-format-format

Conversation

@roberth
Copy link
Member

@roberth roberth commented Jan 15, 2025

Formatting it prominently makes it easy to apply it, especially to simple PRs.

Off-topic: an example that rewrites history might be nice for larger ones, if someone could share something that works well.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

The latter part is structurally a full sentence, if short.
@github-actions github-actions bot added 6.topic: policy discussion Discuss policies to work in and around Nixpkgs 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions labels Jan 15, 2025
@nix-owners nix-owners bot requested a review from infinisil January 15, 2025 05:58
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jan 15, 2025
@infinisil
Copy link
Member

Off-topic: an example that rewrites history might be nice for larger ones, if someone could share something that works well.

#363759 :D

@infinisil infinisil merged commit 8e614ad into NixOS:master Jan 19, 2025
25 of 27 checks passed
@nixpkgs-ci
Copy link
Contributor

nixpkgs-ci bot commented Jan 19, 2025

Successfully created backport PR for release-24.11:

@roberth
Copy link
Member Author

roberth commented Jan 19, 2025

@infinisil, not sure.

Off-topic: an example that rewrites history might be nice for larger ones, if someone could share something that works well.

#363759 :D

That seems either unsuitable or insufficiently documented.

./run.sh script in this directory rebases the current branch onto a target branch,

Not what I want. I want to rewrite commits between merge-base and HEAD.

See the header comment of that file to understand how to mark commits.

Marking commits? I don't see why I would need that.

This is convenient for resolving merge conflicts for pull requests after e.g. treewide reformats.

Not my use case. I just want to format my own commits. It's not a rebase.

@wolfgangwalther
Copy link
Contributor

Setting up nixfmt as a pre-commit hook might help. At least eventually, when all files have been formatted once. Right now, you'd end up with random reformats all the time.

@roberth
Copy link
Member Author

roberth commented Jan 19, 2025

eventually, when all files have been formatted once.

In nix we use a massive list of exclusions so that new files and moved or refactored files are formatted.

I'm trying this now. It requires the arguments from the formatting error to be passed to the new script:

#!/usr/bin/env bash

help='
Usage: format-commits <UPSTREAM> -- FILES...

Format the commits between the current branch and <UPSTREAM>.
'

parse_args() {
  while [ $# -gt 0 ]; do
    case "$1" in
      -h|--help)
        echo "$help"
        exit 0
        ;;
      --)
        shift
        break
        ;;
      *)
        break
        ;;
    esac
    shift
  done
  if [ $# -lt 2 ]; then
    echo "$help"
    exit 1
  fi
  upstream="$1"
  shift
  files=( "$@" )
}

get_commits() {
  git log --oneline "$merge_base"..HEAD -- ${files[@]}
}

checks() {
  if ! type nixfmt &>/dev/null; then
    echo "nixfmt is not on PATH. Try again in nix-shell or nix develop."
    exit 1
  fi
}

main() {
  parse_args "$@"
  # set -x

  checks

  merge_base=$(git merge-base HEAD "$upstream")

  echo "The merge base is $merge_base."

  commits=$(get_commits)

  if [ -z "$commits" ]; then
    echo "No commits to format."
    exit 0
  fi

  if [ -z "$files" ]; then
    echo "No files to format. Pass the files to format as arguments."
    exit 0
  fi

  echo "The commits between $upstream and HEAD will be formatted:"
  # This way git shows it in color
  get_commits

  read -p "Continue? [y/N] "

  if [[ ! $REPLY =~ ^[Yy]$ ]]; then
    echo
    echo "Aborted."
    exit 1
  fi

  # FIXME: Add `--ignore-missing` to nixfmt
  # cp $(dirname ${BASH_SOURCE[0]})/format-commits-helper-optionally.sh /tmp/format-commits-helper-optionally.sh

  FILTER_BRANCH_SQUELCH_WARNING=1 \
  git filter-branch --tree-filter "/tmp/format-commits-helper-optionally.sh ${files[*]@Q}" "$merge_base"..HEAD

}

main "$@"

@infinisil
Copy link
Member

Not my use case. I just want to format my own commits. It's not a rebase.

Fair enough, I thought it would be similar enough to share the same code, but I guess not (or very little) :)

@infinisil
Copy link
Member

Soon we'll do a treewide reformat of all files, after which it'll be possible to run treefmt in pre-commit hooks and nixfmt-on-save in editors, without having to worry about formatting previously-unformatted files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions 6.topic: policy discussion Discuss policies to work in and around Nixpkgs 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants