Isomorphic-git: Pull crashes Node, cannot be caught using try-catch

[TomasHubelbauer]

Hey, pull brings Node down for me in the TypeScript repro below:

import * as git from 'isomorphic-git';
import * as fs from 'fs-extra';
import * as readline from 'readline';

void async function() {
  console.log('Removing…');
  await fs.remove('junk');
  console.log('Removed.');

  let errored = false;

  try {
    console.log('Cloning…');
    await git.clone({ fs, dir: 'junk', ref: 'master', url: 'https://github.com/TomasHubelbauer/bloggo.git' });
    console.log('Cloned.');
  } catch (error) {
    console.log('Failed to clone:', error);
    errored = true;
  }

  try {
    console.log('Pulling…');
    await git.pull({ fs, dir: 'junk', ref: 'master' });
    console.log('Pulled.');
  } catch (error) {
    console.log('Failed to pull:', error);
    errored = true;
  }

  // Added to prove the process will end without any user interaction
  !errored && await new Promise(resolve => {
    readline.createInterface(process.stdin, process.stdout).question('Blocking…', (answer) => resolve(answer));
  });
}()

Here's the full output:

Removing…
Removed.
Cloning…
Cloned.
Pulling…
Using ref=master

I am using Node v9.4.0, Isomorphic Git version "0.9.0" and FS Extra version "5.0.0".

[wmhilton]

Oh boy! That's exciting. I hope I can reproduce that. (We'll find out shortly)

[wmhilton]

Oooh, this is really interesting. I'm not using TypeScript, but can get the same error using plain JS and node v9.6.1:

const git = require('isomorphic-git');
const fs = require('fs-extra');
const readline = require('readline');

void async function() {
  console.log('Removing…');
  await fs.remove('junk');
  console.log('Removed.');

  let errored = false;

  try {
    console.log('Cloning…');
    await git.clone({ fs, dir: 'junk', ref: 'master', url: 'https://github.com/TomasHubelbauer/bloggo.git' });
    console.log('Cloned.');
  } catch (error) {
    console.log('Failed to clone:', error);
    errored = true;
  }

  try {
    console.log('Pulling…');
    await git.pull({ fs, dir: 'junk', ref: 'master' });
    console.log('Pulled.');
  } catch (error) {
    console.log('Failed to pull:', error);
    errored = true;
  }

  // Added to prove the process will end without any user interaction
  !errored && await new Promise(resolve => {
    readline.createInterface(process.stdin, process.stdout).question('Blocking…', (answer) => resolve(answer));
  });
}()

Here is the output... interestingly I get an error during clone before it crashes:

> node index.js
Removing…
Removed.
Cloning…
Failed to clone: Error: Unexpected object type commit found in tree for 'junk'
    at writeTreeToDisk (E:\git\isomorphic-git\isomorphic-git\bugs\86\node_modules\isomorphic-git\dist\for-node\isomorphic-git\commands.js:384:15)
    at <anonymous>
Pulling…
Using ref=master

BUT! if I change the url from https://github.com/TomasHubelbauer/bloggo.git to https://github.com/isomorphic-git/isomorphic-git.git then it doesn't crash. And (if anyone wonders) cloning bloggo with canonical git does not crash.

The plot thickens! If I clone greenkeeper/initial instead of master it still crashes... but I don't get the Failed to clone: Error: Unexpected object type commit found in tree for 'junk'. (In case I need the commits to track this down... greenkeeper/initial is currently at a96fc839d667e7c03de7cc98dc0d5a9530bd5bbc and master is currently at 159af9ed895d49d09af27a6c6b0457961cde3042.)

It's also not the use of fs-extra which was my first thought. Does the exact same behavior with regular fs.

Investigation status: To Be Continued...

[TomasHubelbauer]

The error in clone was caused by Bloggo having a Git directory in its tree at the state it is in in GitHub (I don't push to GitHub much anymore as I've moved Bloggo to GitLab at https://gitlab.com/TomasHubelbauer/bloggo.git where clone works perfectly). I think at some point issues cloning repositories with .git directories in them should be addressed, but it feels like a very low priority issue to me.

Now that I've replaced the URL to the GitLab one so the .git directory is no longer in it, I get issues when doing pull, but this time it doesn't crash node without any errors, it does actually give an error: failed header check.

As far as I can tell that error happens in .pipe(listpack()) in GitPackIndex. listpack is from git-list-pack which in turn calls through, which at some point calls inflate. The value of pack is some binary junk, it comes from GitObjectManager.read in the branch where IDX file is being created if it doesn't exist

Also, how do you test this library during development? I am using nps build && node test.js, but that seems less than ideal? I can't require('./src') with just node test.js because it has imports and is not an MJS file (for me to use --experimental-modules). isogit from bin is not installed for me, I think that's only available when consuming the library, not when developing it?

Also @wmhilton -ing you as I'm not sure if GitHub sends notifications on comment edits.

[wmhilton]

That's really interesting. I actually had to figure out how to store ".git" directories in git for some of my unit tests. It turns out you simply CAN'T, at least in newer versions of git (really good SO explanation). I ended up having to do workarounds.

Regarding development, I've been using jest in watch mode and using a test-driven development approach. Jest automatically takes care of transpiling, so I don't need to run the build step. I generally create a new file like __tests__/test-foo.js and then run "jest --watch foo" and that re-runs whenever I save a file.

[wmhilton]

Yeah, the fact that it hangs/crashes node makes this harder than usual to test lol.

I modified some of the 'checkout' code today to be more efficient, and fixed some subtle bugs I discovered, so maybe that'll magically transfer over and fix this issue.

[wmhilton]

Since this is (hopefully?) a super rare edge case with corrupt git repositories, I'm going to close it and hope it doesn't happen again. 😁 But if it does, we'll have to address it because it was quite a serious crash.