Patchwork [4,of,4,stable] largefiles: don't mute/obfuscate http errors when putlfile fails

login
register
mail settings
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

Mads Kiilerich - Oct. 21, 2015, 7:16 p.m.
# 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

'unexpected putlfile response: None' when an http error occurs is not very
helpful.

Instead, leave the handling of urllib2.HTTPError exceptions to other layers.
Augie Fackler - Oct. 22, 2015, 10:35 a.m.
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
Mads Kiilerich - Oct. 22, 2015, 11:12 a.m.
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
Pierre-Yves David - Nov. 7, 2015, 9:15 p.m.
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