Skip to content

Migrate GTFilter off the deprecated apply callback - #110

Open
hannesa2 wants to merge 4 commits into
masterfrom
fix/GTFilter-migrate-to-streaming-api
Open

hannesa2 wants to merge 4 commits into
masterfrom
fix/GTFilter-migrate-to-streaming-api

Conversation

@hannesa2

@hannesa2 hannesa2 commented Sep 28, 2026 •

Copy link
Copy Markdown

This removes the last remaining import "git2/deprecated.h"

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Downstream stream cleanup must be fixed to prevent leaks.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Migrates GTFilter from libgit2’s deprecated apply callback to the streaming filter API.

Changes:

  • Buffers stream input and applies the Objective-C block on close.
  • Forwards transformed output downstream.
  • Adds stream lifecycle management.
File Summary
ObjectiveGit/​GTFilter.m Implements the streaming filter adapter. The downstream stream is not freed during cleanup, causing a leak; release it before freeing the wrapper.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ObjectiveGit/GTFilter.m
@hannesa2
hannesa2 force-pushed the fix/GTFilter-migrate-to-streaming-api branch from 8da78d6 to 3aa60d3 Compare September 28, 2026 05:33
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

A critical compile error remains, and passthrough behavior lacks regression coverage.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread ObjectiveGit/GTFilter.m
Co-authored-by: hannesa2 <3314607+hannesa2@users.noreply.github.com>
@hannesa2
hannesa2 requested a lite review from Copilot September 28, 2026 06:32

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

libgit2's own pipeline (filter_streams_free in filter.c) already frees
every writestream in the chain exactly once, including next. Its own
buffered_stream_free wrapper (used by the built-in crlf/ident filters)
deliberately does not free its target/next stream for this reason.
Freeing next here as well causes a double free.
@hannesa2
hannesa2 force-pushed the fix/GTFilter-migrate-to-streaming-api branch from 0dc3d46 to fe05cc9 Compare September 28, 2026 06:49
@hannesa2
hannesa2 requested a lite review from Copilot September 29, 2026 04:49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants