Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Break retain cycles and fix memory leaks #224

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Break retain cycles and fix memory leaks #224

wants to merge 11 commits into from

Conversation

5sw
Copy link

@5sw 5sw commented Jul 13, 2013

Added __weak and __unsafe_unretained at the appropriate places to break all retain cycles. So finally the git repository gets dealloced when the window is closed.

@tiennou
Copy link

tiennou commented Jul 13, 2013

Please note that __weak isn't compatible with some classes on 10.7 (see #208 for an example). So if you're reverting anything touched by 5a9bf26, that won't fly for 10.7 users ;-).

Just taking a quick look at the diff, it seems like this ought to be fine tough.

@5sw
Copy link
Author

5sw commented Jul 13, 2013

Sure. I was careful to use __unsafe_unretained instead of __weak for NSViewControllersubclasses. Still no guarantees that I didn’t miss some as I don‘t have 10.7 available for testing.

@rowanj
Copy link
Owner

rowanj commented Aug 5, 2013

Can anyone verify on 10.7?

@5sw
Copy link
Author

5sw commented Jul 13, 2014

Is 10.7 still relevant? Seems nobody here is using that anymore to test this.

It is still very important to break those retain cycles so things get properly released. Not just because of memory.

  • Open a git repository in gitx
  • Close the repo again, but leave gitx running
  • Move the whole repository to the trash
  • Try to empty trash

Doesn't work as some pack-index-files are still open.

@rowanj
Copy link
Owner

rowanj commented Jul 17, 2014

10.7 and 10.8 each have roughly half the market share of 10.9; so dropping 10.7 is potentially 25% of users capable of running the current version.

Weather that is relevant is debatable, but given the voracity with which bugs are reported when compatibility is broken, I'm leaning toward yes - however I have long said that 10.7 support in GitX won't be around forever unless somebody actually volunteers to maintain it.

The time is coming, I think it may be basically after the next round of bug fixes.

Conflicts:
	Classes/git/PBGitRepository.m
@rowanj
Copy link
Owner

rowanj commented Jul 27, 2014

I should note: I like these changes.

ssp pushed a commit to ssp/gitx that referenced this pull request May 23, 2021
Fix exception when dragging files in the unstaged and staged changes table views
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