From dae50ef5afadc3c853fe63ebdba7f914efeb7873 Mon Sep 17 00:00:00 2001 From: toofar Date: Thu, 10 Nov 2022 08:21:43 +1300 Subject: [PATCH 01/14] Some basic stats on which PRs merge now vs after black One of the main issues remaining about the 3.0 release is what autoformatters we can introduce to the codebase and how badly that'll conflict with the open PRs. #1455 This is an attempt to provide some numbers to support that decisions. It runs black and friends then tries to merge all the PRs. The script needs updating to try some different merge strategies to see if we can help people keep their PRs inline with the upcoming changes without all having to be updated manually. TODOs are at the bottom of the script. --- scripts/check_mergability.sh | 209 +++++++++++++++++++++++++++++++++++ 1 file changed, 209 insertions(+) create mode 100755 scripts/check_mergability.sh diff --git a/scripts/check_mergability.sh b/scripts/check_mergability.sh new file mode 100755 index 000000000..ef99194fe --- /dev/null +++ b/scripts/check_mergability.sh @@ -0,0 +1,209 @@ +#!/bin/sh + +# How to run: +# * Install the gh cli and run `gh login`: https://github.com/cli/cli/ +# * install black isort usort pyupgrade and whatever other tools you want to +# play with in your active virtualenv +# * move to a new folder for the script to work in: `mkdir pr_mergability && cd pr_mergability` +# * ../scripts/check_mergability.sh +# +# It'll clone the qutebrowser repo, fetch refs for all the open PRs, checkout +# a branch, run auto formatters, try to merge each PR, report back via CSV +# how badly each merge filed (via "number of conflicting lines"). +# +# For details of what auto formatters are ran see the `tools` variable down +# near the bottom of the script. +# +# If you've checked out a branch and ran auto-formatters or whatever on it +# manually and just want the script to try to merge all PRs you can call it +# with the branch name and it'll do so. Remember to go back up to the work dir +# before calling the script. +# +# If it's been a few days and PRs have been opened or merged delete `prs.json` +# from the working dir to have them re-fetched on next run. +# If PRs have had updates pushed you'll have to update the refs yourself or +# nuke the whole clone in the work dir and let the script re-fetch them all. + +# requires the github binary, authorized, to list open PRs. +command -v gh > /dev/null || { + echo "Error: Install the github CLI, gh, make sure it is in PATH and authenticated." + exit 1 +} +# requires some formatting tools available. The are all installable via pip. +all_formatters="black isort usort pyupgrade" +for cmd in $all_formatters; do + command -v $cmd >/dev/null || { + echo "Error: Requires all these tools to be in PATH (install them with pip): $all_formatters" + exit 1 + } +done + +[ -e qutebrowser/app.py ] && { + echo "don't run this from your qutebrowser checkout. Run it from a tmp dir, it'll checkout out a new copy to work on" + exit 1 +} +[ -d qutebrowser ] || { + git clone git@github.com:qutebrowser/qutebrowser.git + cd qutebrowser + git config --local merge.conflictstyle merge + git config --local rerere.enabled false + cd - +} + +[ -e prs.json ] || { + # (re-)fetch list of open PRs. Pull refs for any new ones. + # Resets master and qt6-v2 in case they have changed. Does not handle + # fetching new changes for updated PRs. + echo "fetching open PRs" + gh -R qutebrowser/qutebrowser pr list -s open --json number,title,mergeable,updatedAt -L 100 > prs.json + cd qutebrowser + git fetch + git checkout master && git pull + git checkout qt6-v2 && git pull + # this is slow for a fresh clone, idk how to fetch all pull/*/head refs at once + jq -r '.[] | "\(.number) \(.updatedAt) \(.title)"' < ../prs.json | while read number updated title; do + git describe pr/$number >/dev/null 2>&1 || git fetch origin refs/pull/$number/head:pr/$number + done + cd - +} + +python3 <<"EOF" +import json +from collections import Counter +import rich + +with open("prs.json") as f: prs=json.load(f) + +rich.print(Counter([p['mergeable'] for p in prs])) +# Counter({'MERGEABLE': 29, 'CONFLICTING': 45}) +EOF + +summary () { + # Summarize the accumulated report CSVs + # Should be the last thing we do since it goes back up to the report dir + cd - >/dev/null + python3 <<"EOF" +import csv, glob + +def read_csv(path): + with open(path) as f: + return list(csv.DictReader(f)) + +for report in sorted(glob.glob("report-*.csv")): + rows = read_csv(report) + succeeded = len([row for row in rows if row["state"] == "succeeded"]) + failed = len([row for row in rows if row["state"] == "failed"]) + print(f"{report} {succeeded=} {failed=}") +EOF +} + +prompt_or_summary () { + printf "$1 [Yn]: " + read ans + case "$ans" in + [nN]*) + summary + exit 0 + ;; + *) true;; + esac +} + +generate_report () { + # checkout a branch, try to merge each of the open PRs, write the results to + # a CSV file + base="${1:-master}" + quiet=$2 + + git checkout -q $base + + report_file=../report-$base.csv + [ -e $report_file ] && [ -z "$quiet" ] && { + prompt_or_summary "$report_file exists, overwrite?" + } + + echo "number,updated,title,state,clean,conflicting" > $report_file + report () { + echo "$1,$2,\"$3\",$4,$5,$6" >> $report_file + } + + jq -r '.[] | "\(.number) \(.updatedAt) \(.title)"' < ../prs.json | while read number updated title; do + [ -n "$quiet" ] || echo "trying pr/$number $updated $title" + head_sha=$(git rev-parse HEAD) + git merge -q --no-ff --no-edit pr/$number 2>&1 1>/dev/null | grep -v preimage + if [ -e .git/MERGE_HEAD ] ;then + # merge failed, clean lines staged and conflicting lines in working + # tree + merged_lines=$(git diff --cached --numstat | awk -F' ' '{sum+=$1;} END{print sum;}') + conflicting_lines=$(git diff | sed -n -e '/<<<<<<< HEAD/,/=======$/p' -e '/=======$/,/>>>>>>> pr/p' | wc -l) + conflicting_lines=$(($conflicting_lines-4)) # account for markers included in both sed expressions + [ -n "$quiet" ] || echo "#$number failed merging merged_lines=$merged_lines conflicting_lines=$conflicting_lines" + git merge --abort + report $number $updated "$title" failed $merged_lines $conflicting_lines + else + [ -n "$quiet" ] || echo "#$number merged fine" + #git show HEAD --oneline --stat + git reset -q --hard $head_sha + report $number $updated "$title" succeeded 0 0 + fi + done +} + +cd qutebrowser + +# run as `$0 some-branch` to report on merging all open PRs to a branch you +# made yourself. Otherwise run without args to try with a bunch of builtin +# configurations. +if [ -n "$1" ] ;then + generate_report "$1" +else + usort () { env usort format "$@"; } + pyupgrade () { git ls-files | grep -F .py | xargs pyupgrade --py37-plus; } + clean_branches () { + # only clean up tmp- branches in case I run it on my main qutebrowser + # checkout by mistake :) + git checkout master + git reset --hard origin/master + git branch -l | grep tmp- | grep -v detached | while read l; do git branch -qD $l ;done + } + + # pre-defined auto-formatter configurations. Branches will be created as + # needed. + # format: branch tool1 tool2 ... + tools="master true + tmp-black black + tmp-black_isort black isort + tmp-black_usort black usort + tmp-black_pyupgrade black pyupgrade + tmp-black_isort_pyupgrade black isort pyupgrade + tmp-black_isort_pyupgrade black usort pyupgrade + qt6-v2 true" + #tools="tmp-black_isort black isort + #tmp-black_usort black usort" + + prompt_or_summary "Generate report for all tool configurations?" + clean_branches + + echo "$tools" | while read branch cmds; do + echo "$branch" + git checkout -q "$branch" 2>/dev/null || git checkout -q -b "$branch" origin/master + echo "$cmds" | tr ' ' '\n' | while read cmd; do + $cmd qutebrowser tests + git commit -am "$cmd" + done + generate_report "$branch" y + done +fi + +summary + +# todo: +# * see if we can run formatters on PR branches before/while merging +# * do most stuff based off of qt6-v2 instead of master, not like most PRs +# will be merged to pre-3.0 master anyway +# notes: +# after merging qt6-v2 would merging old PRs to old master then somehow merging +# the PR merge commit up to the new master easier than rebasing the PR? +# there is a filter attribute you can use to re-write files before committing. +# For this use case probably the same as rebase -i --exec then merge? +# >See "Merging branches with differing checkin/checkout attributes" in gitattributes(5) From fa7b7ee72c95690e9a3cf50ebd204bf7d70429eb Mon Sep 17 00:00:00 2001 From: toofar Date: Wed, 16 Nov 2022 20:07:05 +1300 Subject: [PATCH 02/14] Add initial attempt at applying formatting to PRs See the comments in the script for more details. We are trying to see if we can migrate PRs to be based off of a master branch that has had autoformatting applied to it. Doing this via a merge is very difficult because once you have conflict markers you can't really do much. So you need to apply the autoformatters to the base branch and the PR branch and then try to point the changed PR branch at the changed master. I tried doing it via a rebase first, which necessitates re-writing the commits transparently when applying them. It seems to work well enough. An initial one shows all of the PRs that applies successfully to master applied to the changed master after going through this process. I tried doing it via a rebase first because I find rebases easier to reason about than merges. I did add a merge strategy too but it performed poorly. I haven't put much thought into it and are probably doing something wrong. There's some duplication and hardcoded stuff. The tool "aliases" I moved up because the re-writing is done in two places now. I'm hardcoding the master report for excluding already-failing PRs, that might end up being fine. Next steps: * verify this rebase strategy some more * see how many fail when rebasing on qt6-v2 too * look at the merge strategy again * clean up the script more, consider porting it to python if the results are anywhere near acceptable --- scripts/check_mergability.sh | 165 +++++++++++++++++++++++++++++++++-- 1 file changed, 158 insertions(+), 7 deletions(-) diff --git a/scripts/check_mergability.sh b/scripts/check_mergability.sh index ef99194fe..3ef77b605 100755 --- a/scripts/check_mergability.sh +++ b/scripts/check_mergability.sh @@ -109,15 +109,30 @@ prompt_or_summary () { esac } +# format tool "aliases", where needed +usort () { env usort format "$@"; } +black () { env black -q "$@"; } +pyupgrade () { git ls-files | grep -F .py | xargs pyupgrade --py37-plus; } + generate_report () { # checkout a branch, try to merge each of the open PRs, write the results to # a CSV file base="${1:-master}" - quiet=$2 + quiet="$2" + rewrite_strategy="$3" + cmds="$4" + pr="$5" + report_file=../report-$base.csv + + # prefix for working branch when we are going to re-write stuff so we don't + # mess up the pr/* branches and have to re-fetch them. + [ -n "$rewrite_strategy" ] && { + prefix="tmp-rewrite-" + report_file=../report-$base-$rewrite_strategy.csv + } git checkout -q $base - report_file=../report-$base.csv [ -e $report_file ] && [ -z "$quiet" ] && { prompt_or_summary "$report_file exists, overwrite?" } @@ -128,9 +143,37 @@ generate_report () { } jq -r '.[] | "\(.number) \(.updatedAt) \(.title)"' < ../prs.json | while read number updated title; do - [ -n "$quiet" ] || echo "trying pr/$number $updated $title" + [ -n "$pr" ] && [ "$pr" != "$number" ] continue + [ -n "$quiet" ] || echo "trying ${prefix}pr/$number $updated $title" head_sha=$(git rev-parse HEAD) - git merge -q --no-ff --no-edit pr/$number 2>&1 1>/dev/null | grep -v preimage + + case "$rewrite_strategy" in + rebase|merge) + # Only attempt branches that actually merge cleanly with master. + # Theoretically it wouldn't hurt to do all of them but a) running + # black via the filter driver is slow b) rebase_with_formatting needs + # some work to handle more errors in that case (the "git commit -qam + # 'fix lint" bit at least needs to look for conflict markers) + # I'm hardcoding master because of a lack of imagination. + grep "^$number" ../report-master.csv | grep failed && { + echo "pr/$number succeeded already in ../report-master.csv, skipping" + continue + } + rebase_with_formatting "$number" "$base" "$cmds" "$prefix" "$rewrite_strategy" || { + report $number $updated "$title" failed 999 999 + continue + } + ;; + '') + true + ;; + *) + echo "Unknown rewrite strategy '$rewrite_strategy'" + exit 1 + ;; + esac + + git merge -q --no-ff --no-edit ${prefix}pr/$number 2>&1 1>/dev/null | grep -v preimage if [ -e .git/MERGE_HEAD ] ;then # merge failed, clean lines staged and conflicting lines in working # tree @@ -149,16 +192,124 @@ generate_report () { done } +rebase_with_formatting () { + number="$1" + base="$2" + cmds="$3" + prefix="${4:-tmp-rewrite-}" + strategy="$5" + + # We need to apply formatting to PRs and base them on a reformatted base + # branch. + # I haven't looked into doing that via a merge but here is an attempt + # doing a rebase. + # Rebasing directly on to a formatted branch will fail very easily when it + # runs into a formatting change. So I'm using git's "filter" attribute to + # apply the same formatter to the trees corresponding to the + # commits being rebased. Hopefully if we apply the same formatter to the + # base branch and to the individual commits from the PRs we can minimize + # conflicts. + # An alternative to using the filter attribute might be to use something + # like the "darker" tool to re-write the commits. I suspect that won't + # help with conflicts in the context around changes though. + + # Checkout the parent commit of the branch then apply formatting tools to + # it. This will provide a target for rebasing which doesn't have any + # additional drift from changes to master. After that then we can rebase + # the re-written PR branch to the more current, autoformatted, master. + # TODO: It might be possible to skip the intermediate base branch. + git checkout -b tmp-master-rewrite-pr/$number `git merge-base origin/master pr/$number` + echo "$cmds" | tr ' ' '\n' | while read cmd; do + $cmd qutebrowser tests + git commit -am "dropme! $cmd" # mark commits for dropping when we rebase onto the more recent master + done + + git checkout -b ${prefix}pr/$number pr/$number + + # Setup the filters. A "smudge" filter is configured for each tool then we + # add the required tools to a gitattributes file. And make sure to clean + # it up later. + # Running the formatters as filters is slower than running them directly + # because they seem to be run on the files serially. TODO: can we + # parallelize them? + # Maybe just adding a wrapper around the formatters that caches the output + # would be simpler. At least then you just have to sit through them once. + git config --local --get filter.black.smudge >/dev/null || { + git config --local filter.black.smudge "black -q -" + git config --local filter.isort.smudge "isort -q -" + git config --local filter.pyupgrade.smudge "pyupgrade --py27-plus -" + git config --local filter.usort.smudge "usort format -" + } + printf "*.py" > .git/info/attributes + echo "$cmds" | tr ' ' '\n' | while read cmd; do + printf " filter=$cmd" >> .git/info/attributes + done + echo >> .git/info/attributes + + # not sure why I had to do the extra git commit in there, there are some + # changes left in the working directory sometimes? TODO: change to a + # commit --amend -C HEAD after confirming overall results + # TODO: look for conflict markers before merging + git rebase -q -X renormalize --exec 'git commit -qam "fix lint" || true' tmp-master-rewrite-pr/$number + exit_code="$?" + [ $exit_code -eq 0 ] || git rebase --abort + git branch -D tmp-master-rewrite-pr/$number + rm .git/info/attributes + + [ $exit_code -eq 0 ] || return $exit_code + + if [ "$strategy" = "rebase" ] ;then + # now transplant onto the actual upstream branch -- might have to drop this + # if it causes problems. + EDITOR='sed -i /dropme/d' git rebase -qi "$base" || { + git rebase --abort + return 1 + } + fi + + git checkout -q $base +} + cd qutebrowser # run as `$0 some-branch` to report on merging all open PRs to a branch you # made yourself. Otherwise run without args to try with a bunch of builtin # configurations. + +strategy="" +pull_request="" +while [ -n "$1" ] ;do + case "$1" in + -s|--rewrite-strategy) + shift + [ -n "$1" ] || { + echo "What strategy?" + exit 1 + } + strategy="$1" + ;; + -p|--pull-request) + shift + [ -n "$1" ] || { + echo "Which PR?" + exit 1 + } + pull_request="$1" + ;; + -*) + echo "Unknown argument '$1'" + exit 1 + ;; + *) + break + ;; + esac + shift +done + if [ -n "$1" ] ;then generate_report "$1" else - usort () { env usort format "$@"; } - pyupgrade () { git ls-files | grep -F .py | xargs pyupgrade --py37-plus; } clean_branches () { # only clean up tmp- branches in case I run it on my main qutebrowser # checkout by mistake :) @@ -191,7 +342,7 @@ else $cmd qutebrowser tests git commit -am "$cmd" done - generate_report "$branch" y + generate_report "$branch" y "$strategy" "$cmds" "$pull_request" done fi From 41cb4840ff24a68211b842a87bf86583cf5654e7 Mon Sep 17 00:00:00 2001 From: toofar Date: Fri, 25 Nov 2022 17:25:06 +1300 Subject: [PATCH 03/14] Support running multiple tools in git filter It seems if you specify the same attribute multiple times in gitattributes the last one wins. So on runs with multiple tools we were only running the last one. This switches to running all the tools from a shell script. Which is written out from a shell script. I think there is probably a few more cards we can stack on here before it all comes tumbling down. TODO: * why are we not seeing much of an improvement for the more complicated case, weren't most of the conflicts in the first place from black? report-black.csv succeeded=16 failed=58 report-tmp-black-rebase.csv succeeded=20 failed=9 report-black_isort_pyupgrade.csv succeeded=15 failed=59 report-tmp-black_isort_pyupgrade-rebase.csv succeeded=16 failed=13 --- scripts/check_mergability.sh | 69 +++++++++++++++++++++++++++++------- 1 file changed, 57 insertions(+), 12 deletions(-) diff --git a/scripts/check_mergability.sh b/scripts/check_mergability.sh index 3ef77b605..f9422381f 100755 --- a/scripts/check_mergability.sh +++ b/scripts/check_mergability.sh @@ -4,6 +4,7 @@ # * Install the gh cli and run `gh login`: https://github.com/cli/cli/ # * install black isort usort pyupgrade and whatever other tools you want to # play with in your active virtualenv +# * also requires "sponge" from moreutils # * move to a new folder for the script to work in: `mkdir pr_mergability && cd pr_mergability` # * ../scripts/check_mergability.sh # @@ -234,18 +235,53 @@ rebase_with_formatting () { # parallelize them? # Maybe just adding a wrapper around the formatters that caches the output # would be simpler. At least then you just have to sit through them once. - git config --local --get filter.black.smudge >/dev/null || { - git config --local filter.black.smudge "black -q -" - git config --local filter.isort.smudge "isort -q -" - git config --local filter.pyupgrade.smudge "pyupgrade --py27-plus -" - git config --local filter.usort.smudge "usort format -" - } + git config --local filter.rewrite.smudge "filter-cache" printf "*.py" > .git/info/attributes - echo "$cmds" | tr ' ' '\n' | while read cmd; do - printf " filter=$cmd" >> .git/info/attributes - done + printf " filter=rewrite" >> .git/info/attributes echo >> .git/info/attributes + mkdir filter-tools 2>/dev/null + cat > filter-tools/filter-cache < "\$inputf" + +# TODO: de-dup these with the parent script? +# Can use aliases here? +# Call with the file drectly instead of using stdin? +usort () { env usort format -; } +black () { env black -q -; } +isort () { env isort -q -; } +pyupgrade () { env pyupgrade --py37-plus -; } + +run_with_cache () { + inputf="\$1" + cmd="\$2" + input_hash="\$(sha1sum "\$inputf" | cut -d' ' -f1)" + + mkdir -p /tmp/filter-caches/\$cmd 2>/dev/null + outputf=/tmp/filter-caches/\$cmd/\$input_hash + + [ -e "\$outputf" ] || \$cmd < "\$inputf" > "\$outputf" + + cat "\$outputf" +} + +echo "\$cmds" | tr ' ' '\n' | while read cmd; do + run_with_cache \$inputf "\$cmd" | sponge \$inputf +done + +cat "\$inputf" +EOF + chmod +x filter-tools/filter-cache + export PATH="$PWD/filter-tools:$PATH" + # not sure why I had to do the extra git commit in there, there are some # changes left in the working directory sometimes? TODO: change to a # commit --amend -C HEAD after confirming overall results @@ -327,10 +363,9 @@ else tmp-black_usort black usort tmp-black_pyupgrade black pyupgrade tmp-black_isort_pyupgrade black isort pyupgrade - tmp-black_isort_pyupgrade black usort pyupgrade + tmp-black_usort_pyupgrade black usort pyupgrade qt6-v2 true" - #tools="tmp-black_isort black isort - #tmp-black_usort black usort" + tools="tmp-black black" prompt_or_summary "Generate report for all tool configurations?" clean_branches @@ -352,9 +387,19 @@ summary # * see if we can run formatters on PR branches before/while merging # * do most stuff based off of qt6-v2 instead of master, not like most PRs # will be merged to pre-3.0 master anyway +# * for strategies where we skip PRs that failed in master include them in the +# report to for reference. With a marker to that affect and a total diffstat +# so we can see how big they are +# * *try the more simplistic "Run the formatter on all PR branches then merge" +# instead of trying to do it via a rebase* +# * try rebasing them to an autoformatted qt6-v2 branch # notes: # after merging qt6-v2 would merging old PRs to old master then somehow merging # the PR merge commit up to the new master easier than rebasing the PR? # there is a filter attribute you can use to re-write files before committing. # For this use case probably the same as rebase -i --exec then merge? # >See "Merging branches with differing checkin/checkout attributes" in gitattributes(5) +# if we go with the strategy of rebasing PRs on formatted commits how to deal +# with stopping isort making import loops on every damn PR. Still need to try +# rebasing directly on the latest formatted master instead of doing the +# intermediated one. From 59086b870b476c5b752c5b7e96798a82dc598682 Mon Sep 17 00:00:00 2001 From: toofar Date: Thu, 8 Dec 2022 08:39:19 +1300 Subject: [PATCH 04/14] fix resetting base branch between PRs Previously it was setting head_sha within the loop. When doing rebase_with_formatting and that failed we weren't resetting to $base properly. So it ended up trying to merge subsequent PRs onto previous ones, which caused additional PRs to show as failed. Change to save the base branch sha before we start doing work and just reset at the top of the loop instead of trying to do it on each failure path. --- scripts/check_mergability.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/check_mergability.sh b/scripts/check_mergability.sh index f9422381f..cc432c530 100755 --- a/scripts/check_mergability.sh +++ b/scripts/check_mergability.sh @@ -143,10 +143,11 @@ generate_report () { echo "$1,$2,\"$3\",$4,$5,$6" >> $report_file } + head_sha=$(git rev-parse HEAD) jq -r '.[] | "\(.number) \(.updatedAt) \(.title)"' < ../prs.json | while read number updated title; do [ -n "$pr" ] && [ "$pr" != "$number" ] continue [ -n "$quiet" ] || echo "trying ${prefix}pr/$number $updated $title" - head_sha=$(git rev-parse HEAD) + git reset -q --hard $head_sha case "$rewrite_strategy" in rebase|merge) @@ -187,7 +188,6 @@ generate_report () { else [ -n "$quiet" ] || echo "#$number merged fine" #git show HEAD --oneline --stat - git reset -q --hard $head_sha report $number $updated "$title" succeeded 0 0 fi done From 6ba122ced41524a95484a3f3ebc595ea0a543a01 Mon Sep 17 00:00:00 2001 From: toofar Date: Thu, 8 Dec 2022 08:40:30 +1300 Subject: [PATCH 05/14] fix cache to handle multiple tool runs The cache for changes due to pyupgrade aren't the same from "pyupgrade" as "black pyupgrade", so include the full set of commands in the cache path. Unfortunately that makes the cache less useful across different configurations, but hopefully it still helps me when developing this. --- scripts/check_mergability.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/check_mergability.sh b/scripts/check_mergability.sh index cc432c530..1e8affc89 100755 --- a/scripts/check_mergability.sh +++ b/scripts/check_mergability.sh @@ -249,7 +249,7 @@ rebase_with_formatting () { # it. cmds="$cmds" -inputf="\$(mktemp)" +inputf="\$(mktemp --suffix=rebase)" cat > "\$inputf" # TODO: de-dup these with the parent script? @@ -265,8 +265,8 @@ run_with_cache () { cmd="\$2" input_hash="\$(sha1sum "\$inputf" | cut -d' ' -f1)" - mkdir -p /tmp/filter-caches/\$cmd 2>/dev/null - outputf=/tmp/filter-caches/\$cmd/\$input_hash + mkdir -p "/tmp/filter-caches/\$cmds/\$cmd" 2>/dev/null + outputf="/tmp/filter-caches/\$cmds/\$cmd/\$input_hash" [ -e "\$outputf" ] || \$cmd < "\$inputf" > "\$outputf" From e9da3e4f69c320ad703a01d206ad97ff6991ec28 Mon Sep 17 00:00:00 2001 From: toofar Date: Thu, 8 Dec 2022 08:43:21 +1300 Subject: [PATCH 06/14] add -X theirs to rebase onto formatted master I'm seeing conflicts when trying to apply later commits that are complaining about formatting changes made in earlier commits. In particular the changes that are left in the working tree during a rebase that are getting picked up by the "fix lint" commits. I'm a little worried about this. For resolving those particular conflicts -X theirs is the right thing to do, just take the new commit, it already essentially replaces prior ones. But in cases where there is a real conflict with master might we be obscuring that? Should be able to test that. TODO: * look more into the cause of the "fix lint" commits, maybe even try changing that to `reset HEAD` since those are just lint changes * test if -X theirs will obscure real conflicts, look into some PRs to see if it is doing weird stuff (I checked 7405 and 7312 already and they look good!) --- scripts/check_mergability.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/check_mergability.sh b/scripts/check_mergability.sh index 1e8affc89..a18ad8098 100755 --- a/scripts/check_mergability.sh +++ b/scripts/check_mergability.sh @@ -285,8 +285,13 @@ EOF # not sure why I had to do the extra git commit in there, there are some # changes left in the working directory sometimes? TODO: change to a # commit --amend -C HEAD after confirming overall results + # Need to revisit some assumptions about the smudge filter, does it always + # leave changes in the working tree? # TODO: look for conflict markers before merging - git rebase -q -X renormalize --exec 'git commit -qam "fix lint" || true' tmp-master-rewrite-pr/$number + # `theirs` here applies to the incoming commits, so the branch being + # rebased. Without that changes made by the smudge filter seem to conflict + # with later changes by the smudge filter. See #7312 for example + git rebase -q -X theirs -X renormalize --exec 'git commit -qam "fix lint" || true' tmp-master-rewrite-pr/$number exit_code="$?" [ $exit_code -eq 0 ] || git rebase --abort git branch -D tmp-master-rewrite-pr/$number From c6212e2f5890a0faac14ebe54d447951b5b0afff Mon Sep 17 00:00:00 2001 From: toofar Date: Thu, 8 Dec 2022 08:55:04 +1300 Subject: [PATCH 07/14] add debug hooks Add a second argument to `maybepause()` to get it to pause working so you can investigate in another terminal. --- scripts/check_mergability.sh | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/scripts/check_mergability.sh b/scripts/check_mergability.sh index a18ad8098..eefd7a0e0 100755 --- a/scripts/check_mergability.sh +++ b/scripts/check_mergability.sh @@ -43,6 +43,29 @@ done echo "don't run this from your qutebrowser checkout. Run it from a tmp dir, it'll checkout out a new copy to work on" exit 1 } + +TTY="$(tty)" +DO_PAUSE="no" +maybepause () { + msg="$1" + force="$2" + if [ -n "$force" ] ;then + DO_PAUSE="yes" + elif [ "$DO_PAUSE" = "yes" ] ;then + true + else + return + fi + + echo "$1, investigate in another terminal, continue? [Step|Continue|Quit]" + read response < $TTY + case "$response" in + [Cc]*) DO_PAUSE="no";; + [Qq]*) exit 0;; + *) return;; + esac +} + [ -d qutebrowser ] || { git clone git@github.com:qutebrowser/qutebrowser.git cd qutebrowser @@ -112,6 +135,7 @@ prompt_or_summary () { # format tool "aliases", where needed usort () { env usort format "$@"; } +isort () { env isort -q "$@"; } black () { env black -q "$@"; } pyupgrade () { git ls-files | grep -F .py | xargs pyupgrade --py37-plus; } @@ -183,6 +207,7 @@ generate_report () { conflicting_lines=$(git diff | sed -n -e '/<<<<<<< HEAD/,/=======$/p' -e '/=======$/,/>>>>>>> pr/p' | wc -l) conflicting_lines=$(($conflicting_lines-4)) # account for markers included in both sed expressions [ -n "$quiet" ] || echo "#$number failed merging merged_lines=$merged_lines conflicting_lines=$conflicting_lines" + maybepause "merge of ${prefix}pr/$number into $base failed" git merge --abort report $number $updated "$title" failed $merged_lines $conflicting_lines else @@ -293,7 +318,10 @@ EOF # with later changes by the smudge filter. See #7312 for example git rebase -q -X theirs -X renormalize --exec 'git commit -qam "fix lint" || true' tmp-master-rewrite-pr/$number exit_code="$?" - [ $exit_code -eq 0 ] || git rebase --abort + [ $exit_code -eq 0 ] || { + maybepause "rebase -X renormalize of ${prefix}pr/$number onto tmp-master-rewrite-pr/$number failed" + git rebase --abort + } git branch -D tmp-master-rewrite-pr/$number rm .git/info/attributes @@ -303,6 +331,7 @@ EOF # now transplant onto the actual upstream branch -- might have to drop this # if it causes problems. EDITOR='sed -i /dropme/d' git rebase -qi "$base" || { + maybepause "rebase of ${prefix}pr/$number onto $base failed" git rebase --abort return 1 } From 507e678de8ae78a5365f66bfc03c9d5f995c0271 Mon Sep 17 00:00:00 2001 From: toofar Date: Mon, 2 Jan 2023 15:59:51 +1300 Subject: [PATCH 08/14] add more docs for rebase -X theirs TODO: look at merging instead of rebasing --- scripts/check_mergability.sh | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/scripts/check_mergability.sh b/scripts/check_mergability.sh index eefd7a0e0..69bf85875 100755 --- a/scripts/check_mergability.sh +++ b/scripts/check_mergability.sh @@ -307,15 +307,31 @@ EOF chmod +x filter-tools/filter-cache export PATH="$PWD/filter-tools:$PATH" - # not sure why I had to do the extra git commit in there, there are some - # changes left in the working directory sometimes? TODO: change to a - # commit --amend -C HEAD after confirming overall results - # Need to revisit some assumptions about the smudge filter, does it always - # leave changes in the working tree? - # TODO: look for conflict markers before merging - # `theirs` here applies to the incoming commits, so the branch being - # rebased. Without that changes made by the smudge filter seem to conflict - # with later changes by the smudge filter. See #7312 for example + # Description of extra options: + # --exec 'git commit -qam "fix lint"': The git smudge filter leaves changes + # in the working tree. So we need to include these in a commit if we + # want to keep them. + # --exec '... || true': git commit fails if there is nothing to commit, in + # this case meaning the filter didn't need to make any changes to the + # commit we just applied. So short circuiting to `true` just makes it so + # the result code is always 0 and the rebase continues. + # -X theirs: in the case of conflicts, disregard the changes in the working + # tree and apply those from the incoming commit. In this case when you + # have one commit later in a PR that builds on an earlier one, and we + # re-formatted the earlier one, the later one will fail to apply. Since + # we know these commits already build on each other and that any + # conflicts are due to formatting changes, which'll be applied again + # later we can safely disregard the changes. + # So this ends up with the right result but is problematic for two + # reason: + # a) it adds noise to the PRs because formatting changes are applied, + # reverted, then applied again. + # b) if there are any conflicts with the base branches the base branch + # changes will be reverted. In this script we are already checking + # that PRs apply cleanly to their existing base before rebasing them + # on an auto-formatted version of it. So we shouldn't run into that. + # But if this is used in more scenarios it will likely cause some + # frustration. git rebase -q -X theirs -X renormalize --exec 'git commit -qam "fix lint" || true' tmp-master-rewrite-pr/$number exit_code="$?" [ $exit_code -eq 0 ] || { From 355da9a44e4da7ec3a16613d6375234720e34df6 Mon Sep 17 00:00:00 2001 From: toofar Date: Tue, 3 Jan 2023 17:15:54 +1300 Subject: [PATCH 09/14] Run formatter tools twice Pyupgrade and black were fighting, which made this difficult to pin down. #4578 was a good test case. This makes it a bit slow now, of course. --- scripts/check_mergability.sh | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/scripts/check_mergability.sh b/scripts/check_mergability.sh index 69bf85875..5d4e6a6fa 100755 --- a/scripts/check_mergability.sh +++ b/scripts/check_mergability.sh @@ -249,6 +249,19 @@ rebase_with_formatting () { $cmd qutebrowser tests git commit -am "dropme! $cmd" # mark commits for dropping when we rebase onto the more recent master done + # Occasionally we get situations where black and pyupgrade build on each + # other to enable further changes. So the order you run them matters. But we + # have situations where each one enables the other, in both orders. So we + # run them all yet again to pick up any lingering changes from the first + # run. + # If we don't do this the leftover changes can be picked up by the smudge + # filter during the first rebase below and added to "fix lint" commits. Then + # since they don't have "dropme!" in the messages they stick in the branch + # and end up conflicting with the base branch. + echo "$cmds" | tr ' ' '\n' | while read cmd; do + $cmd qutebrowser tests + git commit -am "dropme! $cmd 2" + done git checkout -b ${prefix}pr/$number pr/$number @@ -427,6 +440,15 @@ else $cmd qutebrowser tests git commit -am "$cmd" done + # Occasionally we get situations where black and pyupgrade build on each + # other to enable further changes. So the order you run them matters. But we + # have situations where each one enables the other, in both orders. So we + # run them all yet again to pick up any lingering changes from the first + # run. + echo "$cmds" | tr ' ' '\n' | while read cmd; do + $cmd qutebrowser tests + git commit -am "$cmd second run" + done generate_report "$branch" y "$strategy" "$cmds" "$pull_request" done fi From 6576c03de08d7878f11d5869c119a1767bf3c897 Mon Sep 17 00:00:00 2001 From: toofar Date: Tue, 3 Jan 2023 17:19:08 +1300 Subject: [PATCH 10/14] misc fixes clean up temp files always log the fail reason even if not pausing --- scripts/check_mergability.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/check_mergability.sh b/scripts/check_mergability.sh index 5d4e6a6fa..5bab9a98c 100755 --- a/scripts/check_mergability.sh +++ b/scripts/check_mergability.sh @@ -54,6 +54,7 @@ maybepause () { elif [ "$DO_PAUSE" = "yes" ] ;then true else + echo "$1" return fi @@ -169,7 +170,7 @@ generate_report () { head_sha=$(git rev-parse HEAD) jq -r '.[] | "\(.number) \(.updatedAt) \(.title)"' < ../prs.json | while read number updated title; do - [ -n "$pr" ] && [ "$pr" != "$number" ] continue + [ -n "$pr" ] && [ "$pr" != "$number" ] && continue [ -n "$quiet" ] || echo "trying ${prefix}pr/$number $updated $title" git reset -q --hard $head_sha @@ -292,7 +293,7 @@ cat > "\$inputf" # TODO: de-dup these with the parent script? # Can use aliases here? -# Call with the file drectly instead of using stdin? +# Call with the file directly instead of using stdin? usort () { env usort format -; } black () { env black -q -; } isort () { env isort -q -; } @@ -316,6 +317,7 @@ echo "\$cmds" | tr ' ' '\n' | while read cmd; do done cat "\$inputf" +rm "\$inputf" EOF chmod +x filter-tools/filter-cache export PATH="$PWD/filter-tools:$PATH" From 8e996f3d67a5e55a94f70b88d751f339b6c95a88 Mon Sep 17 00:00:00 2001 From: toofar Date: Tue, 3 Jan 2023 17:19:55 +1300 Subject: [PATCH 11/14] handle strangely empty cached files in caching filter Weird, not sure where these empty files are coming from. Also tell pyupgrade to always return 0 where nothing odd happens. --- scripts/check_mergability.sh | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/scripts/check_mergability.sh b/scripts/check_mergability.sh index 5bab9a98c..5120e8c01 100755 --- a/scripts/check_mergability.sh +++ b/scripts/check_mergability.sh @@ -297,7 +297,7 @@ cat > "\$inputf" usort () { env usort format -; } black () { env black -q -; } isort () { env isort -q -; } -pyupgrade () { env pyupgrade --py37-plus -; } +pyupgrade () { env pyupgrade --exit-zero-even-if-changed --py37-plus -; } run_with_cache () { inputf="\$1" @@ -307,7 +307,25 @@ run_with_cache () { mkdir -p "/tmp/filter-caches/\$cmds/\$cmd" 2>/dev/null outputf="/tmp/filter-caches/\$cmds/\$cmd/\$input_hash" - [ -e "\$outputf" ] || \$cmd < "\$inputf" > "\$outputf" + if [ -e "\$outputf" ] ;then + lines="\$(wc -l "\$outputf" | cut -d' ' -f1)" + # where are these empty output files coming from??? + # echo "removing bad cached file '\$outputf'" >&2 + [ \$lines -eq 0 ] && rm "\$outputf" + fi + + if ! [ -e "\$outputf" ] ;then + \$cmd < "\$inputf" > "\$outputf" + [ \$? -eq 0 ] || { + echo "\$cmd failed" >&2 + cat "\$inputf" + return + } + lines="\$(wc -l "\$outputf" | cut -d' ' -f1)" + [ \$lines -eq 0 ] && { + echo "tool '\$cmd' produced 0 line output file from '\$inputf'" >&2 + } + fi cat "\$outputf" } From df782f8d38f1d16e9a212e2f90dff2979b575f61 Mon Sep 17 00:00:00 2001 From: toofar Date: Tue, 3 Jan 2023 17:25:06 +1300 Subject: [PATCH 12/14] add plain merge based strategy It seems -X renormalize works from with merge. And I think more consistently than rebasing? Not sure, need to go back and check after the last round of fixes. This just merges an auto-formatted merge-base branch into the PR, and applies more autoformatting in the merge commit. It does not try to merge the PR into a more recent base branch (which is what we want to do with the open PRs). So it's not an answer yet but it might be the first step in one. Probably there could be more code de-duplicated between merge_with_formatting and rebase_with_formatting. TODO: * look at merging into the more recent base instead of the temp one * if that doesn't work see if we can rebase the merge commit up or something after this initial formatting related merge --- scripts/check_mergability.sh | 171 +++++++++++++++++++++++++---------- 1 file changed, 121 insertions(+), 50 deletions(-) diff --git a/scripts/check_mergability.sh b/scripts/check_mergability.sh index 5120e8c01..de6604e39 100755 --- a/scripts/check_mergability.sh +++ b/scripts/check_mergability.sh @@ -174,15 +174,32 @@ generate_report () { [ -n "$quiet" ] || echo "trying ${prefix}pr/$number $updated $title" git reset -q --hard $head_sha + applies_cleanly_to_master () { + number="$1" + grep "^$number" ../report-master.csv | grep failed + [ $? -eq 1 ] + return $? + } + case "$rewrite_strategy" in - rebase|merge) + merge) + applies_cleanly_to_master $number || { + echo "pr/$number succeeded already in ../report-master.csv, skipping" + continue + } + merge_with_formatting "$number" "$base" "$cmds" "$prefix" "$rewrite_strategy" || { + report $number $updated "$title" failed 999 999 + continue + } + ;; + rebase) # Only attempt branches that actually merge cleanly with master. # Theoretically it wouldn't hurt to do all of them but a) running # black via the filter driver is slow b) rebase_with_formatting needs # some work to handle more errors in that case (the "git commit -qam # 'fix lint" bit at least needs to look for conflict markers) # I'm hardcoding master because of a lack of imagination. - grep "^$number" ../report-master.csv | grep failed && { + applies_cleanly_to_master $number || { echo "pr/$number succeeded already in ../report-master.csv, skipping" continue } @@ -219,53 +236,8 @@ generate_report () { done } -rebase_with_formatting () { - number="$1" - base="$2" - cmds="$3" - prefix="${4:-tmp-rewrite-}" - strategy="$5" - - # We need to apply formatting to PRs and base them on a reformatted base - # branch. - # I haven't looked into doing that via a merge but here is an attempt - # doing a rebase. - # Rebasing directly on to a formatted branch will fail very easily when it - # runs into a formatting change. So I'm using git's "filter" attribute to - # apply the same formatter to the trees corresponding to the - # commits being rebased. Hopefully if we apply the same formatter to the - # base branch and to the individual commits from the PRs we can minimize - # conflicts. - # An alternative to using the filter attribute might be to use something - # like the "darker" tool to re-write the commits. I suspect that won't - # help with conflicts in the context around changes though. - - # Checkout the parent commit of the branch then apply formatting tools to - # it. This will provide a target for rebasing which doesn't have any - # additional drift from changes to master. After that then we can rebase - # the re-written PR branch to the more current, autoformatted, master. - # TODO: It might be possible to skip the intermediate base branch. - git checkout -b tmp-master-rewrite-pr/$number `git merge-base origin/master pr/$number` - echo "$cmds" | tr ' ' '\n' | while read cmd; do - $cmd qutebrowser tests - git commit -am "dropme! $cmd" # mark commits for dropping when we rebase onto the more recent master - done - # Occasionally we get situations where black and pyupgrade build on each - # other to enable further changes. So the order you run them matters. But we - # have situations where each one enables the other, in both orders. So we - # run them all yet again to pick up any lingering changes from the first - # run. - # If we don't do this the leftover changes can be picked up by the smudge - # filter during the first rebase below and added to "fix lint" commits. Then - # since they don't have "dropme!" in the messages they stick in the branch - # and end up conflicting with the base branch. - echo "$cmds" | tr ' ' '\n' | while read cmd; do - $cmd qutebrowser tests - git commit -am "dropme! $cmd 2" - done - - git checkout -b ${prefix}pr/$number pr/$number - +add_smudge_filter () { + cmds="$1" # Setup the filters. A "smudge" filter is configured for each tool then we # add the required tools to a gitattributes file. And make sure to clean # it up later. @@ -339,6 +311,105 @@ rm "\$inputf" EOF chmod +x filter-tools/filter-cache export PATH="$PWD/filter-tools:$PATH" +} + +remove_smudge_filter () { + # no need to remove the config or script, it's only active when the + # attribute is set + rm .git/info/attributes +} + +merge_with_formatting () { + number="$1" + base="$2" + cmds="$3" + prefix="${4:-tmp-rewrite-}" + strategy="$5" + + # Use a temp base branch for now but adding "dropme" commits probably isn't the right + # strategy for the end goal of letting PR authors adapt to autoformatter + # changes. At that point we'll already have a re-formatted master branch. + # Unless we can do the merge then rebase-keep-merges-but-drop-dropme or + # something. + # TODO: swap out this block to be based off of real master or qt-v2 or $base + git checkout -b tmp-master-rewrite-pr/$number `git merge-base origin/master pr/$number` + echo "$cmds" | tr ' ' '\n' | while read cmd; do + $cmd qutebrowser tests + git commit -am "dropme! $cmd" # mark commits for dropping when we rebase onto the more recent master + done + echo "$cmds" | tr ' ' '\n' | while read cmd; do + $cmd qutebrowser tests + git commit -am "dropme! $cmd 2" + done + + git checkout -b ${prefix}pr/$number pr/$number + + add_smudge_filter "$cmds" + + git merge -X renormalize tmp-master-rewrite-pr/$number + exit_code="$?" + remove_smudge_filter + if [ $exit_code -eq 0 ] ;then + git commit -qam "fix lint" + else + maybepause "merge of ${prefix}pr/$number onto tmp-master-rewrite-pr/$number failed" + git merge --abort + fi + git branch -D tmp-master-rewrite-pr/$number + + [ $exit_code -eq 0 ] || return $exit_code + + git checkout -q $base +} + +rebase_with_formatting () { + number="$1" + base="$2" + cmds="$3" + prefix="${4:-tmp-rewrite-}" + strategy="$5" + + # We need to apply formatting to PRs and base them on a reformatted base + # branch. + # I haven't looked into doing that via a merge but here is an attempt + # doing a rebase. + # Rebasing directly on to a formatted branch will fail very easily when it + # runs into a formatting change. So I'm using git's "filter" attribute to + # apply the same formatter to the trees corresponding to the + # commits being rebased. Hopefully if we apply the same formatter to the + # base branch and to the individual commits from the PRs we can minimize + # conflicts. + # An alternative to using the filter attribute might be to use something + # like the "darker" tool to re-write the commits. I suspect that won't + # help with conflicts in the context around changes though. + + # Checkout the parent commit of the branch then apply formatting tools to + # it. This will provide a target for rebasing which doesn't have any + # additional drift from changes to master. After that then we can rebase + # the re-written PR branch to the more current, autoformatted, master. + # TODO: It might be possible to skip the intermediate base branch. + git checkout -b tmp-master-rewrite-pr/$number `git merge-base origin/master pr/$number` + echo "$cmds" | tr ' ' '\n' | while read cmd; do + $cmd qutebrowser tests + git commit -am "dropme! $cmd" # mark commits for dropping when we rebase onto the more recent master + done + # Occasionally we get situations where black and pyupgrade build on each + # other to enable further changes. So the order you run them matters. But we + # have situations where each one enables the other, in both orders. So we + # run them all yet again to pick up any lingering changes from the first + # run. + # If we don't do this the leftover changes can be picked up by the smudge + # filter during the first rebase below and added to "fix lint" commits. Then + # since they don't have "dropme!" in the messages they stick in the branch + # and end up conflicting with the base branch. + echo "$cmds" | tr ' ' '\n' | while read cmd; do + $cmd qutebrowser tests + git commit -am "dropme! $cmd 2" + done + + git checkout -b ${prefix}pr/$number pr/$number + + add_smudge_filter "$cmds" # Description of extra options: # --exec 'git commit -qam "fix lint"': The git smudge filter leaves changes @@ -367,12 +438,12 @@ EOF # frustration. git rebase -q -X theirs -X renormalize --exec 'git commit -qam "fix lint" || true' tmp-master-rewrite-pr/$number exit_code="$?" + remove_smudge_filter [ $exit_code -eq 0 ] || { maybepause "rebase -X renormalize of ${prefix}pr/$number onto tmp-master-rewrite-pr/$number failed" git rebase --abort } git branch -D tmp-master-rewrite-pr/$number - rm .git/info/attributes [ $exit_code -eq 0 ] || return $exit_code From c0c09c0faacc308de3e94abe29174df975eed542 Mon Sep 17 00:00:00 2001 From: toofar Date: Tue, 3 Jan 2023 19:04:04 +1300 Subject: [PATCH 13/14] remove "all tools" prompt I'm more interested in refining the strategies to deal with adapting to whatever tools we choose to go with right now --- scripts/check_mergability.sh | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/scripts/check_mergability.sh b/scripts/check_mergability.sh index de6604e39..13da15da3 100755 --- a/scripts/check_mergability.sh +++ b/scripts/check_mergability.sh @@ -511,7 +511,7 @@ else # pre-defined auto-formatter configurations. Branches will be created as # needed. # format: branch tool1 tool2 ... - tools="master true + tools_all="master true tmp-black black tmp-black_isort black isort tmp-black_usort black usort @@ -519,9 +519,15 @@ else tmp-black_isort_pyupgrade black isort pyupgrade tmp-black_usort_pyupgrade black usort pyupgrade qt6-v2 true" - tools="tmp-black black" + tools="tmp-black_isort_pyupgrade black isort pyupgrade" - prompt_or_summary "Generate report for all tool configurations?" + if [ "$(echo "$tools" | wc -l | cut -d' ' -f1)" -gt 1 ] ;then + # TODO: turn this "run it with all tool configurations and see which one + # is the worst" thing into a CLI option. This script is a cross between + # "gather stats" and "refine merge strategies" now and is in need of a bit + # of a refactor. + prompt_or_summary "Generate report for all tool configurations?" + fi clean_branches echo "$tools" | while read branch cmds; do From 0dc6ff3bc3315a61dc7a4d7ec3f2f2b5e8e81fe4 Mon Sep 17 00:00:00 2001 From: toofar Date: Tue, 3 Jan 2023 19:05:45 +1300 Subject: [PATCH 14/14] Remove rebase related hacks I new there was something weird about these issues. They were due to formatting changes but the seemed like ones that should have been taken care of by the smudge filter. Turns out the issue was twofold: 1. there are some formatting changes that arise from running tools one after the other, so that was resolved by running them all twice, so they all get the change to run on each other's output 2. I think I typoed renormalize in my test script (s instead of z) which made me think the option either did nothing or was on by default. Much confusion. --- scripts/check_mergability.sh | 27 +-------------------------- 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/scripts/check_mergability.sh b/scripts/check_mergability.sh index 13da15da3..25c6e0587 100755 --- a/scripts/check_mergability.sh +++ b/scripts/check_mergability.sh @@ -411,32 +411,7 @@ rebase_with_formatting () { add_smudge_filter "$cmds" - # Description of extra options: - # --exec 'git commit -qam "fix lint"': The git smudge filter leaves changes - # in the working tree. So we need to include these in a commit if we - # want to keep them. - # --exec '... || true': git commit fails if there is nothing to commit, in - # this case meaning the filter didn't need to make any changes to the - # commit we just applied. So short circuiting to `true` just makes it so - # the result code is always 0 and the rebase continues. - # -X theirs: in the case of conflicts, disregard the changes in the working - # tree and apply those from the incoming commit. In this case when you - # have one commit later in a PR that builds on an earlier one, and we - # re-formatted the earlier one, the later one will fail to apply. Since - # we know these commits already build on each other and that any - # conflicts are due to formatting changes, which'll be applied again - # later we can safely disregard the changes. - # So this ends up with the right result but is problematic for two - # reason: - # a) it adds noise to the PRs because formatting changes are applied, - # reverted, then applied again. - # b) if there are any conflicts with the base branches the base branch - # changes will be reverted. In this script we are already checking - # that PRs apply cleanly to their existing base before rebasing them - # on an auto-formatted version of it. So we shouldn't run into that. - # But if this is used in more scenarios it will likely cause some - # frustration. - git rebase -q -X theirs -X renormalize --exec 'git commit -qam "fix lint" || true' tmp-master-rewrite-pr/$number + git rebase -q -X renormalize tmp-master-rewrite-pr/$number exit_code="$?" remove_smudge_filter [ $exit_code -eq 0 ] || {