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

Auto detect and merge lockfile conflicts #3544

Merged
merged 4 commits into from
Jul 7, 2017
Merged
Show file tree
Hide file tree
Changes from all 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 __tests__/commands/_helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export async function createLockfile(dir: string): Promise<Lockfile> {

if (await fs.exists(lockfileLoc)) {
const rawLockfile = await fs.readFile(lockfileLoc);
lockfile = parse(rawLockfile);
lockfile = parse(rawLockfile).object;
}

return new Lockfile(lockfile);
Expand Down
4 changes: 2 additions & 2 deletions __tests__/commands/add.js
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ test.concurrent('add with offline mirror', (): Promise<void> => {
).toBeGreaterThanOrEqual(0);

const rawLockfile = await fs.readFile(path.join(config.cwd, constants.LOCKFILE_FILENAME));
const lockfile = parse(rawLockfile);
const {object: lockfile} = parse(rawLockfile);

expect(lockfile['is-array@^1.0.1']['resolved']).toEqual(
'https://registry.yarnpkg.com/is-array/-/is-array-1.0.1.tgz#e9850cc2cc860c3bc0977e84ccf0dd464584279a',
Expand Down Expand Up @@ -393,7 +393,7 @@ test.concurrent('install with --save and without offline mirror', (): Promise<vo
).toEqual(-1);

const rawLockfile = await fs.readFile(path.join(config.cwd, constants.LOCKFILE_FILENAME));
const lockfile = parse(rawLockfile);
const {object: lockfile} = parse(rawLockfile);

expect(lockfile['is-array@^1.0.1']['resolved']).toMatch(
/https:\/\/registry\.yarnpkg\.com\/is-array\/-\/is-array-1\.0\.1\.tgz#[a-f0-9]+/,
Expand Down
6 changes: 3 additions & 3 deletions __tests__/commands/install/integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,7 @@ test.concurrent('offline mirror can be enabled from parent dir', (): Promise<voi
};
return runInstall({}, fixture, async (config, reporter) => {
const rawLockfile = await fs.readFile(path.join(config.cwd, 'yarn.lock'));
const lockfile = parse(rawLockfile);
const {object: lockfile} = parse(rawLockfile);
expect(lockfile['mime-types@2.1.14'].resolved).toEqual(
'https://registry.yarnpkg.com/mime-types/-/mime-types-2.1.14.tgz#f7ef7d97583fcaf3b7d282b6f8b5679dab1e94ee',
);
Expand All @@ -657,7 +657,7 @@ test.concurrent('offline mirror can be enabled from parent dir, with merging of
};
return runInstall({}, fixture, async (config, reporter) => {
const rawLockfile = await fs.readFile(path.join(config.cwd, 'yarn.lock'));
const lockfile = parse(rawLockfile);
const {object: lockfile} = parse(rawLockfile);
expect(lockfile['mime-types@2.1.14'].resolved).toEqual(
'https://registry.yarnpkg.com/mime-types/-/mime-types-2.1.14.tgz#f7ef7d97583fcaf3b7d282b6f8b5679dab1e94ee',
);
Expand All @@ -672,7 +672,7 @@ test.concurrent('offline mirror can be disabled locally', (): Promise<void> => {
};
return runInstall({}, fixture, async (config, reporter) => {
const rawLockfile = await fs.readFile(path.join(config.cwd, 'yarn.lock'));
const lockfile = parse(rawLockfile);
const {object: lockfile} = parse(rawLockfile);
expect(lockfile['mime-types@2.1.14'].resolved).toEqual(
'https://registry.yarnpkg.com/mime-types/-/mime-types-2.1.14.tgz#f7ef7d97583fcaf3b7d282b6f8b5679dab1e94ee',
);
Expand Down
121 changes: 112 additions & 9 deletions __tests__/lockfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,19 @@ const objs = [{foo: 'bar'}, {foo: {}}, {foo: 'foo', bar: 'bar'}, {foo: 5}];
let i = 0;
for (const obj of objs) {
test(`parse/stringify ${++i}`, () => {
expect(parse(stringify(obj))).toEqual(nullify(obj));
expect(parse(stringify(obj)).object).toEqual(nullify(obj));
});
}

test('parse', () => {
expect(parse('foo "bar"')).toEqual(nullify({foo: 'bar'}));
expect(parse('"foo" "bar"')).toEqual(nullify({foo: 'bar'}));
expect(parse('foo "bar"')).toEqual(nullify({foo: 'bar'}));

expect(parse(`foo:\n bar "bar"`)).toEqual(nullify({foo: {bar: 'bar'}}));
expect(parse(`foo:\n bar:\n foo "bar"`)).toEqual(nullify({foo: {bar: {}, foo: 'bar'}}));
expect(parse(`foo:\n bar:\n foo "bar"`)).toEqual(nullify({foo: {bar: {foo: 'bar'}}}));
expect(parse('foo:\n bar:\n yes no\nbar:\n yes no')).toEqual(
expect(parse('foo "bar"').object).toEqual(nullify({foo: 'bar'}));
expect(parse('"foo" "bar"').object).toEqual(nullify({foo: 'bar'}));
expect(parse('foo "bar"').object).toEqual(nullify({foo: 'bar'}));

expect(parse(`foo:\n bar "bar"`).object).toEqual(nullify({foo: {bar: 'bar'}}));
expect(parse(`foo:\n bar:\n foo "bar"`).object).toEqual(nullify({foo: {bar: {}, foo: 'bar'}}));
expect(parse(`foo:\n bar:\n foo "bar"`).object).toEqual(nullify({foo: {bar: {foo: 'bar'}}}));
expect(parse('foo:\n bar:\n yes no\nbar:\n yes no').object).toEqual(
nullify({
foo: {
bar: {
Expand Down Expand Up @@ -193,3 +193,106 @@ test('Lockfile.getLockfile (sorting)', () => {

expect(actual).toEqual(expected);
});

test('parse single merge conflict', () => {
const file = `
Copy link
Member

Choose a reason for hiding this comment

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

Shall we not put these into a separate file? I can see both ways so just raising the question, not strong opinions.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's much easier to manage it like this, and we care more about the text input than we care about where it is coming from for the purpose of this test.

a:
no "yes"

<<<<<<< HEAD
b:
foo "bar"
=======
c:
bar "foo"
>>>>>>> branch-a

d:
yes "no"
`;

const {type, object} = parse(file);
expect(type).toEqual('merge');
expect(object).toEqual({
a: {no: 'yes'},
b: {foo: 'bar'},
c: {bar: 'foo'},
d: {yes: 'no'},
});
});

test('parse multiple merge conflicts', () => {
const file = `
a:
no "yes"

<<<<<<< HEAD
b:
foo "bar"
=======
c:
bar "foo"
>>>>>>> branch-a

d:
yes "no"

<<<<<<< HEAD
e:
foo "bar"
=======
f:
bar "foo"
>>>>>>> branch-b
`;

const {type, object} = parse(file);
expect(type).toEqual('merge');
expect(object).toEqual({
a: {no: 'yes'},
b: {foo: 'bar'},
c: {bar: 'foo'},
d: {yes: 'no'},
e: {foo: 'bar'},
f: {bar: 'foo'},
});
});

test('parse merge conflict fail', () => {
const file = `
<<<<<<< HEAD
b:
foo: "bar"
=======
Copy link
Member

Choose a reason for hiding this comment

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

Why not try adding both, which is most probably what is needed when installing packages?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, it might not be obvious but in this example, the format is actually wrong because yarn lockfiles don't support colons :, which is why it results in a conflict.

c:
bar "foo"
>>>>>>> branch-a
`;

const {type, object} = parse(file);
expect(type).toEqual('conflict');
expect(Object.keys(object).length).toEqual(0);
});

test('discards common ancestors in merge conflicts', () => {
const file = `
<<<<<<< HEAD
b:
foo "bar"
||||||| common ancestor
d:
yes "no"
=======
c:
bar "foo"
>>>>>>> branch-a
`;

const {type, object} = parse(file);
expect(type).toEqual('merge');
expect(object).toEqual({
b: {foo: 'bar'},
c: {bar: 'foo'},
});
expect(object.d).toBe(undefined);
});
101 changes: 92 additions & 9 deletions src/lockfile/parse.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,18 @@ import map from '../util/map.js';
const invariant = require('invariant');
const stripBOM = require('strip-bom');

type Token = {
line: number,
col: number,
type: string,
value: boolean | number | string | void,
};

type ParseResult = {
type: 'merge' | 'none' | 'conflict',
object: Object,
};

const VERSION_REGEX = /^yarn lockfile v(\d+)$/;

const TOKEN_TYPES = {
Expand All @@ -30,13 +42,6 @@ function isValidPropValueToken(token): boolean {
return VALID_PROP_VALUE_TOKENS.indexOf(token.type) >= 0;
}

type Token = {
line: number,
col: number,
type: string,
value: boolean | number | string | void,
};

export function* tokenise(input: string): Iterator<Token> {
let lastNewline = false;
let line = 1;
Expand Down Expand Up @@ -314,9 +319,87 @@ export class Parser {
}
}

export default function(str: string, fileLoc: string = 'lockfile'): Object {
str = stripBOM(str);
const MERGE_CONFLICT_ANCESTOR = '|||||||';
const MERGE_CONFLICT_END = '>>>>>>>';
const MERGE_CONFLICT_SEP = '=======';
const MERGE_CONFLICT_START = '<<<<<<<';

/**
* Extract the two versions of the lockfile from a merge conflict.
*/
function extractConflictVariants(str: string): [string, string] {
const variants = [[], []];
const lines = str.split(/\n/g);
let skip = false;

while (lines.length) {
const line = lines.shift();
if (line.startsWith(MERGE_CONFLICT_START)) {
// get the first variant
while (lines.length) {
const conflictLine = lines.shift();
if (conflictLine === MERGE_CONFLICT_SEP) {
skip = false;
break;
} else if (skip || conflictLine.startsWith(MERGE_CONFLICT_ANCESTOR)) {
skip = true;
continue;
} else {
variants[0].push(conflictLine);
}
}

// get the second variant
while (lines.length) {
const conflictLine = lines.shift();
if (conflictLine.startsWith(MERGE_CONFLICT_END)) {
break;
} else {
variants[1].push(conflictLine);
}
}
} else {
variants[0].push(line);
variants[1].push(line);
}
}

return [variants[0].join('\n'), variants[1].join('\n')];
}

/**
* Check if a lockfile has merge conflicts.
*/
function hasMergeConflicts(str: string): boolean {
return str.includes(MERGE_CONFLICT_START) && str.includes(MERGE_CONFLICT_SEP) && str.includes(MERGE_CONFLICT_END);
}

/**
* Parse the lockfile.
*/
function parse(str: string, fileLoc: string): Object {
const parser = new Parser(str, fileLoc);
parser.next();
return parser.parse();
}

/**
* Parse and merge the two variants in a conflicted lockfile.
*/
function parseWithConflict(str: string, fileLoc: string): ParseResult {
const variants = extractConflictVariants(str);
try {
return {type: 'merge', object: Object.assign({}, parse(variants[0], fileLoc), parse(variants[1], fileLoc))};
} catch (err) {
if (err instanceof SyntaxError) {
return {type: 'conflict', object: {}};
} else {
throw err;
}
}
}

export default function(str: string, fileLoc: string = 'lockfile'): ParseResult {
str = stripBOM(str);
return hasMergeConflicts(str) ? parseWithConflict(str, fileLoc) : {type: 'none', object: parse(str, fileLoc)};
}
12 changes: 11 additions & 1 deletion src/lockfile/wrapper.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,17 @@ export default class Lockfile {

if (await fs.exists(lockfileLoc)) {
rawLockfile = await fs.readFile(lockfileLoc);
lockfile = parse(rawLockfile, lockfileLoc);
const lockResult = parse(rawLockfile, lockfileLoc);

if (reporter) {
if (lockResult.type === 'merge') {
reporter.info(reporter.lang('lockfileMerged'));
} else if (lockResult.type === 'conflict') {
reporter.warn(reporter.lang('lockfileConflict'));
}
}

lockfile = lockResult.object;
} else {
if (reporter) {
reporter.info(reporter.lang('noLockfileFound'));
Expand Down
2 changes: 1 addition & 1 deletion src/rc.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ let rcArgsCache;

const buildRcConf = () =>
rcUtil.findRc('yarn', (fileText, filePath) => {
const values = parse(fileText, 'yarnrc');
const {object: values} = parse(fileText, 'yarnrc');
const keys = Object.keys(values);

for (const key of keys) {
Expand Down
1 change: 0 additions & 1 deletion src/registries/npm-registry.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import envReplace from '../util/env-replace.js';
import Registry from './base-registry.js';
import {addSuffix} from '../util/misc';
import {getPosixPath, resolveWithHome} from '../util/path';
import isRequestToRegistry from './is-request-to-registry.js';

const userHome = require('../util/user-home-dir').default;
const path = require('path');
Expand Down
2 changes: 1 addition & 1 deletion src/registries/yarn-registry.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export default class YarnRegistry extends NpmRegistry {

async loadConfig(): Promise<void> {
for (const [isHome, loc, file] of await this.getPossibleConfigLocations('.yarnrc', this.reporter)) {
const config = parse(file, loc);
const {object: config} = parse(file, loc);

if (isHome) {
this.homeConfig = config;
Expand Down
3 changes: 3 additions & 0 deletions src/reporters/lang/en.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ const messages = {
shrinkwrapWarning:
'npm-shrinkwrap.json found. This will not be updated or respected. See https://yarnpkg.com/en/docs/migrating-from-npm for more information.',
lockfileOutdated: 'Outdated lockfile. Please run `yarn install` and try again.',
lockfileMerged: 'Merge conflict detected in yarn.lock and successfully merged.',
lockfileConflict:
'A merge conflict was found in yarn.lock but it could not be successfully merged, regenerating yarn.lock from scratch.',
ignoredScripts: 'Ignored scripts due to flag.',
missingAddDependencies: 'Missing list of packages to add to your project.',
yesWarning:
Expand Down