Fire progress and update tips callbacks also for pushes.

[jacquesg]

It's not very useful to only know that a pre-receive hook has declined a push, you probably want to know why. We were only firing of the progress callback for fetch operations.

I've also fixed a leak here if you return < 0 in the callback (and cleaned it up a little) and made it only not fire any more callbacks of that type if you return < 0. I don't think that on receiving a text message from the remote should allow you to cancel the operation. Normally if the operation should not succeed, the remote side will anyway force failure (cancellation) by not allowing the push/fetch to continue.

[jacquesg]

We should probably also run the online tests, not sure how to do this?

[jacquesg]

Some travis failure :( Will fix it.

[jamill]

1. The client might not be making a decision to cancel based on the data in the progress callback - this might just be the first chance that the client has an opportunity to set the cancel flag.

2. If we don't want to expose cancellation through the progress callback, maybe we should just remove the return value on the callback (so if you sign up for progress callbacks, you always get them).

[jacquesg]

Don't you think if you need to be able to cancel the operation, it would be better suited to do it in i.e. completion, transfer_progress or update_tips? Realistically, you will see activity on the progress callback when the remote side is compressing data (for fetches)/ running hooks (for pushes). By the time it is actually running the hooks, its quite late in the operation, that is, data has already been uploaded and its just (possibly) running pre-receive and/or post-receive (when it runs post-receive cancelling will have no effect).

[jacquesg]

I actually feel that progress is too overloaded here, because we also have transfer_progress and push_transfer_progress. @carlosmn It may be a good idea to rename this to something like remote_text? Ultimately this callback always receives text that the remote side has sent us and IMHO would be a more apt name.

[jacquesg]

Actually the test case failure that I had was attributed to the naming confusion 😄 Checked the incorrect callback...

[arthurschreiber]

Don't you think if you need to be able to cancel the operation, it would be better suited to do it in i.e. completion, transfer_progress or update_tips?

The main use-case here is for bindings. If e.g. we see an exception getting raised from Ruby-Land in Rugged, we need to somehow signal that to libgit2, and correctly abort the current operation.

[jacquesg]

Fair enough.

[carlosmn]

@jacquesg 'progress' is the channel used to transmit said data.

[jacquesg]

Ok well, then we should probably not change it :)

[jacquesg]

I've changed it so the return code of callbacks are checked. If != 0, we convert to GIT_EUSER (based on existing one I found). Also fixed 1 or 2 others that didn't check the return code.

[carlosmn]

Current (wanted) behaviour is to propagate the error from the caller.

[jacquesg]

Ok, so the user is required to return < 0 and we should just set error to that? If so, do we consider return codes >=0 as success?

[jamill]

'progress' is the channel used to transmit said data.

I also think that progress is a bit generic and can cause confusion - especially for someone not familiar with the names of the channels on the network protocol. What if we named this something a little more descriptive then just progress (e.g. progress_channel, progress_channel_text,...) .

[carlosmn]

We could call it sideband_progress. I'd rather stay away from anyting with 'text' in it, as we can't really promise that text is going to come out.

[jacquesg]

sideband_progress sound reasonable.

[jacquesg]

Turns out that we are also never executing update_tips callbacks for pushes. There is some extra work to be done in the test cases, we are recording the information, but never actually checking it.

[jacquesg]

Comments?

[vmg]

Looks good to me. @carlosmn?

[jacquesg]

Probably ok unless someone still wants to review but hasn't gotten round to it?