Fetch should report progress through callbacks. #238
Conversation
|
Applying this patch diff --git a/LibGit2Sharp.Tests/RemoteFixture.cs b/LibGit2Sharp.Tests/RemoteFixture.cs
index dda9fdb..4db2664 100644
--- a/LibGit2Sharp.Tests/RemoteFixture.cs
+++ b/LibGit2Sharp.Tests/RemoteFixture.cs
@@ -141,13 +141,30 @@ public void CanFetchAllTagsIntoAnEmptyRepository(string url)
}
// Perform the actual fetch
- remote.Fetch(TagFetchMode.All, onUpdateTips: expectedFetchState.RemoteUpdateTipsHandler);
+ remote.Fetch(TagFetchMode.All, mess, comp, expectedFetchState.RemoteUpdateTipsHandler, trans);
// Verify the expected
expectedFetchState.CheckUpdatedReferences(repo);
}
}
+ private void trans(TransferProgress progress)
+ {
+ Console.WriteLine("trans: Received/Index/Total = {0}/{1}/{2} - Bytes received = {3}", progress.ReceivedObjectCount, progress.IndexedObjectCount, progress.TotalObjectCount, progress.BytesReceived);
+ }
+
+ private int comp(RemoteCompletionType remotecompletiontype)
+ {
+ Console.WriteLine("comp:{0}", remotecompletiontype);
+
+ return 0;
+ }
+
+ private void mess(string message)
+ {
+ Console.Write(message);
+ }
+
[Fact]
public void CreatingANewRemoteAddsADefaultRefSpec()
{produces the following output which is rather nice 🆒 ❗ However, it looks like the number of received bytes doesn't grow... Did I mess up anything? |
| /// </summary> | ||
| [CLSCompliant(false)] | ||
| public virtual uint TotalObjectCount | ||
| { |
There was a problem hiding this comment.
I'd prefer not decorating this with a [CLSCompliant(false)]
What's the maximum object numbers that could be reached? Wouldn't int be enough? Should we have to deal with more than 2,147,483,647 objects, we could still make this a long
There was a problem hiding this comment.
I imagine int would be enough. I was reflecting the types that these values are as in libgit2. I will change the uints to be ints, and the ulong to be a long .
|
The bytes are now also updated with WinHTTP (fix came in recently). The completion callbacks still aren't being called, though we should just add them in the library. No reason to wait, really. |
LibGit2Sharp runs against the latest libgit2 Moreover, as you can below, no bytes are received, whatever the protocol used (git/http(s)) Note: While running the tests I also encountered the dreaded "Result too large" error: |
|
I have not had a chance yet to debug the issue with the bytes not updating with the WinHttp fix. I will look into this (probably not until Monday). |
|
@jamill Last nitpick using the following callback as the private void mess(string message)
{
Console.WriteLine("[BEG]{0}[END]", message);
}Produces the following output: As one can see, the Thinking about it, maybe a slight update to the XML doc wouldn't be so bad as well ;-) |
@ben As you've brilliantly dived into the network code (and got back safe), do you think you could tackle this as well? 🙇 🙏 |
Is it too late to change how libgit2 treats progress? This chunked response (which appears to be based on an internal buffer size?) seems less useful than self-contained messages. For example, if I wanted to show fetch progress on a single line I would have to parse the last full line out of the message, and remember the rest to prepend to the next message. |
|
The way we treat progress is the way that git gives it to us. Somebody has to parse it, and I'd rather not have the library decide what belongs where, especially since there is absolutely no fixed format and any number of processes on the remote end can send us data, not just git. This is clearer for push. We get the hook's output exactly the same way (which is what that channel is for) and whether to wait for a newline (or CR as is the case for fetch) to send something to the library's user is not something the library is in a position to judge. The main problem there is that git on the remote end assumes that the output will be thrown directly to the console, which will parse it without problems. The "real" solution is to have git send us machine-readable output through a different channel, but we need to standardise it somehow (and convince the other implementations to support it). |
I'm not even sure that @carlosmn would agree to this. We call this progress, but basically, those are only printable chars that the server throws at us. We could do the buffering and the line splitting on our side, though. However, I'm not sure that everything will be |
|
Ah. It looks like I should pay more attention to page refreshes. @carlosmn already answered :-) ^^ |
|
No, the output isn't LF-delimited. The remote git is sending the output so it looks good (updating the one line) when you show it to the console, so each "line" is delimited by CR. The only LF would come at the last one, so the indexing process can give its own output. |
Is this "stable" enough to build up a line-splitting process is C#?
Is this being sent by libgit2 as well? |
|
I doubt very much that it's going to change, even if just for backwards compatibility, If the output changes, the 'git fetch' output is going to break without local changes, so I wouldn't worry about that part of the format changing. The problem would come if a post-receive hook shows output by adding dots to the same line without any explicit separation, expecting it to be shown to the user immediately (not that likely a situation, but people do all sorts of weird things). |
Cool. So, provided we implement a buffered line splitting process, we've got a stable signal to let us know when we can emit a complete line. @dahlbyk @jamill Guys, I'll be off the hook for a few days and won't merge this PR untill I'm back. So if you're willing to try and update the |
Are you thinking that the delegate signature would remain the same but only emit complete lines? or, will it return a CR delimited list of strings? That is, how will it handle a string that contains multiple lines. Would the signature look something like: Or, stay as is, but not buffer the text of an incomplete line (until it is able to emit it as part of a full line). If we do not like the parameter being named |
This. And when libgit2 invokes us with a block of 3 CR delimited lines, we would issue 3 callbacks of one line, in a row. Would this make sense? If the |
|
Or we could fire the progress callback with a single string of all complete lines (including line endings, should a client wish to handle CR/LF differently), storing the remainder be sent with the next callback. |
|
@dahlbyk I was rather thinking of removing the trailing CR or LF before firing each callback. So, the client could either "replace" the line being displayed or pack them together using whatever line ending terminator. Given: we would issue First invocation:
Second invocation:
Third invocation:
|
|
LF doesn't necessarily mean end of progress. This: private void mess(string message)
{
message = message.Replace("\r", "\\r\r").Replace("\n", "\\n\n");
Console.Write(message);
}Yields this: So that the shell leaves only the messages preceding LF: If our consumers want to emulate this behavior they need the line ending. |
|
You're right. Beside this, we don't need to identify the final chunk. We only need to be sure that it will bear a line terminator. So, given: we would issue First invocation:
Second invocation:
Third invocation:
Better? |
|
If you only call These channels are really meant to correspond nicely to stdout/stderr. It seems like the best analog to them is a Stream... why don't we just write the literal bytes to a |
@ben @nulltoken It looks like this is due to the granularity of the number of bytes reported. This value gets updated when fetching a larger repository.
I was also able to repro and get the part of the stack that sets this error. Looks like the call to |
👍 |
|
I like very much the TextWriter idea as well. And I'd be very interested in having a sample usage in the tests of what an advanced implementation (callback based?) could look like. |
|
@jamill updates look good |
|
I figured out the problem with |
|
@ben Thanks! I have rebased this PR on top of @ben's fix. The output fron @nulltoken's patch:
|
|
I also made some tweaks with the commit in my last update. This was mainly to some of comments, but also included the following changes (I think these are for the better)
|
@nulltoken - I am not be against implementing the |
|
@jamill @nulltoken See dahlbyk/libgit2sharp@jamill:fetch_update...fetch_update
|
|
And 👍 for polish changes |
|
@dahlbyk Thanks for the review! I like your suggestions. Do we need both the |
|
Part of me thinks the callback maybe gives a bit of flexibility that would be harder to achieve with the |
|
My thoughts were that having a single way to expose the text stream from the server would leave the interface a bit simpler (and that if there is another scenario that we want to support which exposing the callback would enable / make easier, we could add it then...) Either way, I am fine as well. Assuming we want to expose both interfaces, what would be the best way to include your modifications? I could directly include your commits in this PR? you could follow up with a PR? ... |
|
For now I would bring them into this PR, then squash as appropriate when we're ready to merge. I think we're close? |
|
@nulltoken will have to make the call on the progress handler. :) |
@dahlbyk I agree with you. This is why was asking for some advanced usage in a test. I think that redirecting thr output to a Console might be trivial. However, if the client is a GUI a wishes to update a progress dialog, this may require some trickier tweaking. @jamill This PR is awesome as it is! Let's keep the Could you please update the |
|
Thanks for the reviews!
Done! @dahlbyk - I also included your suggestion for moving the null check into the callback generation. Thanks! |
|
👍 |
|
Merged! ❤️ + ✨ = 💖 |
Update to LibGit2Sharp that incorporates the new progress callbacks exposed in libgit2 (PR libgit2/libgit2#990)