Fix basic static checks comparing same commit with itself - #60194

Closed
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting
Closed

Fix basic static checks comparing same commit with itself#60194
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting

Conversation

@jedcunningham

Copy link
Copy Markdown
Member

The prek command was comparing github.sha with itself, which checked no files. Now uses merge-base to check all files changed in the PR against the base branch.

This ensures static checks are actually ran in PRs like #60168.

The prek command was comparing `github.sha` with itself, which checked
no files. Now uses `merge-base` to check all files changed in the PR
against the base branch.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a bug in the basic-tests workflow where static checks were comparing the same commit (github.sha) with itself, resulting in no files being checked. The fix introduces a merge-base calculation to properly identify all changed files in a PR by comparing against the base branch.

Key changes:

  • Implements merge-base calculation to find the common ancestor between PR and base branch
  • Updates static check command to compare merge-base commit against HEAD instead of comparing github.sha with itself
  • Increases fetch depth and adds fallback logic to handle cases where merge-base isn't found in shallow clones

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread.github/workflows/basic-tests.yml
Comment thread.github/workflows/basic-tests.yml
Comment on lines +231 to +245
git fetch origin "$BASE_BRANCH" --depth=50

# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")

if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi

echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
git fetch origin "$BASE_BRANCH" --depth=50
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
git fetch origin "$BASE_BRANCH" --depth=$FETCH_DEPTH
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=$FETCH_DEPTH, fetching more history..."
git fetch --deepen=$DEEPEN
git fetch origin "$BASE_BRANCH" --deepen=$DEEPEN
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
FETCH_DEPTH: 50
DEEPEN: 450

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, maybe we can make the values parameterised for both --depth and --deepen from env :)

@amoghrajeshamoghrajesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, thats a strange one

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange one .. I wonder when it happened and how it did not work (I am sure it worked before). But it can be done way simpler.

All you need it is to add missing ^ in --from-ref

At the moment you do this check - github.sha already contains merge commit - this is how pull-request workflow works.

https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request

Note that GITHUB_SHA for this event is the last merge commit of the pull request merge branch. If you want to get the commit ID for the last commit to the head branch of the pull request, use github.event.pull_request.head.sha instead.

@potiuk

Copy link
Copy Markdown
Member

Superseded by #60202

@potiukpotiuk closed this Jan 7, 2026
@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

This is precisely why we needed 2 depth - because in pull_request the github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased).

@potiuk

Copy link
Copy Markdown
Member

I merged my fix and rebased #60168 to verify that it works (it should fail)

@potiuk

Copy link
Copy Markdown
Member

Nice catch @jedcunningham BTW :)

@potiuk

Copy link
Copy Markdown
Member

@jedcunningham

jedcunningham commented Jan 7, 2026

Copy link
Copy Markdown
MemberAuthor

Nice, I'd considered just adding "^" but somehow I confused myself thinking that would only go 1 commit up - that if we had many commits on the branch it'd only look at the last one. I guess it must squash? Anyways, glad its working.

edit:

github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased)

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

@jedcunningham
jedcunningham deleted the fix_inthewild_sorting branch January 7, 2026 18:52
@potiuk

Copy link
Copy Markdown
Member

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

Correct. This is actually what we very heavily bank on (this why I knew it by heart).
Basically whole selective checks code is based on the fact that we have "merge" commit passed as github.sha and it's first parent is base branch.

https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/commands/ci_commands.py#L163:

defget_changed_files(commit_ref: str|None) ->tuple[str, ...]:
ifcommit_refisNone:
return ()
cmd= [
"git",
"diff-tree",
"--no-commit-id",
"--name-only",
"-r",
commit_ref+"^",
commit_ref,
]
result=run_command(cmd, check=False, capture_output=True, text=True)
ifresult.returncode!=0:
get_console().print(
f"[warning] Error when running diff-tree command [/]\n{result.stdout}\n{result.stderr}"
)
return ()
changed_files=tuple(result.stdout.splitlines()) ifresult.stdoutelse ()
get_console().print("\n[info]Changed files:[/]\n")
get_console().print(changed_files)
get_console().print()
returnchanged_files

@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

BTW. There is also another interesting thing I learned with prek

There is a very nice shortcut to not have to find merge-base.

  • git diff main..your_pr_branch
  • git diff main...your_pr_branch

Spot the difference .... there are THREE dots in second case. And the ... is actually showing the changes in your branch from the merge-base

And ... (pun intended) there are few other gotchas: https://darekkay.com/blog/git-commit-ranges/#git-diff

For example git diff main...your-branch is not reverse of git diff your-branch...diff as you would suspect. The first one shows your changes since merge-base, the second shows SURPRISE -> all the changes in main (!) since merge-base 😱 😱 😱 😱 😱 😱 😱 😱 😱

Git is wild.

@potiuk

Copy link
Copy Markdown
Member

So basically if you want to have prek to execute on "only your changes" from a branch you have PR to main with - do this (without running any of git merge-base thingies):

prek --from-ref maim

(we have it in our docs)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jedcunningham@potiuk@bugraoz93@amoghrajesh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fix basic static checks comparing same commit with itself - #60194

Closed
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting
Closed

Fix basic static checks comparing same commit with itself#60194
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting

Conversation

@jedcunningham

Copy link
Copy Markdown
Member

The prek command was comparing github.sha with itself, which checked no files. Now uses merge-base to check all files changed in the PR against the base branch.

This ensures static checks are actually ran in PRs like #60168.

The prek command was comparing `github.sha` with itself, which checked
no files. Now uses `merge-base` to check all files changed in the PR
against the base branch.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a bug in the basic-tests workflow where static checks were comparing the same commit (github.sha) with itself, resulting in no files being checked. The fix introduces a merge-base calculation to properly identify all changed files in a PR by comparing against the base branch.

Key changes:

  • Implements merge-base calculation to find the common ancestor between PR and base branch
  • Updates static check command to compare merge-base commit against HEAD instead of comparing github.sha with itself
  • Increases fetch depth and adds fallback logic to handle cases where merge-base isn't found in shallow clones

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread.github/workflows/basic-tests.yml
Comment thread.github/workflows/basic-tests.yml
Comment on lines +231 to +245
git fetch origin "$BASE_BRANCH" --depth=50

# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")

if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi

echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
git fetch origin "$BASE_BRANCH" --depth=50
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
git fetch origin "$BASE_BRANCH" --depth=$FETCH_DEPTH
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=$FETCH_DEPTH, fetching more history..."
git fetch --deepen=$DEEPEN
git fetch origin "$BASE_BRANCH" --deepen=$DEEPEN
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
FETCH_DEPTH: 50
DEEPEN: 450

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, maybe we can make the values parameterised for both --depth and --deepen from env :)

@amoghrajeshamoghrajesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, thats a strange one

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange one .. I wonder when it happened and how it did not work (I am sure it worked before). But it can be done way simpler.

All you need it is to add missing ^ in --from-ref

At the moment you do this check - github.sha already contains merge commit - this is how pull-request workflow works.

https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request

Note that GITHUB_SHA for this event is the last merge commit of the pull request merge branch. If you want to get the commit ID for the last commit to the head branch of the pull request, use github.event.pull_request.head.sha instead.

@potiuk

Copy link
Copy Markdown
Member

Superseded by #60202

@potiukpotiuk closed this Jan 7, 2026
@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

This is precisely why we needed 2 depth - because in pull_request the github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased).

@potiuk

Copy link
Copy Markdown
Member

I merged my fix and rebased #60168 to verify that it works (it should fail)

@potiuk

Copy link
Copy Markdown
Member

Nice catch @jedcunningham BTW :)

@potiuk

Copy link
Copy Markdown
Member

@jedcunningham

jedcunningham commented Jan 7, 2026

Copy link
Copy Markdown
MemberAuthor

Nice, I'd considered just adding "^" but somehow I confused myself thinking that would only go 1 commit up - that if we had many commits on the branch it'd only look at the last one. I guess it must squash? Anyways, glad its working.

edit:

github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased)

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

@jedcunningham
jedcunningham deleted the fix_inthewild_sorting branch January 7, 2026 18:52
@potiuk

Copy link
Copy Markdown
Member

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

Correct. This is actually what we very heavily bank on (this why I knew it by heart).
Basically whole selective checks code is based on the fact that we have "merge" commit passed as github.sha and it's first parent is base branch.

https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/commands/ci_commands.py#L163:

defget_changed_files(commit_ref: str|None) ->tuple[str, ...]:
ifcommit_refisNone:
return ()
cmd= [
"git",
"diff-tree",
"--no-commit-id",
"--name-only",
"-r",
commit_ref+"^",
commit_ref,
]
result=run_command(cmd, check=False, capture_output=True, text=True)
ifresult.returncode!=0:
get_console().print(
f"[warning] Error when running diff-tree command [/]\n{result.stdout}\n{result.stderr}"
)
return ()
changed_files=tuple(result.stdout.splitlines()) ifresult.stdoutelse ()
get_console().print("\n[info]Changed files:[/]\n")
get_console().print(changed_files)
get_console().print()
returnchanged_files

@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

BTW. There is also another interesting thing I learned with prek

There is a very nice shortcut to not have to find merge-base.

  • git diff main..your_pr_branch
  • git diff main...your_pr_branch

Spot the difference .... there are THREE dots in second case. And the ... is actually showing the changes in your branch from the merge-base

And ... (pun intended) there are few other gotchas: https://darekkay.com/blog/git-commit-ranges/#git-diff

For example git diff main...your-branch is not reverse of git diff your-branch...diff as you would suspect. The first one shows your changes since merge-base, the second shows SURPRISE -> all the changes in main (!) since merge-base 😱 😱 😱 😱 😱 😱 😱 😱 😱

Git is wild.

@potiuk

Copy link
Copy Markdown
Member

So basically if you want to have prek to execute on "only your changes" from a branch you have PR to main with - do this (without running any of git merge-base thingies):

prek --from-ref maim

(we have it in our docs)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jedcunningham@potiuk@bugraoz93@amoghrajesh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix basic static checks comparing same commit with itself - #60194

Closed
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting
Closed

Fix basic static checks comparing same commit with itself#60194
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting

Conversation

@jedcunningham

Copy link
Copy Markdown
Member

The prek command was comparing github.sha with itself, which checked no files. Now uses merge-base to check all files changed in the PR against the base branch.

This ensures static checks are actually ran in PRs like #60168.

The prek command was comparing `github.sha` with itself, which checked
no files. Now uses `merge-base` to check all files changed in the PR
against the base branch.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a bug in the basic-tests workflow where static checks were comparing the same commit (github.sha) with itself, resulting in no files being checked. The fix introduces a merge-base calculation to properly identify all changed files in a PR by comparing against the base branch.

Key changes:

  • Implements merge-base calculation to find the common ancestor between PR and base branch
  • Updates static check command to compare merge-base commit against HEAD instead of comparing github.sha with itself
  • Increases fetch depth and adds fallback logic to handle cases where merge-base isn't found in shallow clones

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread.github/workflows/basic-tests.yml
Comment thread.github/workflows/basic-tests.yml
Comment on lines +231 to +245
git fetch origin "$BASE_BRANCH" --depth=50

# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")

if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi

echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
git fetch origin "$BASE_BRANCH" --depth=50
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
git fetch origin "$BASE_BRANCH" --depth=$FETCH_DEPTH
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=$FETCH_DEPTH, fetching more history..."
git fetch --deepen=$DEEPEN
git fetch origin "$BASE_BRANCH" --deepen=$DEEPEN
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
FETCH_DEPTH: 50
DEEPEN: 450

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, maybe we can make the values parameterised for both --depth and --deepen from env :)

@amoghrajeshamoghrajesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, thats a strange one

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange one .. I wonder when it happened and how it did not work (I am sure it worked before). But it can be done way simpler.

All you need it is to add missing ^ in --from-ref

At the moment you do this check - github.sha already contains merge commit - this is how pull-request workflow works.

https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request

Note that GITHUB_SHA for this event is the last merge commit of the pull request merge branch. If you want to get the commit ID for the last commit to the head branch of the pull request, use github.event.pull_request.head.sha instead.

@potiuk

Copy link
Copy Markdown
Member

Superseded by #60202

@potiukpotiuk closed this Jan 7, 2026
@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

This is precisely why we needed 2 depth - because in pull_request the github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased).

@potiuk

Copy link
Copy Markdown
Member

I merged my fix and rebased #60168 to verify that it works (it should fail)

@potiuk

Copy link
Copy Markdown
Member

Nice catch @jedcunningham BTW :)

@potiuk

Copy link
Copy Markdown
Member

@jedcunningham

jedcunningham commented Jan 7, 2026

Copy link
Copy Markdown
MemberAuthor

Nice, I'd considered just adding "^" but somehow I confused myself thinking that would only go 1 commit up - that if we had many commits on the branch it'd only look at the last one. I guess it must squash? Anyways, glad its working.

edit:

github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased)

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

@jedcunningham
jedcunningham deleted the fix_inthewild_sorting branch January 7, 2026 18:52
@potiuk

Copy link
Copy Markdown
Member

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

Correct. This is actually what we very heavily bank on (this why I knew it by heart).
Basically whole selective checks code is based on the fact that we have "merge" commit passed as github.sha and it's first parent is base branch.

https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/commands/ci_commands.py#L163:

defget_changed_files(commit_ref: str|None) ->tuple[str, ...]:
ifcommit_refisNone:
return ()
cmd= [
"git",
"diff-tree",
"--no-commit-id",
"--name-only",
"-r",
commit_ref+"^",
commit_ref,
]
result=run_command(cmd, check=False, capture_output=True, text=True)
ifresult.returncode!=0:
get_console().print(
f"[warning] Error when running diff-tree command [/]\n{result.stdout}\n{result.stderr}"
)
return ()
changed_files=tuple(result.stdout.splitlines()) ifresult.stdoutelse ()
get_console().print("\n[info]Changed files:[/]\n")
get_console().print(changed_files)
get_console().print()
returnchanged_files

@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

BTW. There is also another interesting thing I learned with prek

There is a very nice shortcut to not have to find merge-base.

  • git diff main..your_pr_branch
  • git diff main...your_pr_branch

Spot the difference .... there are THREE dots in second case. And the ... is actually showing the changes in your branch from the merge-base

And ... (pun intended) there are few other gotchas: https://darekkay.com/blog/git-commit-ranges/#git-diff

For example git diff main...your-branch is not reverse of git diff your-branch...diff as you would suspect. The first one shows your changes since merge-base, the second shows SURPRISE -> all the changes in main (!) since merge-base 😱 😱 😱 😱 😱 😱 😱 😱 😱

Git is wild.

@potiuk

Copy link
Copy Markdown
Member

So basically if you want to have prek to execute on "only your changes" from a branch you have PR to main with - do this (without running any of git merge-base thingies):

prek --from-ref maim

(we have it in our docs)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jedcunningham@potiuk@bugraoz93@amoghrajesh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix basic static checks comparing same commit with itself - #60194

Closed
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting
Closed

Fix basic static checks comparing same commit with itself#60194
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting

Conversation

@jedcunningham

Copy link
Copy Markdown
Member

The prek command was comparing github.sha with itself, which checked no files. Now uses merge-base to check all files changed in the PR against the base branch.

This ensures static checks are actually ran in PRs like #60168.

The prek command was comparing `github.sha` with itself, which checked
no files. Now uses `merge-base` to check all files changed in the PR
against the base branch.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a bug in the basic-tests workflow where static checks were comparing the same commit (github.sha) with itself, resulting in no files being checked. The fix introduces a merge-base calculation to properly identify all changed files in a PR by comparing against the base branch.

Key changes:

  • Implements merge-base calculation to find the common ancestor between PR and base branch
  • Updates static check command to compare merge-base commit against HEAD instead of comparing github.sha with itself
  • Increases fetch depth and adds fallback logic to handle cases where merge-base isn't found in shallow clones

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread.github/workflows/basic-tests.yml
Comment thread.github/workflows/basic-tests.yml
Comment on lines +231 to +245
git fetch origin "$BASE_BRANCH" --depth=50

# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")

if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi

echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
git fetch origin "$BASE_BRANCH" --depth=50
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
git fetch origin "$BASE_BRANCH" --depth=$FETCH_DEPTH
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=$FETCH_DEPTH, fetching more history..."
git fetch --deepen=$DEEPEN
git fetch origin "$BASE_BRANCH" --deepen=$DEEPEN
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
FETCH_DEPTH: 50
DEEPEN: 450

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, maybe we can make the values parameterised for both --depth and --deepen from env :)

@amoghrajeshamoghrajesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, thats a strange one

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange one .. I wonder when it happened and how it did not work (I am sure it worked before). But it can be done way simpler.

All you need it is to add missing ^ in --from-ref

At the moment you do this check - github.sha already contains merge commit - this is how pull-request workflow works.

https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request

Note that GITHUB_SHA for this event is the last merge commit of the pull request merge branch. If you want to get the commit ID for the last commit to the head branch of the pull request, use github.event.pull_request.head.sha instead.

@potiuk

Copy link
Copy Markdown
Member

Superseded by #60202

@potiukpotiuk closed this Jan 7, 2026
@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

This is precisely why we needed 2 depth - because in pull_request the github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased).

@potiuk

Copy link
Copy Markdown
Member

I merged my fix and rebased #60168 to verify that it works (it should fail)

@potiuk

Copy link
Copy Markdown
Member

Nice catch @jedcunningham BTW :)

@potiuk

Copy link
Copy Markdown
Member

@jedcunningham

jedcunningham commented Jan 7, 2026

Copy link
Copy Markdown
MemberAuthor

Nice, I'd considered just adding "^" but somehow I confused myself thinking that would only go 1 commit up - that if we had many commits on the branch it'd only look at the last one. I guess it must squash? Anyways, glad its working.

edit:

github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased)

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

@jedcunningham
jedcunningham deleted the fix_inthewild_sorting branch January 7, 2026 18:52
@potiuk

Copy link
Copy Markdown
Member

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

Correct. This is actually what we very heavily bank on (this why I knew it by heart).
Basically whole selective checks code is based on the fact that we have "merge" commit passed as github.sha and it's first parent is base branch.

https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/commands/ci_commands.py#L163:

defget_changed_files(commit_ref: str|None) ->tuple[str, ...]:
ifcommit_refisNone:
return ()
cmd= [
"git",
"diff-tree",
"--no-commit-id",
"--name-only",
"-r",
commit_ref+"^",
commit_ref,
]
result=run_command(cmd, check=False, capture_output=True, text=True)
ifresult.returncode!=0:
get_console().print(
f"[warning] Error when running diff-tree command [/]\n{result.stdout}\n{result.stderr}"
)
return ()
changed_files=tuple(result.stdout.splitlines()) ifresult.stdoutelse ()
get_console().print("\n[info]Changed files:[/]\n")
get_console().print(changed_files)
get_console().print()
returnchanged_files

@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

BTW. There is also another interesting thing I learned with prek

There is a very nice shortcut to not have to find merge-base.

  • git diff main..your_pr_branch
  • git diff main...your_pr_branch

Spot the difference .... there are THREE dots in second case. And the ... is actually showing the changes in your branch from the merge-base

And ... (pun intended) there are few other gotchas: https://darekkay.com/blog/git-commit-ranges/#git-diff

For example git diff main...your-branch is not reverse of git diff your-branch...diff as you would suspect. The first one shows your changes since merge-base, the second shows SURPRISE -> all the changes in main (!) since merge-base 😱 😱 😱 😱 😱 😱 😱 😱 😱

Git is wild.

@potiuk

Copy link
Copy Markdown
Member

So basically if you want to have prek to execute on "only your changes" from a branch you have PR to main with - do this (without running any of git merge-base thingies):

prek --from-ref maim

(we have it in our docs)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jedcunningham@potiuk@bugraoz93@amoghrajesh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fix basic static checks comparing same commit with itself - #60194

Closed
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting
Closed

Fix basic static checks comparing same commit with itself#60194
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting

Conversation

@jedcunningham

Copy link
Copy Markdown
Member

The prek command was comparing github.sha with itself, which checked no files. Now uses merge-base to check all files changed in the PR against the base branch.

This ensures static checks are actually ran in PRs like #60168.

The prek command was comparing `github.sha` with itself, which checked
no files. Now uses `merge-base` to check all files changed in the PR
against the base branch.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a bug in the basic-tests workflow where static checks were comparing the same commit (github.sha) with itself, resulting in no files being checked. The fix introduces a merge-base calculation to properly identify all changed files in a PR by comparing against the base branch.

Key changes:

  • Implements merge-base calculation to find the common ancestor between PR and base branch
  • Updates static check command to compare merge-base commit against HEAD instead of comparing github.sha with itself
  • Increases fetch depth and adds fallback logic to handle cases where merge-base isn't found in shallow clones

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread.github/workflows/basic-tests.yml
Comment thread.github/workflows/basic-tests.yml
Comment on lines +231 to +245
git fetch origin "$BASE_BRANCH" --depth=50

# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")

if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi

echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
git fetch origin "$BASE_BRANCH" --depth=50
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
git fetch origin "$BASE_BRANCH" --depth=$FETCH_DEPTH
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=$FETCH_DEPTH, fetching more history..."
git fetch --deepen=$DEEPEN
git fetch origin "$BASE_BRANCH" --deepen=$DEEPEN
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
FETCH_DEPTH: 50
DEEPEN: 450

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, maybe we can make the values parameterised for both --depth and --deepen from env :)

@amoghrajeshamoghrajesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, thats a strange one

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange one .. I wonder when it happened and how it did not work (I am sure it worked before). But it can be done way simpler.

All you need it is to add missing ^ in --from-ref

At the moment you do this check - github.sha already contains merge commit - this is how pull-request workflow works.

https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request

Note that GITHUB_SHA for this event is the last merge commit of the pull request merge branch. If you want to get the commit ID for the last commit to the head branch of the pull request, use github.event.pull_request.head.sha instead.

@potiuk

Copy link
Copy Markdown
Member

Superseded by #60202

@potiukpotiuk closed this Jan 7, 2026
@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

This is precisely why we needed 2 depth - because in pull_request the github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased).

@potiuk

Copy link
Copy Markdown
Member

I merged my fix and rebased #60168 to verify that it works (it should fail)

@potiuk

Copy link
Copy Markdown
Member

Nice catch @jedcunningham BTW :)

@potiuk

Copy link
Copy Markdown
Member

@jedcunningham

jedcunningham commented Jan 7, 2026

Copy link
Copy Markdown
MemberAuthor

Nice, I'd considered just adding "^" but somehow I confused myself thinking that would only go 1 commit up - that if we had many commits on the branch it'd only look at the last one. I guess it must squash? Anyways, glad its working.

edit:

github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased)

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

@jedcunningham
jedcunningham deleted the fix_inthewild_sorting branch January 7, 2026 18:52
@potiuk

Copy link
Copy Markdown
Member

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

Correct. This is actually what we very heavily bank on (this why I knew it by heart).
Basically whole selective checks code is based on the fact that we have "merge" commit passed as github.sha and it's first parent is base branch.

https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/commands/ci_commands.py#L163:

defget_changed_files(commit_ref: str|None) ->tuple[str, ...]:
ifcommit_refisNone:
return ()
cmd= [
"git",
"diff-tree",
"--no-commit-id",
"--name-only",
"-r",
commit_ref+"^",
commit_ref,
]
result=run_command(cmd, check=False, capture_output=True, text=True)
ifresult.returncode!=0:
get_console().print(
f"[warning] Error when running diff-tree command [/]\n{result.stdout}\n{result.stderr}"
)
return ()
changed_files=tuple(result.stdout.splitlines()) ifresult.stdoutelse ()
get_console().print("\n[info]Changed files:[/]\n")
get_console().print(changed_files)
get_console().print()
returnchanged_files

@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

BTW. There is also another interesting thing I learned with prek

There is a very nice shortcut to not have to find merge-base.

  • git diff main..your_pr_branch
  • git diff main...your_pr_branch

Spot the difference .... there are THREE dots in second case. And the ... is actually showing the changes in your branch from the merge-base

And ... (pun intended) there are few other gotchas: https://darekkay.com/blog/git-commit-ranges/#git-diff

For example git diff main...your-branch is not reverse of git diff your-branch...diff as you would suspect. The first one shows your changes since merge-base, the second shows SURPRISE -> all the changes in main (!) since merge-base 😱 😱 😱 😱 😱 😱 😱 😱 😱

Git is wild.

@potiuk

Copy link
Copy Markdown
Member

So basically if you want to have prek to execute on "only your changes" from a branch you have PR to main with - do this (without running any of git merge-base thingies):

prek --from-ref maim

(we have it in our docs)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jedcunningham@potiuk@bugraoz93@amoghrajesh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix basic static checks comparing same commit with itself - #60194

Closed
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting
Closed

Fix basic static checks comparing same commit with itself#60194
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting

Conversation

@jedcunningham

Copy link
Copy Markdown
Member

The prek command was comparing github.sha with itself, which checked no files. Now uses merge-base to check all files changed in the PR against the base branch.

This ensures static checks are actually ran in PRs like #60168.

The prek command was comparing `github.sha` with itself, which checked
no files. Now uses `merge-base` to check all files changed in the PR
against the base branch.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a bug in the basic-tests workflow where static checks were comparing the same commit (github.sha) with itself, resulting in no files being checked. The fix introduces a merge-base calculation to properly identify all changed files in a PR by comparing against the base branch.

Key changes:

  • Implements merge-base calculation to find the common ancestor between PR and base branch
  • Updates static check command to compare merge-base commit against HEAD instead of comparing github.sha with itself
  • Increases fetch depth and adds fallback logic to handle cases where merge-base isn't found in shallow clones

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread.github/workflows/basic-tests.yml
Comment thread.github/workflows/basic-tests.yml
Comment on lines +231 to +245
git fetch origin "$BASE_BRANCH" --depth=50

# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")

if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi

echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
git fetch origin "$BASE_BRANCH" --depth=50
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
git fetch origin "$BASE_BRANCH" --depth=$FETCH_DEPTH
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=$FETCH_DEPTH, fetching more history..."
git fetch --deepen=$DEEPEN
git fetch origin "$BASE_BRANCH" --deepen=$DEEPEN
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
FETCH_DEPTH: 50
DEEPEN: 450

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, maybe we can make the values parameterised for both --depth and --deepen from env :)

@amoghrajeshamoghrajesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, thats a strange one

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange one .. I wonder when it happened and how it did not work (I am sure it worked before). But it can be done way simpler.

All you need it is to add missing ^ in --from-ref

At the moment you do this check - github.sha already contains merge commit - this is how pull-request workflow works.

https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request

Note that GITHUB_SHA for this event is the last merge commit of the pull request merge branch. If you want to get the commit ID for the last commit to the head branch of the pull request, use github.event.pull_request.head.sha instead.

@potiuk

Copy link
Copy Markdown
Member

Superseded by #60202

@potiukpotiuk closed this Jan 7, 2026
@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

This is precisely why we needed 2 depth - because in pull_request the github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased).

@potiuk

Copy link
Copy Markdown
Member

I merged my fix and rebased #60168 to verify that it works (it should fail)

@potiuk

Copy link
Copy Markdown
Member

Nice catch @jedcunningham BTW :)

@potiuk

Copy link
Copy Markdown
Member

@jedcunningham

jedcunningham commented Jan 7, 2026

Copy link
Copy Markdown
MemberAuthor

Nice, I'd considered just adding "^" but somehow I confused myself thinking that would only go 1 commit up - that if we had many commits on the branch it'd only look at the last one. I guess it must squash? Anyways, glad its working.

edit:

github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased)

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

@jedcunningham
jedcunningham deleted the fix_inthewild_sorting branch January 7, 2026 18:52
@potiuk

Copy link
Copy Markdown
Member

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

Correct. This is actually what we very heavily bank on (this why I knew it by heart).
Basically whole selective checks code is based on the fact that we have "merge" commit passed as github.sha and it's first parent is base branch.

https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/commands/ci_commands.py#L163:

defget_changed_files(commit_ref: str|None) ->tuple[str, ...]:
ifcommit_refisNone:
return ()
cmd= [
"git",
"diff-tree",
"--no-commit-id",
"--name-only",
"-r",
commit_ref+"^",
commit_ref,
]
result=run_command(cmd, check=False, capture_output=True, text=True)
ifresult.returncode!=0:
get_console().print(
f"[warning] Error when running diff-tree command [/]\n{result.stdout}\n{result.stderr}"
)
return ()
changed_files=tuple(result.stdout.splitlines()) ifresult.stdoutelse ()
get_console().print("\n[info]Changed files:[/]\n")
get_console().print(changed_files)
get_console().print()
returnchanged_files

@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

BTW. There is also another interesting thing I learned with prek

There is a very nice shortcut to not have to find merge-base.

  • git diff main..your_pr_branch
  • git diff main...your_pr_branch

Spot the difference .... there are THREE dots in second case. And the ... is actually showing the changes in your branch from the merge-base

And ... (pun intended) there are few other gotchas: https://darekkay.com/blog/git-commit-ranges/#git-diff

For example git diff main...your-branch is not reverse of git diff your-branch...diff as you would suspect. The first one shows your changes since merge-base, the second shows SURPRISE -> all the changes in main (!) since merge-base 😱 😱 😱 😱 😱 😱 😱 😱 😱

Git is wild.

@potiuk

Copy link
Copy Markdown
Member

So basically if you want to have prek to execute on "only your changes" from a branch you have PR to main with - do this (without running any of git merge-base thingies):

prek --from-ref maim

(we have it in our docs)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jedcunningham@potiuk@bugraoz93@amoghrajesh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix basic static checks comparing same commit with itself - #60194

Closed
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting
Closed

Fix basic static checks comparing same commit with itself#60194
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting

Conversation

@jedcunningham

Copy link
Copy Markdown
Member

The prek command was comparing github.sha with itself, which checked no files. Now uses merge-base to check all files changed in the PR against the base branch.

This ensures static checks are actually ran in PRs like #60168.

The prek command was comparing `github.sha` with itself, which checked
no files. Now uses `merge-base` to check all files changed in the PR
against the base branch.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a bug in the basic-tests workflow where static checks were comparing the same commit (github.sha) with itself, resulting in no files being checked. The fix introduces a merge-base calculation to properly identify all changed files in a PR by comparing against the base branch.

Key changes:

  • Implements merge-base calculation to find the common ancestor between PR and base branch
  • Updates static check command to compare merge-base commit against HEAD instead of comparing github.sha with itself
  • Increases fetch depth and adds fallback logic to handle cases where merge-base isn't found in shallow clones

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread.github/workflows/basic-tests.yml
Comment thread.github/workflows/basic-tests.yml
Comment on lines +231 to +245
git fetch origin "$BASE_BRANCH" --depth=50

# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")

if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi

echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
git fetch origin "$BASE_BRANCH" --depth=50
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
git fetch origin "$BASE_BRANCH" --depth=$FETCH_DEPTH
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=$FETCH_DEPTH, fetching more history..."
git fetch --deepen=$DEEPEN
git fetch origin "$BASE_BRANCH" --deepen=$DEEPEN
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
FETCH_DEPTH: 50
DEEPEN: 450

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, maybe we can make the values parameterised for both --depth and --deepen from env :)

@amoghrajeshamoghrajesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, thats a strange one

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange one .. I wonder when it happened and how it did not work (I am sure it worked before). But it can be done way simpler.

All you need it is to add missing ^ in --from-ref

At the moment you do this check - github.sha already contains merge commit - this is how pull-request workflow works.

https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request

Note that GITHUB_SHA for this event is the last merge commit of the pull request merge branch. If you want to get the commit ID for the last commit to the head branch of the pull request, use github.event.pull_request.head.sha instead.

@potiuk

Copy link
Copy Markdown
Member

Superseded by #60202

@potiukpotiuk closed this Jan 7, 2026
@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

This is precisely why we needed 2 depth - because in pull_request the github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased).

@potiuk

Copy link
Copy Markdown
Member

I merged my fix and rebased #60168 to verify that it works (it should fail)

@potiuk

Copy link
Copy Markdown
Member

Nice catch @jedcunningham BTW :)

@potiuk

Copy link
Copy Markdown
Member

@jedcunningham

jedcunningham commented Jan 7, 2026

Copy link
Copy Markdown
MemberAuthor

Nice, I'd considered just adding "^" but somehow I confused myself thinking that would only go 1 commit up - that if we had many commits on the branch it'd only look at the last one. I guess it must squash? Anyways, glad its working.

edit:

github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased)

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

@jedcunningham
jedcunningham deleted the fix_inthewild_sorting branch January 7, 2026 18:52
@potiuk

Copy link
Copy Markdown
Member

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

Correct. This is actually what we very heavily bank on (this why I knew it by heart).
Basically whole selective checks code is based on the fact that we have "merge" commit passed as github.sha and it's first parent is base branch.

https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/commands/ci_commands.py#L163:

defget_changed_files(commit_ref: str|None) ->tuple[str, ...]:
ifcommit_refisNone:
return ()
cmd= [
"git",
"diff-tree",
"--no-commit-id",
"--name-only",
"-r",
commit_ref+"^",
commit_ref,
]
result=run_command(cmd, check=False, capture_output=True, text=True)
ifresult.returncode!=0:
get_console().print(
f"[warning] Error when running diff-tree command [/]\n{result.stdout}\n{result.stderr}"
)
return ()
changed_files=tuple(result.stdout.splitlines()) ifresult.stdoutelse ()
get_console().print("\n[info]Changed files:[/]\n")
get_console().print(changed_files)
get_console().print()
returnchanged_files

@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

BTW. There is also another interesting thing I learned with prek

There is a very nice shortcut to not have to find merge-base.

  • git diff main..your_pr_branch
  • git diff main...your_pr_branch

Spot the difference .... there are THREE dots in second case. And the ... is actually showing the changes in your branch from the merge-base

And ... (pun intended) there are few other gotchas: https://darekkay.com/blog/git-commit-ranges/#git-diff

For example git diff main...your-branch is not reverse of git diff your-branch...diff as you would suspect. The first one shows your changes since merge-base, the second shows SURPRISE -> all the changes in main (!) since merge-base 😱 😱 😱 😱 😱 😱 😱 😱 😱

Git is wild.

@potiuk

Copy link
Copy Markdown
Member

So basically if you want to have prek to execute on "only your changes" from a branch you have PR to main with - do this (without running any of git merge-base thingies):

prek --from-ref maim

(we have it in our docs)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jedcunningham@potiuk@bugraoz93@amoghrajesh
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fix basic static checks comparing same commit with itself - #60194

Closed
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting
Closed

Fix basic static checks comparing same commit with itself#60194
jedcunningham wants to merge 1 commit into
mainfrom
fix_inthewild_sorting

Conversation

@jedcunningham

Copy link
Copy Markdown
Member

The prek command was comparing github.sha with itself, which checked no files. Now uses merge-base to check all files changed in the PR against the base branch.

This ensures static checks are actually ran in PRs like #60168.

The prek command was comparing `github.sha` with itself, which checked
no files. Now uses `merge-base` to check all files changed in the PR
against the base branch.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a bug in the basic-tests workflow where static checks were comparing the same commit (github.sha) with itself, resulting in no files being checked. The fix introduces a merge-base calculation to properly identify all changed files in a PR by comparing against the base branch.

Key changes:

  • Implements merge-base calculation to find the common ancestor between PR and base branch
  • Updates static check command to compare merge-base commit against HEAD instead of comparing github.sha with itself
  • Increases fetch depth and adds fallback logic to handle cases where merge-base isn't found in shallow clones

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread.github/workflows/basic-tests.yml
Comment thread.github/workflows/basic-tests.yml
Comment on lines +231 to +245
git fetch origin "$BASE_BRANCH" --depth=50

# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")

if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi

echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
git fetch origin "$BASE_BRANCH" --depth=50
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=50, fetching more history..."
git fetch --deepen=450
git fetch origin "$BASE_BRANCH" --deepen=450
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
git fetch origin "$BASE_BRANCH" --depth=$FETCH_DEPTH
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=$FETCH_DEPTH, fetching more history..."
git fetch --deepen=$DEEPEN
git fetch origin "$BASE_BRANCH" --deepen=$DEEPEN
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
FETCH_DEPTH: 50
DEEPEN: 450

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, maybe we can make the values parameterised for both --depth and --deepen from env :)

@amoghrajeshamoghrajesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, thats a strange one

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange one .. I wonder when it happened and how it did not work (I am sure it worked before). But it can be done way simpler.

All you need it is to add missing ^ in --from-ref

At the moment you do this check - github.sha already contains merge commit - this is how pull-request workflow works.

https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request

Note that GITHUB_SHA for this event is the last merge commit of the pull request merge branch. If you want to get the commit ID for the last commit to the head branch of the pull request, use github.event.pull_request.head.sha instead.

@potiuk

Copy link
Copy Markdown
Member

Superseded by #60202

@potiukpotiuk closed this Jan 7, 2026
@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

This is precisely why we needed 2 depth - because in pull_request the github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased).

@potiuk

Copy link
Copy Markdown
Member

I merged my fix and rebased #60168 to verify that it works (it should fail)

@potiuk

Copy link
Copy Markdown
Member

Nice catch @jedcunningham BTW :)

@potiuk

Copy link
Copy Markdown
Member

@jedcunningham

jedcunningham commented Jan 7, 2026

Copy link
Copy Markdown
MemberAuthor

Nice, I'd considered just adding "^" but somehow I confused myself thinking that would only go 1 commit up - that if we had many commits on the branch it'd only look at the last one. I guess it must squash? Anyways, glad its working.

edit:

github.sha is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased)

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

@jedcunningham
jedcunningham deleted the fix_inthewild_sorting branch January 7, 2026 18:52
@potiuk

Copy link
Copy Markdown
Member

Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.

Correct. This is actually what we very heavily bank on (this why I knew it by heart).
Basically whole selective checks code is based on the fact that we have "merge" commit passed as github.sha and it's first parent is base branch.

https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/commands/ci_commands.py#L163:

defget_changed_files(commit_ref: str|None) ->tuple[str, ...]:
ifcommit_refisNone:
return ()
cmd= [
"git",
"diff-tree",
"--no-commit-id",
"--name-only",
"-r",
commit_ref+"^",
commit_ref,
]
result=run_command(cmd, check=False, capture_output=True, text=True)
ifresult.returncode!=0:
get_console().print(
f"[warning] Error when running diff-tree command [/]\n{result.stdout}\n{result.stderr}"
)
return ()
changed_files=tuple(result.stdout.splitlines()) ifresult.stdoutelse ()
get_console().print("\n[info]Changed files:[/]\n")
get_console().print(changed_files)
get_console().print()
returnchanged_files

@potiuk

potiuk commented Jan 7, 2026

Copy link
Copy Markdown
Member

BTW. There is also another interesting thing I learned with prek

There is a very nice shortcut to not have to find merge-base.

  • git diff main..your_pr_branch
  • git diff main...your_pr_branch

Spot the difference .... there are THREE dots in second case. And the ... is actually showing the changes in your branch from the merge-base

And ... (pun intended) there are few other gotchas: https://darekkay.com/blog/git-commit-ranges/#git-diff

For example git diff main...your-branch is not reverse of git diff your-branch...diff as you would suspect. The first one shows your changes since merge-base, the second shows SURPRISE -> all the changes in main (!) since merge-base 😱 😱 😱 😱 😱 😱 😱 😱 😱

Git is wild.

@potiuk

Copy link
Copy Markdown
Member

So basically if you want to have prek to execute on "only your changes" from a branch you have PR to main with - do this (without running any of git merge-base thingies):

prek --from-ref maim

(we have it in our docs)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jedcunningham@potiuk@bugraoz93@amoghrajesh