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

Fix bug where GTPushTransferProgressBlock is unused #508

Merged
merged 3 commits into from
Aug 14, 2015

Conversation

dleehr
Copy link
Contributor

@dleehr dleehr commented Aug 14, 2015

In pushRefspecs:toRemote:withOptions, the remote_callbacks struct was incorrectly configured and the push_transfer_progress callback was not set. This caused the user-provided GTRemotePushTransferProgressBlock to never be used, since it was bound to GTRemotePushTransferProgressCallback, which was not set in the struct.

This change fixes the bug in objective-git, but there's a very similar bug in libgit2 that results in the callback only being called once for smart protocol pushes, at the end - fixed in libgit2/libgit2#3377

@joshaber joshaber self-assigned this Aug 14, 2015
@joshaber
Copy link
Member

Awesome, thanks!

It looks like there are a couple tests that need to be updated:

Failures:
  0) -[GTRemotePushSpec pushing__to_remote__can_push_one_commit] (ObjectiveGit-MacTests.xctest)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
/Users/travis/build/libgit2/objective-git/ObjectiveGitTests/GTRemotePushSpec.m:155: failed - expected to be falsy, got <1.0000>:
152             }];
153             expect(error).to(beNil());
154             expect(@(result)).to(beTruthy());
155             expect(@(transferProgressed)).to(beFalsy()); // Local transport doesn't currently call progress callbacks
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
156 
157             // Number of commits on tracking branch after push
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
  1) -[GTRemotePushSpec pushing__to_remote__when_the_local_and_remote_branches_are_in_sync__should_push_no_commits] (ObjectiveGit-MacTests.xctest)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
/Users/travis/build/libgit2/objective-git/ObjectiveGitTests/GTRemotePushSpec.m:118: failed - expected to be falsy, got <1.0000>:
115                 }];
116                 expect(error).to(beNil());
117                 expect(@(result)).to(beTruthy());
118                 expect(@(transferProgressed)).to(beFalsy()); // Local transport doesn't currently call progress callbacks
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
119 
120                 // Same number of commits after push, refresh branch first

@phatblat
Copy link
Member

I've been wondering why the progress blocks didn't get called in those tests. Thanks for the fix @dleehr!

@dleehr
Copy link
Contributor Author

dleehr commented Aug 14, 2015

Thanks @joshaber. I updated the tests. Actually the block will be called once at the end of the push with this change. libgit2/libgit2#3377 will fix it so that it's called regularly during the push too (at least in smart protocol)

@joshaber
Copy link
Member

🤘

joshaber added a commit that referenced this pull request Aug 14, 2015
Fix bug where GTPushTransferProgressBlock is unused
@joshaber joshaber merged commit f86fc7e into libgit2:master Aug 14, 2015
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