| Submitter | Mads Kiilerich |
|---|---|
| Date | Oct. 21, 2015, 7:16 p.m. |
| Message ID | <672fe15eacfa6cb600fe.1445454967@madski> |
| Download | mbox | patch |
| Permalink | /patch/11214/ |
| State | Deferred |
| Delegated to: | Augie Fackler |
| Headers | show |
Comments
On Wed, Oct 21, 2015 at 09:16:07PM +0200, Mads Kiilerich wrote: > # HG changeset patch > # User Mads Kiilerich <madski@unity3d.com> > # Date 1445454338 -7200 > # Wed Oct 21 21:05:38 2015 +0200 > # Branch stable > # Node ID 672fe15eacfa6cb600febf2e0ad48e9f9fd81c18 > # Parent 45a74ff314b78e2539049bbf3e90d59e9a6c90dd > largefiles: don't mute/obfuscate http errors when putlfile fails Looks good, but these seem a bit new-feature-y and not regression-fix-y for stable to me. mpm? > > 'unexpected putlfile response: None' when an http error occurs is not very > helpful. > > Instead, leave the handling of urllib2.HTTPError exceptions to other layers. > > diff --git a/hgext/largefiles/proto.py b/hgext/largefiles/proto.py > --- a/hgext/largefiles/proto.py > +++ b/hgext/largefiles/proto.py > @@ -86,15 +86,14 @@ def wirereposetup(ui, repo): > # input file-like into a bundle before sending it, so we can't use > # it ... > if issubclass(self.__class__, httppeer.httppeer): > - res = None > + res = self._call('putlfile', data=fd, sha=sha, > + headers={'content-type':'application/mercurial-0.1'}) > try: > - res = self._call('putlfile', data=fd, sha=sha, > - headers={'content-type':'application/mercurial-0.1'}) > d, output = res.split('\n', 1) > for l in output.splitlines(True): > self.ui.warn(_('remote: '), l) # assume l ends with \n > return int(d) > - except (ValueError, urllib2.HTTPError): > + except ValueError: > self.ui.warn(_('unexpected putlfile response: %r\n') % res) > return 1 > # ... but we can't use sshrepository._call because the data= > diff --git a/tests/test-largefiles-cache.t b/tests/test-largefiles-cache.t > --- a/tests/test-largefiles-cache.t > +++ b/tests/test-largefiles-cache.t > @@ -212,8 +212,7 @@ Test coverage of error handling from put > $ hg push http://localhost:$HGPORT1 -f --config files.usercache=nocache > pushing to http://localhost:$HGPORT1/ > searching for changes > - unexpected putlfile response: None > - abort: remotestore: could not put $TESTTMP/src/.hg/largefiles/e2fb5f2139d086ded2cb600d5a91a196e76bf020 to remote store http://localhost:$HGPORT1/ > + abort: remotestore: could not open file $TESTTMP/src/.hg/largefiles/e2fb5f2139d086ded2cb600d5a91a196e76bf020: HTTP Error 403: ssl required > [255] > > $ rm .hg/largefiles/e2fb5f2139d086ded2cb600d5a91a196e76bf020 > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > https://selenic.com/mailman/listinfo/mercurial-devel
On 10/22/2015 12:35 PM, Augie Fackler wrote: > On Wed, Oct 21, 2015 at 09:16:07PM +0200, Mads Kiilerich wrote: >> # HG changeset patch >> # User Mads Kiilerich <madski@unity3d.com> >> # Date 1445454338 -7200 >> # Wed Oct 21 21:05:38 2015 +0200 >> # Branch stable >> # Node ID 672fe15eacfa6cb600febf2e0ad48e9f9fd81c18 >> # Parent 45a74ff314b78e2539049bbf3e90d59e9a6c90dd >> largefiles: don't mute/obfuscate http errors when putlfile fails > Looks good, but these seem a bit new-feature-y and not > regression-fix-y for stable to me. mpm? I agree they not are regression fixes and thus could be postponed; it fixes stuff that was introduced by this 'various' dude who introduced largefiles. I do however consider it a bug that store corruption could go unnoticed and be propagated to the working directory where it caused an "impossible" "--clean leave working dir with [incorrect] stuff to commit". The fix for that is appropriate for stable. Incorrect and misleading reporting of bugs also seems like a bug to me. /Mads
On 10/22/2015 07:12 AM, Mads Kiilerich wrote: > On 10/22/2015 12:35 PM, Augie Fackler wrote: >> On Wed, Oct 21, 2015 at 09:16:07PM +0200, Mads Kiilerich wrote: >>> # HG changeset patch >>> # User Mads Kiilerich <madski@unity3d.com> >>> # Date 1445454338 -7200 >>> # Wed Oct 21 21:05:38 2015 +0200 >>> # Branch stable >>> # Node ID 672fe15eacfa6cb600febf2e0ad48e9f9fd81c18 >>> # Parent 45a74ff314b78e2539049bbf3e90d59e9a6c90dd >>> largefiles: don't mute/obfuscate http errors when putlfile fails >> Looks good, but these seem a bit new-feature-y and not >> regression-fix-y for stable to me. mpm? > > I agree they not are regression fixes and thus could be postponed; it > fixes stuff that was introduced by this 'various' dude who introduced > largefiles. > > I do however consider it a bug that store corruption could go unnoticed > and be propagated to the working directory where it caused an > "impossible" "--clean leave working dir with [incorrect] stuff to > commit". The fix for that is appropriate for stable. > > Incorrect and misleading reporting of bugs also seems like a bug to me. Should we get this patch someone now that the freeze is over?
Patch
diff --git a/hgext/largefiles/proto.py b/hgext/largefiles/proto.py --- a/hgext/largefiles/proto.py +++ b/hgext/largefiles/proto.py @@ -86,15 +86,14 @@ def wirereposetup(ui, repo): # input file-like into a bundle before sending it, so we can't use # it ... if issubclass(self.__class__, httppeer.httppeer): - res = None + res = self._call('putlfile', data=fd, sha=sha, + headers={'content-type':'application/mercurial-0.1'}) try: - res = self._call('putlfile', data=fd, sha=sha, - headers={'content-type':'application/mercurial-0.1'}) d, output = res.split('\n', 1) for l in output.splitlines(True): self.ui.warn(_('remote: '), l) # assume l ends with \n return int(d) - except (ValueError, urllib2.HTTPError): + except ValueError: self.ui.warn(_('unexpected putlfile response: %r\n') % res) return 1 # ... but we can't use sshrepository._call because the data= diff --git a/tests/test-largefiles-cache.t b/tests/test-largefiles-cache.t --- a/tests/test-largefiles-cache.t +++ b/tests/test-largefiles-cache.t @@ -212,8 +212,7 @@ Test coverage of error handling from put $ hg push http://localhost:$HGPORT1 -f --config files.usercache=nocache pushing to http://localhost:$HGPORT1/ searching for changes - unexpected putlfile response: None - abort: remotestore: could not put $TESTTMP/src/.hg/largefiles/e2fb5f2139d086ded2cb600d5a91a196e76bf020 to remote store http://localhost:$HGPORT1/ + abort: remotestore: could not open file $TESTTMP/src/.hg/largefiles/e2fb5f2139d086ded2cb600d5a91a196e76bf020: HTTP Error 403: ssl required [255] $ rm .hg/largefiles/e2fb5f2139d086ded2cb600d5a91a196e76bf020