This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7960

httpconnection: allow `httpsendfile` subclasses to suppress the progressbar
ClosedPublic

Authored by mharbison72 on Jan 21 2020, 1:37 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG0e8b28fb751b: httpconnection: allow `httpsendfile` subclasses to suppress the progressbar
Summary

This will be neccessary for LFS, which manages the progressbar outside of the
file.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mharbison72 created this revision.Jan 21 2020, 1:37 PM
marmoute added a subscriber: marmoute.EditedFeb 5 2020, 6:13 PM

This is very unappealling ☹ could we update the total value in case authentication is needed instead ?

(also, the double sending is very sad, could we have a small wireprotocol command dedicated at testing authentication first instead ?)

mharbison72 added a comment.Feb 5 2020, 9:12 PM
In D7960#119446, @marmoute wrote:

This is very unappealling ☹ could we update the total value in case authentication is needed instead ?

Sorry, I'm not sure what you mean. The progressbar is per-file in the existing uses. With LFS, the worker pool drives the bus and just keeps bumping the completed amount as workers finish.

This is also a bit different for LFS because LFS sends a small text volley to the server first. That triggers the authentication before sending everything, and then the auth hash is carried in the headers of each blob. (It might seem a bit weird that the bug is fixed by reusing code that works around the auth issue. I can't explain it either; that's why I'm hoping there's somebody with a deep knowledge of how all of this URL handling and keepalive works.)

(also, the double sending is very sad, could we have a small wireprotocol command dedicated at testing authentication first instead ?)

Presumably, but IDK anything about the wire protocol, and the existing code would still be needed for the case where the server doesn't know about the command.

durin42 added a subscriber: durin42.Feb 10 2020, 4:49 PM
In D7960#119477, @mharbison72 wrote:
In D7960#119446, @marmoute wrote:

(also, the double sending is very sad, could we have a small wireprotocol command dedicated at testing authentication first instead ?)

Presumably, but IDK anything about the wire protocol, and the existing code would still be needed for the case where the server doesn't know about the command.

I looked into that for the core wireproto years ago. The summary is basically "no, httplib is a very simple http client and not readily extended." If you want to do better, you're in the bad place of doing something like using libcurl in hg, which will be very complicated. :(

mharbison72 added a commit: rHG0e8b28fb751b: httpconnection: allow `httpsendfile` subclasses to suppress the progressbar.Feb 10 2020, 5:01 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG0e8b28fb751b: httpconnection: allow `httpsendfile` subclasses to suppress the progressbar (authored by mharbison72). · Explain Why
This revision was automatically updated to reflect the committed changes.
mharbison72 added a comment.Feb 10 2020, 6:55 PM

I forgot to reply to this last week- I was concerned when the subsequent part of the stack went in without suppressing the per file progress bars here, that the displayed progress bar would be bouncing around randomly. But it wasn't- it was ever increasing like only the main bar was being changed. Any ideas why that would be?

durin42 added a comment.Feb 10 2020, 7:25 PM
In D7960#120242, @mharbison72 wrote:

I forgot to reply to this last week- I was concerned when the subsequent part of the stack went in without suppressing the per file progress bars here, that the displayed progress bar would be bouncing around randomly. But it wasn't- it was ever increasing like only the main bar was being changed. Any ideas why that would be?

Progress bars have some stickiness and won't show brief sub-topics. It's all pretty configurable, but I think the default is we don't show a nested topic if it exists for less than 2 seconds of ticks of the upper level progress bar.

Revision Contents
Changeset List

PathPackages
Mmercurial/httpconnection.py (5 lines)

Diff 19494

mercurial/httpconnection.py

def __init__(self, ui, *args, **kwargs): def __init__(self, ui, *args, **kwargs):
self.ui = ui self.ui = ui
self._data = open(*args, **kwargs) self._data = open(*args, **kwargs)
self.seek = self._data.seek self.seek = self._data.seek
self.close = self._data.close self.close = self._data.close
self.write = self._data.write self.write = self._data.write
self.length = os.fstat(self._data.fileno()).st_size self.length = os.fstat(self._data.fileno()).st_size
self._pos = 0 self._pos = 0
self._progress = self._makeprogress()
def _makeprogress(self):
# We pass double the max for total because we currently have # We pass double the max for total because we currently have
# to send the bundle twice in the case of a server that # to send the bundle twice in the case of a server that
# requires authentication. Since we can't know until we try # requires authentication. Since we can't know until we try
# once whether authentication will be required, just lie to # once whether authentication will be required, just lie to
# the user and maybe the push succeeds suddenly at 50%. # the user and maybe the push succeeds suddenly at 50%.
self._progress = ui.makeprogress( return self.ui.makeprogress(
_(b'sending'), unit=_(b'kb'), total=(self.length // 1024 * 2) _(b'sending'), unit=_(b'kb'), total=(self.length // 1024 * 2)
) )
def read(self, *args, **kwargs): def read(self, *args, **kwargs):
ret = self._data.read(*args, **kwargs) ret = self._data.read(*args, **kwargs)
if not ret: if not ret:
self._progress.complete() self._progress.complete()
return ret return ret