GeezerVison is showing github.com — Improve pre-push file list when merges are involved · Issue #860 · pre-commit/pre-commit
Improve pre-push file list when merges are involved #860DescriptionIf I have a feature branch based on some old commit in develop or master, and I merge in develop or master, the pre-push hooks will run over all the merge commit files. Would it be possible to add a setting to provide a known "good" branch whose changes can be ignored? This is potentially useful as you gradually roll out a new check on a codebase, or if only some people have hooks installed.
A current workaround for this is by manually doing something like git diff $(git merge-base HEAD develop) --name-only [--diff-filter=ACMR?] in custom hooks, but I'd like to do this for other hooks I haven't written. The effect of this setting would just be to shrink the list of files provided to the hook, so their code wouldn't change.
Example repo: https://github.com/prem-nuro/precommit-issue-860 Reacted by Ian Chamberlain, Jamison Lahman, Paweł Żukowski, Yilei and Maxime Brunet Activityasottile commented on Nov 6, 2018 Hmmm, the three dots here are supposed to avoid merge commits.
I'm also not sure:
- how this feature would be configured
- top level configuration value?
- refspec?
- that it's a good idea
- even with a branch name, how would
pre-commit know where to generate the diff from?
- what if that branch doesn't exist locally?
- what if that branch does exist locally but is out of date?
- what if the fetched version of the remote branch is out of date?
- what if someone is pushing to that named branch?
and I really don't think I'd want to take on that complexity for a feature that would only be used very rarely (pre-push already doesn't see all that much use as it is). It would likely be broken very easily :(
So basically: convince me this is a problem worth fixing, convince me it'll not add significant complexity / special snowflakes, convince me it can't (easily) be done in hooks themselves :) prem-nuro commented on Nov 12, 2018 Regarding the current use of ..., I thought you'd need --no-merges or --first-parent or something.
Good points, here's my attempt at addressing them:
top level configuration value?
One option is a per-hook configuration: ignore_branch: [develop]
Any hooks already aware of special branches don't need them, but we can add them to hooks we haven't authored. I think a simple implementation supporting one special branch or tag is good enough for most use cases. And maybe this option could only have relevant for push stage hooks.
Another option is just top level overall.
refspec?
I think something simpler would be most convenient, matching no-commit-to-branch, hence just develop.
even with a branch name, how would pre-commit know where to generate the diff from?
We can simplify and just do git diff develop...foo. The diff will grow larger as time goes on, but that's okay. We're totally ignoring the current behavior here, which AFAIK is to just do the diff on un-pushed commits.
what if that branch doesn't exist locally?
Use the remote of the current branch's upstream. If it does exist locally, use the upstream version of that branch. Error if it doesn't exist in whatever remote we use.
what if that branch does exist locally but is out of date?
I think the ... notation will do the right thing here, if we use the remote upstream instead.
what if the fetched version of the remote branch is out of date?
Use the upstream version of the branch.
what if someone is pushing to that named branch?
If pushing to the branch, do the diff with whatever is upstream like origin/develop...develop. Special care for no upstream.
Summary:
|
Special branch is not local or remote |
Pushing to special branch which only exists locally |
Pushing to special branch |
Pushing to other branch |
| Diff computation |
Error |
Everything |
develop...origin/develop |
origin/develop...foo |
| Which remote/upstream |
Error |
None required |
Special branch remote/upstream |
Special branch remote/upstream if it exists locally. Else, assume it is the same name in current branch's remote. If that doesn't exist, error. |
And to reiterate, I think the use-case here is:
- We pre-push because we want to run a lot of hooks but not on every commit since they're all work in progress until we push.
- Everything's going to be merged into
develop eventually, which has varying levels of satisfying each hook as a new one is turned on. So we really care only about the diff with develop, such that any new code will satisfy hooks. This also helps us ignore things like rebasing on develop.
asottile commented on Nov 12, 2018 The way you describe it should already be the existing behaviour.
Since the commits in develop already exist on the remote, it should only notice the difference between those not pushed and those that you're pushing. Can you provide an example showing that not being the case? prem-nuro commented on Nov 12, 2018 I see. I linked an example repo above: https://github.com/prem-nuro/precommit-issue-860
master is the special branch and foo is the regular one. If you look at the entries in LOG you should be able to follow along, let me know if the entries aren't clear.
asottile commented on Nov 12, 2018 I had trouble following your example so I created a script:
script
#!/usr/bin/env bash
set -euxo pipefail
rm -rf upstream clone
git init upstream
git -C upstream commit --allow-empty -m 'Initial commit'
git -C upstream config receive.denyCurrentBranch ignore
git clone upstream clone
cd clone
cat > .pre-commit-config.yaml <<EOF
repos:
- repo: local
hooks:
- id: echo
name: echo
entry: echo
verbose: true
language: system
stages: [push]
EOF
git add .pre-commit-config.yaml
pre-commit install -t pre-push
git commit -m "Add push"
git push origin HEAD
touch master_only_file
git add master_only_file
git commit -m "Add master_only_file"
git push origin HEAD
git checkout origin/master^ -b bar
touch bar
git add bar
git commit -m "Add bar"
git merge origin/master --no-edit
git log --oneline --graph --decorate
: merge base
mb="$(git merge-base origin/master HEAD)"
git diff "${mb}" HEAD --name-only
: commits to push
git rev-list HEAD --topo-order --reverse --not --remotes=origin |
xargs git show
: source vs origin calculation
first_ancestor=$(
git rev-list HEAD --topo-order --reverse --not --remotes=origin |
head -1
)
source="$(git rev-parse "${first_ancestor}^")"
git show "${source}"
git diff --name-only "${source}...HEAD"
git push origin HEADoutput
$ bash t.sh
+ rm -rf upstream clone
+ git init upstream
Initialized empty Git repository in /tmp/t/upstream/.git/
+ git -C upstream commit --allow-empty -m 'Initial commit'
[master (root-commit) 50bebbc] Initial commit
+ git -C upstream config receive.denyCurrentBranch ignore
+ git clone upstream clone
Cloning into 'clone'...
done.
+ cd clone
+ cat
+ git add .pre-commit-config.yaml
+ pre-commit install -t pre-push
pre-commit installed at /tmp/t/clone/.git/hooks/pre-push
+ git commit -m 'Add push'
[master 625ef71] Add push
1 file changed, 9 insertions(+)
create mode 100644 .pre-commit-config.yaml
+ git push origin HEAD
echo.....................................................................Passed
hookid: echo
.pre-commit-config.yaml
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 354 bytes | 354.00 KiB/s, done.
Total 3 (delta 0), reused 0 (delta 0)
To /tmp/t/upstream
50bebbc..625ef71 HEAD -> master
+ touch master_only_file
+ git add master_only_file
+ git commit -m 'Add master_only_file'
[master bbcf4f2] Add master_only_file
1 file changed, 0 insertions(+), 0 deletions(-)
create mode 100644 master_only_file
+ git push origin HEAD
echo.....................................................................Passed
hookid: echo
master_only_file
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 306 bytes | 306.00 KiB/s, done.
Total 3 (delta 0), reused 0 (delta 0)
To /tmp/t/upstream
625ef71..bbcf4f2 HEAD -> master
+ git checkout 'origin/master^' -b bar
Switched to a new branch 'bar'
+ touch bar
+ git add bar
+ git commit -m 'Add bar'
[bar 90ef098] Add bar
1 file changed, 0 insertions(+), 0 deletions(-)
create mode 100644 bar
+ git merge origin/master --no-edit
Merge made by the 'recursive' strategy.
master_only_file | 0
1 file changed, 0 insertions(+), 0 deletions(-)
create mode 100644 master_only_file
+ git log --oneline --graph --decorate
* aabbc0a (HEAD -> bar) Merge remote-tracking branch 'origin/master' into bar
|\
| * bbcf4f2 (origin/master, origin/HEAD, master) Add master_only_file
* | 90ef098 Add bar
|/
* 625ef71 Add push
* 50bebbc Initial commit
+ : merge base
++ git merge-base origin/master HEAD
+ mb=bbcf4f2e695bf186b7af76f49ec288e068ad804e
+ git diff bbcf4f2e695bf186b7af76f49ec288e068ad804e HEAD --name-only
bar
+ : commits to push
+ git rev-list HEAD --topo-order --reverse --not --remotes=origin
+ xargs git show
commit 90ef098415b3069e676bde710e9885243b02b0bc
Author: Anthony Sottile <[email protected]>
Date: Mon Nov 12 11:20:31 2018 -0800
Add bar
diff --git a/bar b/bar
new file mode 100644
index 0000000..e69de29
commit aabbc0a9e4f1c3b60b0d93d4fd977efb8f8bf996 (HEAD -> bar)
Merge: 90ef098 bbcf4f2
Author: Anthony Sottile <[email protected]>
Date: Mon Nov 12 11:20:31 2018 -0800
Merge remote-tracking branch 'origin/master' into bar
+ : source vs origin calculation
++ head -1
++ git rev-list HEAD --topo-order --reverse --not --remotes=origin
+ first_ancestor=90ef098415b3069e676bde710e9885243b02b0bc
++ git rev-parse '90ef098415b3069e676bde710e9885243b02b0bc^'
+ source=625ef7124a9b348855de5c34caa7cbced08cf011
+ git show 625ef7124a9b348855de5c34caa7cbced08cf011
commit 625ef7124a9b348855de5c34caa7cbced08cf011
Author: Anthony Sottile <[email protected]>
Date: Mon Nov 12 11:20:30 2018 -0800
Add push
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
new file mode 100644
index 0000000..79a4a6f
--- /dev/null
+++ b/.pre-commit-config.yaml
@@ -0,0 +1,9 @@
+repos:
+- repo: local
+ hooks:
+ - id: echo
+ name: echo
+ entry: echo
+ verbose: true
+ language: system
+ stages: [push]
+ git diff --name-only 625ef7124a9b348855de5c34caa7cbced08cf011...HEAD
bar
master_only_file
+ git push origin HEAD
echo.....................................................................Passed
hookid: echo
bar master_only_file
Counting objects: 4, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (4/4), done.
Writing objects: 100% (4/4), 523 bytes | 523.00 KiB/s, done.
Total 4 (delta 1), reused 0 (delta 0)
To /tmp/t/upstream
* [new branch] HEAD -> barasottile commented on Nov 12, 2018 changed the title [-]Allow specifying good branch[/-] [+]Improve pre-push file list when merges are involved[/+] on Feb 21, 2019sluongng commented on Jun 17, 2022 #2424 was marked as duplicate of this, but the workflow I reported there does not involve merges but strictly rebase + force pushes.
If we do this at all, I don't want a configuration value. If you can find a way to improve the push routine or revision differencing to determine this I'd be happiest with that.
Internally I patched pre-commit in our monorepo with this
--- a/pre_commit/commands/hook_impl.py
+++ b/pre_commit/commands/hook_impl.py
@@ -117,14 +117,16 @@ def _pre_push_ns(
local_branch, local_sha, remote_branch, remote_sha = line.split()
if local_sha == Z40:
continue
- elif remote_sha != Z40 and _rev_exists(remote_sha):
- return _ns(
- 'pre-push', color,
- from_ref=remote_sha, to_ref=local_sha,
- remote_branch=remote_branch,
- local_branch=local_branch,
- remote_name=remote_name, remote_url=remote_url,
- )
+ # Irrelevant to monorepo workflow
+ #
+ # elif remote_sha != Z40 and _rev_exists(remote_sha):
+ # return _ns(
+ # 'pre-push', color,
+ # from_ref=remote_sha, to_ref=local_sha,
+ # remote_branch=remote_branch,
+ # local_branch=local_branch,
+ # remote_name=remote_name, remote_url=remote_url,
+ # )
else:
# ancestors not found in remote
ancestors = subprocess.check_output((
If current feature branch was updated because of a git rebase origin/master, we don't want our user have to pay attention to the files that was updated between origin/master@{1}..origin/master, but strictly only the files that was touched between the merge-base commit and feature-branch's HEAD.
So it makes no sense for us to run pre-commit for the diff between remote commit vs local commit in this workflow. sluongng commented on Jun 17, 2022 sluongng commented on Jun 17, 2022 solutioning:
I do recognize that this problem is workflow specific. So I think a top-level configuration to enable/disable this pre-push strategy, depending on workflow of the repository, would be much appreciated here. Reacted by Jamison Lahman, Gregory P. Smith and Travis Downs Reacted by anthony sottile Artalus commented on Apr 25, 2024 I would also vote for #2424 to be reopened, since the problems described in these two tickets are related to two different workflows, and the issue with rebase seems more addressable to me at least.
The fix suggested by sluongng makes sense to me and I don't think it needs any additional configuration. Contrary to the git merge case described by OP, with the git rebase workflow described in #2424 one essentially "grafts" the whole branch from one part of the "trunk" to another. There are no "ambigious" commits present both in the branch and the trunk, only the branching point, the branch head, and branched-out commits inbetween them.
With the current implementation from hook_impl.py, you get git diff <old branch head>...<new branch head> - this includes commits in the branch as well as commits in the trunk, same as in OP's issue with merges. But with sluongng's change applied you would get git diff <new branching point>...<new branch head>, same as if it was a fresh branch being pushed anew without remote to track. Note that this does not address the original problem with merges, as the merge commit is not present in the trunk and the changes introduced by it are indeed new to the branch.
I am actually curious if there are any cases at all, where one would prever the original behavior when pushing after rebase.
The only downside I see with the change discussed, is that instead of using a remote_sha provided to the hook from git itself, even for pushing regular commits there would be3 git rev-... invokes to parse the whole commit tree. For the record, the slowest part is git rev-list --max-parents=0; on my below-average laptop it takes ~15s in Linux repo (>1m commits) and ~1s in VSCode repo (100k commits). Considering people seem to use pre-push to run longer hooks already, that might be okay. sarvi commented on Aug 28, 2024 Needs some help. This thread is closest to the problem we are facing.
when trying to collect diffs --name-only to process during a "git push" command, we are using
git diff --name-only <HEADSHA-local-PR-branch>...<HEADSHA-remote-PR-branch>
This works fine until the local PR branch does a merge from its parent branch into the local PR branch.
When the try push the command shows tonnes of files from the parent branch sync.
which we dont want to see.
would prefer to focus on only files that have local changes in the PR branche between
<HEADSHA-local-PR-branch>...<HEADSHA-remote-PR-branch>
that will be pushed and exclude all the files from merge from parent.
Basically I only want a list files changed, unpushed commits on the local PR branch, without all the extra files I am currently seeing from the merge from parent
How do we do this ? travisdowns commented on Oct 30, 2025
We're running into this problem too. Would a decent compromise here be to treat a non-fastforward "force push" the same as pushing a new branch? I think that would fully solve the rebase problem.
It might solve it for users who push to a branch on the upstream (in order to submit a branch to branch PR within that same upstream), but not for users (like us) who push to a fork of upstream (in order to submit a repo to repo PR from our fork to upstream). As I understand it, the "new branch" logic essentially includes all the commits which aren't in the destination remote, which works well if you are pushing to upstream since it excludes all existing commits, but not if you are pushing to a fork, since many of the commits will be new on the fork for many pushes (every commit has to get into your fork, so you end up running pre-commit on every change on upstream eventually).
This is also described above in the comment from @pettermahlen . travisdowns commented on Oct 30, 2025
I think a better solution for this situation would be to be able to indicate the name of the main branch, and exclude any commits on that branch. I believe that could also solve @sarvi's problem with merges, and it seems to be the solution described in the 'watchman' link above.
@asottile would you be open to something like this? I imagine it like an (optional) configured "upstream tracking branch" or something like that, and if set the logic to find the rev range uses that as the LHS of the diff. So the change to the logic would be quite small. I think this is likely to work well for many workflows, including ones that fail with the current "new branch" heuristic. asottile commented on Oct 30, 2025 that's already answered in the comments above travisdowns commented on Oct 30, 2025 The most relevant one I saw was:
So basically: convince me this is a problem worth fixing, convince me it'll not add significant complexity / special snowflakes, convince me it can't (easily) be done in hooks themselves :)
So seems like you are on board as long as there is a reasonable approach and it's a problem worth fixing. I think it's clear this is worth fixing by now, just wanted to double check on the approach! asottile commented on Oct 30, 2025 keep reading! it's after that travisdowns commented on Oct 30, 2025 I found this, it may be what you are referring to:
If we do this at all, I don't want a configuration value. If you can find a way to improve the push routine or revision differencing to determine this I'd be happiest with that.
I think the discussion that follows shows it won't work, unless everyone happens to push their branches to the same repo.
Just to add some votes to the "is pre-push important" side of things: we are pretty interested in pre-push as some hooks are to slow to run on every commit and there is also a desire to allow some local commits during the development stage to break the rules, but then be fixed up prior to push, so those are both important use cases for pre-push. asottile commented on Oct 30, 2025 imo most things are "too slow" are better put in ci anyway travisdowns commented on Oct 30, 2025
imo most things are "too slow" are better put in ci anyway
It is also in CI, but too slow is 5-20 seconds, so folks would very reasonably like to find out locally about it but not slow down every commit by 5-20 seconds. The CI feedback loop in our case can take minutes and that's only if folks are on top of that, it might otherwise run one hour (using lots of $$) then require a re-push to fix the issue.
Of course, it's your project, so should I read between the lines that any approach that requires config is DOA here, or are we still in the "I can be convinced" stage? morganhein commented on Nov 21, 2025 The behavior where a rebase and force pushing to a branch causing all tasks to run is causing lots of friction in our setup. What needs to happen for this one edge-case to get resolved? I see there are patches that make pre-commit behave correctly in this one scenario. Can that patch be toggled on via the pre-commit config? What decisions need to happen to get this done? Someone please tell me there's a direction to fix this critical issue?
@asottile can you please just give some idea your blessing, and then let the masses finish it? asottile commented on Nov 21, 2025 read the thread? I already did morganhein commented on Nov 21, 2025 It seems to me you said that if we could find a way to have better detection as opposed to a config, you'd prefer that. It hasn't happened in SEVEN YEARS, so that says to me "we can't do that". So instead of just being passive aggressive for those trying to solve their immediate problems, i'm trying to find a solution that will actually help people. asottile commented on Nov 21, 2025 I think you're projecting. this comment is clear as day what I want and maybe you just missed it #860 (comment) taliastocks commented on Jul 15, 2026
If we do this at all, I don't want a configuration value. If you can find a way to improve the push routine or revision differencing to determine this I'd be happiest with that.
#3427 was an improvement to just the push routine and didn't require any additional config. locked as off topic and limited conversation to collaborators on Jul 16, 2026
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|