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.

Sign up for GitHub

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

Jump to bottom

Fix last updated time misleading, only show when file content change or otherwise when it first created #1023

Merged
merged 5 commits into from
Oct 14, 2018
Merged

Fix last updated time misleading, only show when file content change or otherwise when it first created #1023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
26957af
Fix last updated time misleading, only show when file content change
fiennyangeln Oct 8, 2018
5d1e552
Fix prettier
fiennyangeln Oct 8, 2018
100b117
- Simplify regex
fiennyangeln Oct 10, 2018
e3f3ca3
Add test when repositories is moved
fiennyangeln Oct 10, 2018
b38dd21
Use shell.exec mock
fiennyangeln Oct 14, 2018
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
35 changes: 35 additions & 0 deletions v1/lib/core/__tests__/utils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

const path = require('path');
const fs = require('fs');
const shell = require('shelljs');
const utils = require('../utils');

const blogPostWithTruncateContents = fs.readFileSync(
Expand Down Expand Up @@ -96,6 +97,40 @@ describe('utils', () => {
fs.writeFileSync(tempFilePath, 'Lorem ipsum :)');
expect(utils.getGitLastUpdated(tempFilePath)).toBeNull();
fs.unlinkSync(tempFilePath);

// test renaming and moving file

const tempFilePath2 = path.join(__dirname, '__fixtures__', '.temp2');
const tempFilePath3 = path.join(
__dirname,
'__fixtures__',
'test',
'.temp3',
);

// create new file
shell.exec = jest.fn(() => ({
stdout:
'1539502055\n' +
'\n' +
' create mode 100644 v1/lib/core/__tests__/__fixtures__/.temp2\n',
}));
const createTime = utils.getGitLastUpdated(tempFilePath2);
expect(typeof createTime).toBe('string');

// rename / move the file
shell.exec = jest.fn(() => ({
stdout:
'1539502056\n' +
'\n' +
' rename v1/lib/core/__tests__/__fixtures__/{.temp2 => test/.temp3} (100%)\n' +
'1539502055\n' +
'\n' +
' create mode 100644 v1/lib/core/__tests__/__fixtures__/.temp2\n',
}));
const lastUpdateTime = utils.getGitLastUpdated(tempFilePath3);
// should only consider file content change
expect(lastUpdateTime).toEqual(createTime);
});

test('idx', () => {
Expand Down
32 changes: 28 additions & 4 deletions v1/lib/core/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
const spawn = require('cross-spawn');
const shell = require('shelljs');

const TRUNCATE_MARKER = /<!--\s*truncate\s*-->/;

Expand Down Expand Up @@ -38,10 +38,34 @@ function idx(target, keyPaths) {
);
}

function isNormalInteger(str) {
return /^\d+$/.test(str);
}

function getGitLastUpdated(filepath) {
const timeSpan = spawn
.sync('git', ['log', '-1', '--format=%ct', filepath])
.stdout.toString('utf-8');
// To differentiate between content change and file renaming / moving, use --summary
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for only noticing this now, but we should add a try/catch around the code within getGitLastUpdated and return null in the catch case something blows up (for instance, git is not installed on the device, or if the repository is a Mercurial-based one).

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually im hoping we remove cross-spawn dependency and use existing shelljs to call the bash command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@endiliey yeah i see some v2 code uses shell.exec()

Copy link
Contributor

Choose a reason for hiding this comment

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

V1 too. Check v1/lib/publish-pages.js

// To follow the file history until before it is moved (when we create new version), use
// --follow
const silentState = shell.config.silent; // save old silent state
shell.config.silent = true;
const result = shell
.exec(`git log --follow --summary --format=%ct ${filepath}`)
.stdout.trim();
shell.config.silent = silentState;

// Format the log results to be ['1234567', 'rename ...', '1234566', 'move ...', '1234565', '1234564']
const records = result
.toString('utf-8')
.replace(/\n\s*\n/g, '\n')
.split('\n')
.filter(String);

const timeSpan = records.find((item, index, arr) => {
const isTimestamp = isNormalInteger(item);
const isLastTwoItem = index + 2 >= arr.length;
const nextItemIsTimestamp = isNormalInteger(arr[index + 1]);
return isTimestamp && (isLastTwoItem || nextItemIsTimestamp);
});
if (timeSpan) {
const date = new Date(parseInt(timeSpan, 10) * 1000);
return date.toLocaleString();
Expand Down