isomorphic-git: TypeError is thrown during clone when permission is denied to repository on BitBucket

My proposal does include the entire unparseable line. While not particularly useful in the particular case Ec Roc found, indicating why the response is unparseable could be useful if for instance a line arrives with two adjacent spaces instead of one. Including the entire response or the response up to and including the problem part would be great, but I don’t immediately see how to do that efficiently. If the error occurs on the 1500th ref the first few hundred characters wouldn’t include it. Having a special error seems like a great idea. How about including the bad line in the message along with the problem, and the entire response in error.data.response? If it’s something like a double space, it’s going to be much easier to see if you know what line it’s in.

@djencks I don’t think we’re disagreeing. If the line is malformed, isomorphic-git should throw an error saying so and preserve the line so that it can be inspected.

While the error Ec got was a permission error, it was not sent by the server in the correct way. A permission error should be a 403 error. Instead, the server returned a 200 status and stuffed the error message into the body. In that case, isomorphic-git should simply say “no way, that’s a bad response” and then pass that response along as the exhibit.

@mojavelinux My whole aim here has been that when something unexpected happens to show the user what that was, and something about how it’s unexpected, so they have a chance to figure out what is wrong, and perhaps submit an informative bug. In Ec Roc’s case, seeing the original line immediately led to a solution to the problem.

BTW perhaps the response Ec Roc got should be expected, I think the regular git client reports something similar to me when I try to push a branch to the antora or isomorphic git repo rather than my fork. I haven’t really investigated however.

@wmhilton I modified my branch to restore informative exception contents (the expected line should be a description of what we expected, e.g., “two strings separated by ’ '”, not the separator character itself) and to use Ec Roc’s observed server output in the \0 test. I think this produces reasonable if not ideal output from the sorts of unparseable input I can imagine going into the method. You can clearly see the line that was unparseable, and draw your own conclusions; in Ec Roc’s case, the conclusion was obvious.

Trying the same tests on William’s https://github.com/isomorphic-git/isomorphic-git/pull/924, no exceptions are thrown at all, wholly concealing the existence of any problem:

  ● git wire protocol › parseRefsAdResponse bad null separated

    Failed: "expected an error"

      210 |         service: 'git-upload-pack'
      211 |       })
    > 212 |       fail('expected an error')
          |       ^
      213 |     } catch (error) {
      214 |       expect(error.code).toEqual(E.AssertServerResponseFail)
      215 |       expect(error.data).toEqual({

      at Env.fail (node_modules/jest-jasmine2/build/jasmine/Env.js:778:61)
      at Object.fail (__tests__/test-wire.js:212:7)

  ● git wire protocol › parseRefsAdResponse HEAD bad space separated

    Failed: "expected an error"

      241 |         service: 'git-upload-pack'
      242 |       })
    > 243 |       fail('expected an error')
          |       ^
      244 |     } catch (error) {
      245 |       expect(error.code).toEqual(E.AssertServerResponseFail)
      246 |       expect(error.data).toEqual({

      at Env.fail (node_modules/jest-jasmine2/build/jasmine/Env.js:778:61)
      at Object.fail (__tests__/test-wire.js:243:7)

  ● git wire protocol › parseRefsAdResponse refs bad space separated

    Failed: "expected an error"

      267 |         service: 'git-upload-pack'
      268 |       })
    > 269 |       fail('expected an error')
          |       ^
      270 |     } catch (error) {
      271 |       expect(error.code).toEqual(E.AssertServerResponseFail)
      272 |       expect(error.data).toEqual({

To me this is not an acceptable resolution of the original problem. It’s worse than the original situation, where Antora at least could identify the line in isomorphic-git where the problem was concealed and we could work backwards to figure out what was going on.

We know what response Ec Roc got from the git server, so perhaps we should have a test with that response?

001e# service=git-upload-pack
ERR Repository not found
The requested repository does not exist, or you do not have permission to
access it.

My main concern here is that if we (i.e., isomorphic git) receive something we don’t 100% understand from the git server, we transmit that information complete and unmodified, with appropriate context information, to the caller. I haven’t tried writing a test yet but after briefly looking at the diff I think perhaps William’s proposal would more or less ignore the response above and that there are at least two further unchecked spilts that could, with unexpected input, still give misleading and uninformative errors. Perhaps later I’ll have time to write a test using the server response above to see what happens.