Skip to content

Conversation

@BenBeattieHood
Copy link
Contributor

exec will cause a buffer overflow if the result is >200kb (it's default maxBuffer). Ideally we'd use spawn instead here, as git should be treated as a subprocess rather than in-process; but there's also efficiency in just limiting the refs to the first 100 instead.

If you prefer, we could swap the _exec function to use spawn as below instead:

private async _exec(args: string[], cwd: string): Promise<string> => {
    const start = Date.now();
    const cmd = gitCommand;
    try {
        const result = await new Promise<string>((resolve, reject) => {
            const childProcess = spawn(cmd, args, { cwd });
            let stdout = '', stderr = '';
            childProcess.stdout.on('data', chunk => { stdout += chunk; });
            childProcess.stderr.on('data', chunk => { stderr += chunk; });
            childProcess
            .on('error', reject)
            .on('close', code => {
                if (code === 0) {
                    resolve(stdout);
                } else {
                    reject(stderr);
                }
            });
        });
        
        Tracer.verbose(`git command: ${cmd}. Output size: ${result.length} (${Date.now() - start}ms) ${cwd}`);
        return result;
    }
    catch (err) {
        Tracer.error(`git command failed: ${cmd} (${Date.now() - start}ms) ${cwd} ${err}`);
        throw err;
    }
}

`exec` will cause a buffer overflow if the result is >200kb (it's default maxBuffer). Ideally we'd use spawn instead here, as git should be treated as a subprocess rather than in-process; but there's also efficiency in just limiting the refs to the first 100 instead.
@BenBeattieHood
Copy link
Contributor Author

@huizhougit 🙏 (or, can we up the exec maxBuffer size? I need to use this in a monorepo, which would need >4.5mb buffer, so it's kinda large)

@huizhougit
Copy link
Owner

@BenBeattieHood thanks for looking into it. spawn sounds good to me. Would you mind making a PR?

@BenBeattieHood
Copy link
Contributor Author

Sure thing - here you go: #88

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants