Improve pre-push file list when merges are involved · Issue #860 · pre-commit/pre-commit · GitHub
Skip to content

Improve pre-push file list when merges are involved #860

Open
Open
Improve pre-push file list when merges are involved#860
Labels
feature

Description

@prem-nuro
prem-nuro
opened on Nov 6, 2018

If 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

Activity

  1. asottile commented on Nov 6, 2018

    @asottile
    asottile
    on Nov 6, 2018
    Member

    Hmmm, the three dots here are supposed to avoid merge commits.

    I'm also not sure:

    1. how this feature would be configured
      • top level configuration value?
      • refspec?
    2. 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 :)

  2. asottile
    added
    question
    on Nov 6, 2018

  3. prem-nuro commented on Nov 12, 2018

    @prem-nuro
    prem-nuro
    on Nov 12, 2018
    Author

    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.

  4. asottile commented on Nov 12, 2018

    @asottile
    asottile
    on Nov 12, 2018
    Member

    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?

  5. prem-nuro commented on Nov 12, 2018

    @prem-nuro
    prem-nuro
    on Nov 12, 2018
    Author

    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.

  6. asottile commented on Nov 12, 2018

    @asottile
    asottile
    on Nov 12, 2018
    Member

    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 HEAD

    output

    $ 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 <asottile@umich.edu>
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 <asottile@umich.edu>
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 <asottile@umich.edu>
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 -> bar

  7. asottile commented on Nov 12, 2018

    @asottile
    asottile
    on Nov 12, 2018
    Member

    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.

  8. asottile
    changed the title [-]Allow specifying good branch[/-] [+]Improve pre-push file list when merges are involved[/+] on Feb 21, 2019
  9. asottile
    added
    feature
    and removed
    question
    on Feb 21, 2019

  10. sluongng commented on Jun 17, 2022

    @sluongng
    sluongng
    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.

  11. sluongng commented on Jun 17, 2022

    @sluongng
    sluongng
    on Jun 17, 2022

    I think https://facebook.github.io/watchman/docs/scm-query.html described this class of diff calculation problem really well.
    Definitely worth a read.

  12. sluongng commented on Jun 17, 2022

    @sluongng
    sluongng
    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.

  13. Artalus commented on Apr 25, 2024

    @Artalus
    Artalus
    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.

  14. sarvi commented on Aug 28, 2024

    @sarvi
    sarvi
    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 ?

  15. 7 remaining items

  16. travisdowns commented on Oct 30, 2025

    @travisdowns
    travisdowns
    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 .

  17. travisdowns commented on Oct 30, 2025

    @travisdowns
    travisdowns
    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.

  18. asottile commented on Oct 30, 2025

    @asottile
    asottile
    on Oct 30, 2025
    Member

    that's already answered in the comments above

  19. travisdowns commented on Oct 30, 2025

    @travisdowns
    travisdowns
    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!

  20. asottile commented on Oct 30, 2025

    @asottile
    asottile
    on Oct 30, 2025
    Member

    keep reading! it's after that

  21. travisdowns commented on Oct 30, 2025

    @travisdowns
    travisdowns
    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.

  22. asottile commented on Oct 30, 2025

    @asottile
    asottile
    on Oct 30, 2025
    Member

    imo most things are "too slow" are better put in ci anyway

  23. travisdowns commented on Oct 30, 2025

    @travisdowns
    travisdowns
    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?

  24. morganhein commented on Nov 21, 2025

    @morganhein
    morganhein
    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?

  25. asottile commented on Nov 21, 2025

    @asottile
    asottile
    on Nov 21, 2025
    Member

    read the thread? I already did

  26. morganhein commented on Nov 21, 2025

    @morganhein
    morganhein
    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.

  27. asottile commented on Nov 21, 2025

    @asottile
    asottile
    on Nov 21, 2025
    Member

    I think you're projecting. this comment is clear as day what I want and maybe you just missed it #860 (comment)

  28. taliastocks commented on Jul 15, 2026

    @taliastocks
    taliastocks
    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.

  29. pre-commit
    deleted a comment from travisdowns on Jul 16, 2026
  30. pre-commit
    locked as off topic and limited conversation to collaborators on Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    feature

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions