Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[question] Unexpected stdout when running git check-ignore #178

Open
kittaakos opened this issue Mar 30, 2018 · 9 comments
Open

[question] Unexpected stdout when running git check-ignore #178

kittaakos opened this issue Mar 30, 2018 · 9 comments

Comments

@kittaakos
Copy link
Contributor

I tried to run git check-ignore from dugite. Unfortunately, I got different results when I ran it from a terminal, and when I executed the command from code. Perhaps, I misunderstood something, or I must be blind. Could you please tell me what is wrong with my test case? Thank you!

From the code:

  describe('check-ignore', () => {
    it('should list the ignored files form the root', async () => {
      const testRepoPath = await initialize('check-ignore')

      const aFile = path.join(testRepoPath, 'a.txt')
      fs.writeFileSync(aFile, 'foo')
      expect(fs.readFileSync(aFile, { encoding: 'utf8' })).to.be.equal('foo')

      const gitignore = path.join(testRepoPath, '.gitignore')
      fs.writeFileSync(gitignore, 'a.txt')
      expect(fs.readFileSync(gitignore, { encoding: 'utf8' })).to.be.equal('a.txt')

      const result = await GitProcess.exec(['check-ignore', '*'], testRepoPath)

      verify(result, r => {
        expect(r.stdout.trim()).to.be.equal('a.txt')
      })
    })
  })
       check-ignore
         should list the ignored files form the root:

      AssertionError: expected '' to equal 'a.txt'
      + expected - actual

      +a.txt

      at helpers_1.verify.r (test/fast/git-process-test.ts:126:39)
      at Object.verify (test/helpers.ts:19:5)
      at Object.<anonymous> (test/fast/git-process-test.ts:125:7)
      at Generator.next (<anonymous>)
      at fulfilled (test/fast/git-process-test.ts:4:58)
      at <anonymous>

From a terminal:

mkdir tmp-check-ignore \
&& cd tmp-check-ignore \
&& git init -q \
&& echo "foo" > a.txt \
&& echo "a.txt" > .gitignore \
&& git check-ignore *
a.txt
@shiftkey
Copy link
Member

@kittaakos I haven't had a chance to look into this, but a first step might be to check if stderr contains your expected message, rather than stdout?

@kittaakos
Copy link
Contributor Author

kittaakos commented Apr 1, 2018

I have checked both stderr and stdout; they're empty strings. I did some further investigation, and it seems, git check-ignore works correctly if I do not use the * as an argument. Here is a short example what I meant:

  describe('check-ignore', () => {
    it('should list the ignored files from the root', async () => {
      const testRepoPath = await initialize('check-ignore')

      const aFile = Path.join(testRepoPath, 'a.txt')
      Fs.writeFileSync(aFile, 'foo')
      expect(Fs.readFileSync(aFile, { encoding: 'utf8' })).to.be.equal('foo')

      const bFile = Path.join(testRepoPath, 'a.txt')
      Fs.writeFileSync(bFile, 'bar')
      expect(Fs.readFileSync(bFile, { encoding: 'utf8' })).to.be.equal('bar')

      const gitignore = Path.join(testRepoPath, '.gitignore')
      Fs.writeFileSync(gitignore, 'a.txt')
      expect(Fs.readFileSync(gitignore, { encoding: 'utf8' })).to.be.equal('a.txt')

      const aResult = await GitProcess.exec(['check-ignore', 'a.txt'], testRepoPath)
      verify(aResult, r => {
        expect(r.exitCode).to.be.equal(0)
        expect(r.stdout.trim()).to.be.equal('a.txt')
      })

      const bResult = await GitProcess.exec(['check-ignore', 'b.txt'], testRepoPath)
      verify(bResult, r => {
        expect(r.exitCode).to.be.equal(1)
        expect(r.stdout.trim()).to.be.equal('')
      })
    })
  })

And the result is the same from a terminal:

mkdir tmp-check-ignore \
&& cd tmp-check-ignore \
&& git init -q \
&& echo "foo" > a.txt \
&& echo "bar" > b.txt \
&& echo "a.txt" > .gitignore \
&& git check-ignore a.txt \
&& git check-ignore b.txt
a.txt

Update: stdin -> stderr

@shiftkey
Copy link
Member

shiftkey commented Apr 1, 2018

@kittaakos I think this is because NodeJS might not be expanding * when it's an argument in the child_process APIs, unlike how a shell might do this.

This question delves into a couple of the alternatives to this, but I don't think they'll work directly for this use case.

@kittaakos
Copy link
Contributor Author

I found the exact same thread and tried out to call git check-ignore * with that way (spawnSync ); it worked.
(Please note, the snippet assumes you have git on your path.)

describe('check-ignore', () => {
    it('should list the ignored files from the root', async () => {
      const testRepoPath = await initialize('check-ignore')

      const aFile = Path.join(testRepoPath, 'a.txt')
      Fs.writeFileSync(aFile, 'foo')
      expect(Fs.readFileSync(aFile, { encoding: 'utf8' })).to.be.equal('foo')

      const bFile = Path.join(testRepoPath, 'a.txt')
      Fs.writeFileSync(bFile, 'bar')
      expect(Fs.readFileSync(bFile, { encoding: 'utf8' })).to.be.equal('bar')

      const gitignore = Path.join(testRepoPath, '.gitignore')
      Fs.writeFileSync(gitignore, 'a.txt')
      expect(Fs.readFileSync(gitignore, { encoding: 'utf8' })).to.be.equal('a.txt')

      const cp = require('child_process')
      let child
      const cmd = 'git check-ignore *'
      const cwd = testRepoPath

      if ('win32' === process.platform) {
        child = cp.spawnSync('cmd.exe', ['/s', '/c', '"' + cmd + '"'], {
          cwd,
          windowsVerbatimArguments: true
        })
      } else {
        child = cp.spawnSync('/bin/sh', ['-c', cmd], { cwd })
      }
      expect(child.stdout.toString().trim()).to.be.equal('a.txt')
    })
  })

@kittaakos
Copy link
Contributor Author

If I change the exec implementation from execFile to spawn, it works.

// ...
const command = [gitLocation, ...args].join(' ')
let cp: ChildProcess
if ('win32' === process.platform) {
  cp = spawn('cmd.exe', ['/s', '/c', '"' + command + '"'], spawnOptions)
} else {
  cp = spawn('/bin/sh', ['-c', command], spawnOptions)
}

// ...
cp.on('error', ...)
cp.on('exit', ...)
cp.stdout.on('data', ...)
cp.stderr.on('data', ...)

(Just wanted to try it out.)

Also, all tests pass except the throws error when exceeding the output range. The maxBuffer seems to be ignored, and the test passes instead of the expected error. I am not yet positive, but spawn might not have the 200 * 1024 byte limitation like execFile has.

Although maxBuffer is declared on the SpawnSyncOptionsWithStringEncoding in the typings, it is not mentioned in the documentation and does not seem to be used in the code either.

I also found this and this threads on the spawn maxBuffer topic.

By the way, alternatively, I could use the git clean -ndX instead of git check-ignore *.

@kittaakos
Copy link
Contributor Author

@shiftkey, are there any plans adjusting the implementation. I could take care of the PR if you do not have time.

Also, I could understand if you do not want to switch from cp.execFile to cp.spawn but then, could we expose the error handler code into the API so that I could wrap spawn and call git-process#spawn instead of git-process#exec. Thanks!

@shiftkey
Copy link
Member

are there any plans adjusting the implementation. I could take care of the PR if you do not have time.

I'm swamped, but would be happy to review the changes if you've found a way to resolve it.

@kittaakos
Copy link
Contributor Author

Great, I will take care of it. Thanks!

kittaakos added a commit to kittaakos/dugite that referenced this issue Apr 19, 2018
kittaakos added a commit to kittaakos/dugite that referenced this issue Apr 19, 2018
@kittaakos
Copy link
Contributor Author

@shiftkey, here is a work-in-progress PR with my changes: #181

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

No branches or pull requests

2 participants