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

remove unused imports, make clicktests more reliable, fix git rev-parse with bare repositories, fix crash when directory gets deleted #1263

Merged
merged 7 commits into from
Jan 24, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/credentials-helper
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
#!/usr/bin/env node
const winston = require('winston');
const http = require('http');
const socketId = process.argv[2];
const portAndRootPath = process.argv[3];
const remote = process.argv[4];
const action = process.argv[5];
const http = require('http');

winston.remove(winston.transports.Console);
winston.info(`Credentials helper invoked; portAndRootPath:${portAndRootPath} socketId:${socketId}`);
Expand Down
1 change: 0 additions & 1 deletion components/app/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
const ko = require('knockout');
const components = require('ungit-components');
const programEvents = require('ungit-program-events');
const navigation = require('ungit-navigation');
const storage = require('ungit-storage');

components.register('app', (args) => {
Expand Down
1 change: 0 additions & 1 deletion components/commitdiff/commitlinediff.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
const ko = require('knockout');
const components = require('ungit-components');
const inherits = require('util').inherits;
const programEvents = require('ungit-program-events');

class CommitLineDiff {
Expand Down
1 change: 0 additions & 1 deletion components/dialogs/dialogs.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@

const ko = require('knockout');
const inherits = require('util').inherits;
const components = require('ungit-components');
const Bluebird = require('bluebird');
const programEvents = require('ungit-program-events');
Expand Down
2 changes: 0 additions & 2 deletions components/staging/staging.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ const components = require('ungit-components');
const programEvents = require('ungit-program-events');
const filesToDisplayIncrmentBy = 50;
const filesToDisplayLimit = filesToDisplayIncrmentBy;
// when discard button is clicked and disable discard warning is selected, for next 5 minutes disable discard warnings
const muteGraceTimeDuration = 60 * 1000 * 5;
const mergeTool = ungit.config.mergeTool;

components.register('staging', args => new StagingViewModel(args.server, args.repoPath, args.graph));
Expand Down
2 changes: 1 addition & 1 deletion components/textdiff/textdiff.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ class TextDiffViewModel {

let html;

if (this.textDiffType.value() === 'sidebysidediff') {
if (this.textDiffType.value() === sideBySideDiff) {
html = diff2html.getPrettySideBySideHtmlFromJson(this.diffJson);
} else {
html = diff2html.getPrettyHtmlFromJson(this.diffJson);
Expand Down
16 changes: 8 additions & 8 deletions nmclicktests/environment.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,6 @@ Nightmare.action('ug', {
this.ug.backgroundAction('POST', `${rootUrl}/api/testing/createfile`, { file: filename })
.then(done.bind(null, null), done);
},
'shutdownServer': function(done) {
this.ug.backgroundAction('POST', `${rootUrl}/api/testing/shutdown`, undefined)
.then(done.bind(null, null), done);
},

'changeTestFile': function(filename, done) {
this.ug.backgroundAction('POST', `${rootUrl}/api/testing/changefile`, { file: filename })
Expand Down Expand Up @@ -250,14 +246,15 @@ class Environment {
});
}

shutdown(doNotClose) {
shutdown() {
this.shuttinDown = true;
return this.nm.ug.backgroundAction('POST', `${rootUrl}/api/testing/cleanup`, undefined)
.ug.shutdownServer()
.then(() => {
if (!doNotClose) {
return this.nm.end();
if (this.ungitServer) {
this.ungitServer.kill('SIGINT');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of exiting the process from inside, the child process gets killed from the outside. (See above for reasons.)

this.ungitServer = null;
}
return this.nm.end();
});
}

Expand Down Expand Up @@ -326,6 +323,9 @@ class Environment {
}
});
ungitServer.on('exit', () => winston.info('UNGIT SERVER EXITED'));

this.ungitServer = ungitServer;

return Bluebird.resolve();
}
}
2 changes: 1 addition & 1 deletion nmclicktests/spec.authentication.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const environment = require('./environment')({

describe('[AUTHENTICATION]', () => {
before('Environment init without temp folder', () => environment.init());
after('Close nightmare', () => environment.nm.end());
after('Environment stop', () => environment.shutdown());

it('Open home screen should show authentication dialog', () => {
return environment.goto(environment.getRootUrl())
Expand Down
2 changes: 2 additions & 0 deletions nmclicktests/spec.discard.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ describe('[DISCARD - noWarn]', () => {
return environment.init()
.then(() => environment.createRepos(testRepoPaths, [{ bare: false }]))
});
after('Environment stop', () => environment.shutdown());

it('Open path screen', () => {
return environment.nm.ug.openUngit(testRepoPaths[0]);
Expand All @@ -52,6 +53,7 @@ describe('[DISCARD - withWarn]', () => {
return environment.init()
.then(() => environment.createRepos(testRepoPaths, [{ bare: false }]))
});
after('Environment stop', () => environment.shutdown());

it('Open path screen', () => {
return environment.nm.ug.openUngit(testRepoPaths[0]);
Expand Down
1 change: 0 additions & 1 deletion public/source/server.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@

var signals = require('signals');
var programEvents = require('ungit-program-events');
var _ = require('lodash');
var Promise = require("bluebird");
Expand Down
24 changes: 13 additions & 11 deletions source/git-api.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ const gitPromise = require('./git-promise');
const fs = require('./utils/fs-async');
const ignore = require('ignore');
const Bluebird = require('bluebird');
const crypto = require('crypto');

const isMac = /^darwin/.test(process.platform);
const isWindows = /^win/.test(process.platform);
Expand Down Expand Up @@ -72,7 +71,8 @@ exports.registerApi = (env) => {
});
}
}).then(() => {
socket.watcher.push(fs.watch(pathToWatch, options || {}, (event, filename) => {
const watcher = fs.watch(pathToWatch, options || {});
watcher.on('change', (event, filename) => {
if (!filename) return;
const filePath = path.join(subfolderPath, filename);
winston.debug(`File change: ${filePath}`);
Expand All @@ -81,7 +81,11 @@ exports.registerApi = (env) => {
emitGitDirectoryChanged(socket.watcherPath);
emitWorkingTreeChanged(socket.watcherPath);
}
}));
});
watcher.on('error', (err) => {
Copy link
Collaborator Author

@campersau campersau Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Listening for watcher errors is required otherwise the ungit process crashes when errors occur.

Steps to reproduce (on windows at least I got a permission error):

  • initialize a new git repro in some folder
  • navigate to some folder in ungit
  • remove some folder

winston.warn(`Error watching ${pathToWatch}: `, JSON.stringify(err));
});
socket.watcher.push(watcher);
});
};

Expand Down Expand Up @@ -556,7 +560,7 @@ exports.registerApi = (env) => {
.then((baseRepoPath) => {
return { path: path.resolve(baseRepoPath.trim()) };
}).catch((e) => {
if (e.errorCode === 'not-a-repository') {
if (e.errorCode === 'not-a-repository' || e.errorCode === 'must-be-in-working-tree') {
return {};
campersau marked this conversation as resolved.
Show resolved Hide resolved
}
throw e;
Expand Down Expand Up @@ -717,13 +721,11 @@ exports.registerApi = (env) => {
app.post(`${exports.pathPrefix}/testing/git`, ensureAuthenticated, (req, res) => {
jsonResultOrFailProm(res, gitPromise(req.body.command, req.body.repo))
});
app.post(`${exports.pathPrefix}/testing/cleanup`, ensureAuthenticated, (req, res) => {
//winston.info('Cleaned up: ' + JSON.stringify(cleaned));
res.json({ result: temp.cleanup() });
});
app.post(`${exports.pathPrefix}/testing/shutdown`, ensureAuthenticated, (req, res) => {
res.json({});
process.exit();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exiting the process during a request / response can result in connection reset errors on the client which can make tests fail.

app.post(`${exports.pathPrefix}/testing/cleanup`, (req, res) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed ensureAuthenticated from this endpoint so it can be called during the AUTHENTICATION clicktests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these endpoints still available when running ungit outside of tests?
If it doesn't require authentication, should it be hidden behind a TEST env variable for example?

Copy link
Collaborator Author

@campersau campersau Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they are hidden behind the dev config (and only the testing/cleanup endpoint is affected by this change).

if (config.dev) {

ungit/source/config.js

Lines 65 to 66 in 68d69bb

// Used for development purposes.
dev: false,

temp.cleanup((err, cleaned) => {
winston.info('Cleaned up: ' + JSON.stringify(cleaned));
res.json({ result: cleaned });
});
});
}
};
4 changes: 1 addition & 3 deletions source/git-parser.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
const moment = require('moment');
const fs = require('fs');
const path = require('path');
const fileType = require('./utils/file-type.js');
const _ = require('lodash')
const _ = require('lodash');

exports.parseGitStatus = (text, args) => {
const lines = text.split('\n');
Expand Down
19 changes: 11 additions & 8 deletions source/git-promise.js
Original file line number Diff line number Diff line change
Expand Up @@ -420,16 +420,19 @@ git.commit = (repoPath, amend, emptyCommit, message, files) => {
}

git.revParse = (repoPath) => {
return git(['rev-parse', '--is-inside-work-tree', '--is-bare-repository', '--show-toplevel'], repoPath)
return git(['rev-parse', '--is-inside-work-tree', '--is-bare-repository'], repoPath)
.then((result) => {
const resultLines = result.toString().split('\n');
const rootPath = path.normalize(resultLines[2] ? resultLines[2] : repoPath);
if (resultLines[0].indexOf('true') > -1) {
return { type: 'inited', gitRootPath: rootPath };
} else if (resultLines[1].indexOf('true') > -1) {
return { type: 'bare', gitRootPath: rootPath };
const resultLines = result.split('\n');
if (resultLines[1].indexOf('true') > -1) { // bare repositories don't support `--show-toplevel` since git 2.25
return { type: 'bare', gitRootPath: repoPath };
}
return { type: 'uninited', gitRootPath: rootPath };
return git(['rev-parse', '--show-toplevel'], repoPath).then((topLevel) => {
const rootPath = path.normalize(topLevel.trim() ? topLevel.trim() : repoPath);
if (resultLines[0].indexOf('true') > -1) {
return { type: 'inited', gitRootPath: rootPath };
}
return { type: 'uninited', gitRootPath: rootPath };
});
}).catch((err) => ({ type: 'uninited', gitRootPath: path.normalize(repoPath) }));
}

Expand Down
1 change: 0 additions & 1 deletion test/spec.credentials-helper.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@

const expect = require('expect.js');
const child_process = require('child_process');
const path = require('path');
const http = require('http');
const config = require('../source/config');
const url = require('url');
Expand Down
1 change: 0 additions & 1 deletion test/spec.git-api.submodule.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ const express = require('express');
const path = require('path');
const restGit = require('../source/git-api');
const common = require('./common-es6.js');
const wrapErrorHandler = common.wrapErrorHandler;

const app = express();
app.use(require('body-parser').json());
Expand Down