From 45fe2bfa572533a16dc481b0886c72b2287b1bfd Mon Sep 17 00:00:00 2001 From: Sebastian McKenzie Date: Wed, 31 May 2017 12:52:26 +0100 Subject: [PATCH 1/4] auto detect and merge lockfile conflicts --- __tests__/commands/_helpers.js | 2 +- __tests__/commands/add.js | 4 +- __tests__/commands/install/integration.js | 6 +- __tests__/lockfile.js | 59 ++++++++++++--- src/lockfile/parse.js | 92 ++++++++++++++++++++++- src/lockfile/wrapper.js | 12 ++- src/rc.js | 2 +- src/registries/yarn-registry.js | 2 +- src/reporters/lang/en.js | 3 + 9 files changed, 162 insertions(+), 20 deletions(-) diff --git a/__tests__/commands/_helpers.js b/__tests__/commands/_helpers.js index a75f62516d..92ea22e41b 100644 --- a/__tests__/commands/_helpers.js +++ b/__tests__/commands/_helpers.js @@ -35,7 +35,7 @@ export async function createLockfile(dir: string): Promise { if (await fs.exists(lockfileLoc)) { const rawLockfile = await fs.readFile(lockfileLoc); - lockfile = parse(rawLockfile); + lockfile = parse(rawLockfile).object; } return new Lockfile(lockfile); diff --git a/__tests__/commands/add.js b/__tests__/commands/add.js index 2fea1c10a1..6d039546b1 100644 --- a/__tests__/commands/add.js +++ b/__tests__/commands/add.js @@ -330,7 +330,7 @@ test.concurrent('add with offline mirror', (): Promise => { ).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', @@ -393,7 +393,7 @@ test.concurrent('install with --save and without offline mirror', (): Promise { 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', ); @@ -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', ); @@ -672,7 +672,7 @@ test.concurrent('offline mirror can be disabled locally', (): Promise => { }; 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', ); diff --git a/__tests__/lockfile.js b/__tests__/lockfile.js index 3f0ddad6f2..3d7a775523 100644 --- a/__tests__/lockfile.js +++ b/__tests__/lockfile.js @@ -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: { @@ -193,3 +193,44 @@ test('Lockfile.getLockfile (sorting)', () => { expect(actual).toEqual(expected); }); + +test('parse merge conflicts', () => { + const file = ` +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.a.no).toEqual('yes'); + expect(object.b.foo).toEqual('bar'); + expect(object.c.bar).toEqual('foo'); + expect(object.d.yes).toEqual('no'); +}); + +test('parse merge conflict fail', () => { + const file = ` +<<<<<<< HEAD +b: + foo: "bar" +======= +c: + bar "foo" +>>>>>>> branch-a +`; + + const {type, object} = parse(file); + expect(type).toEqual('conflict'); + expect(Object.keys(object).length).toEqual(0); +}); diff --git a/src/lockfile/parse.js b/src/lockfile/parse.js index 9fe0a73826..0a8ec170b2 100644 --- a/src/lockfile/parse.js +++ b/src/lockfile/parse.js @@ -314,9 +314,97 @@ export class Parser { } } -export default function(str: string, fileLoc: string = 'lockfile'): Object { - str = stripBOM(str); +const MERGE_CONFLICT_START = '<<<<<<<'; +const MERGE_CONFLICT_SEP = '======='; +const MERGE_CONFLICT_END = '>>>>>>>'; + +/** + * Extract the two versions of the lockfile from a merge conflict. + */ + +export function extractConflictVariants(str: string): Array { + const variants: Array> = [[], []]; + const lines = str.split(/\n/g); + + while (lines.length) { + const line = lines.shift(); + if (line.startsWith(MERGE_CONFLICT_START)) { + // get the first variant + while (lines.length) { + const line = lines.shift(); + if (line === MERGE_CONFLICT_SEP) { + break; + } else { + variants[0].push(line); + } + } + + // get the second variant + while (lines.length) { + const line = lines.shift(); + if (line.startsWith(MERGE_CONFLICT_END)) { + break; + } else { + variants[1].push(line); + } + } + } else { + variants[0].push(line); + variants[1].push(line); + } + } + + return variants.map(lines => lines.join('\n')); +} + +/** + * Check if a lockfile has merge conflicts. + */ + +export function hasMergeConflicts(str: string): boolean { + return str.includes(MERGE_CONFLICT_START); +} + +/** + * Parse the lockfile. + */ + +function parse(str: string, fileLoc: string): Object { const parser = new Parser(str, fileLoc); parser.next(); return parser.parse(); } + +type ParseResult = { + type: 'merge' | 'none' | 'conflict', + object: Object, +}; + +/** + * Parse and merge the two variants in a conflicted lockfile. + */ + +function parseWithConflict(str: string, fileLoc: string): ParseResult { + const variants = extractConflictVariants(str); + + try { + const obj = Object.assign({}, parse(variants[0], fileLoc), parse(variants[1], fileLoc)); + return {type: 'merge', object: obj}; + } 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); + + if (hasMergeConflicts(str)) { + return parseWithConflict(str, fileLoc); + } else { + return {type: 'none', object: parse(str, fileLoc)}; + } +} diff --git a/src/lockfile/wrapper.js b/src/lockfile/wrapper.js index e328bc5cf2..338dc29683 100644 --- a/src/lockfile/wrapper.js +++ b/src/lockfile/wrapper.js @@ -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')); diff --git a/src/rc.js b/src/rc.js index e3894cab0b..3eeba66d7f 100644 --- a/src/rc.js +++ b/src/rc.js @@ -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) { diff --git a/src/registries/yarn-registry.js b/src/registries/yarn-registry.js index e4633ebd85..0f79fae2a2 100644 --- a/src/registries/yarn-registry.js +++ b/src/registries/yarn-registry.js @@ -72,7 +72,7 @@ export default class YarnRegistry extends NpmRegistry { async loadConfig(): Promise { 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; diff --git a/src/reporters/lang/en.js b/src/reporters/lang/en.js index 0cfcd4f808..9675e473e4 100644 --- a/src/reporters/lang/en.js +++ b/src/reporters/lang/en.js @@ -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: From 2003e8bacc071af60d570c6b21f0a18a8f5efcec Mon Sep 17 00:00:00 2001 From: cpojer Date: Thu, 6 Jul 2017 15:25:42 +0100 Subject: [PATCH 2/4] Skip over common ancestors when using diff3. --- __tests__/lockfile.js | 58 ++++++++++++++++++++++++++++++++++++++++++- src/lockfile/parse.js | 10 ++++++-- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/__tests__/lockfile.js b/__tests__/lockfile.js index 3d7a775523..35a7de103f 100644 --- a/__tests__/lockfile.js +++ b/__tests__/lockfile.js @@ -194,7 +194,7 @@ test('Lockfile.getLockfile (sorting)', () => { expect(actual).toEqual(expected); }); -test('parse merge conflicts', () => { +test('parse single merge conflict', () => { const file = ` a: no "yes" @@ -219,6 +219,41 @@ d: expect(object.d.yes).toEqual('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.a.no).toEqual('yes'); + expect(object.b.foo).toEqual('bar'); + expect(object.c.bar).toEqual('foo'); + expect(object.d.yes).toEqual('no'); + expect(object.e.foo).toEqual('bar'); + expect(object.f.bar).toEqual('foo'); +}); + test('parse merge conflict fail', () => { const file = ` <<<<<<< HEAD @@ -234,3 +269,24 @@ c: 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.b.foo).toEqual('bar'); + expect(object.c.bar).toEqual('foo'); + expect(object.d).toBe(undefined); +}); diff --git a/src/lockfile/parse.js b/src/lockfile/parse.js index 0a8ec170b2..b1ecf42094 100644 --- a/src/lockfile/parse.js +++ b/src/lockfile/parse.js @@ -314,9 +314,10 @@ export class Parser { } } -const MERGE_CONFLICT_START = '<<<<<<<'; -const MERGE_CONFLICT_SEP = '======='; +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. @@ -325,6 +326,7 @@ const MERGE_CONFLICT_END = '>>>>>>>'; export function extractConflictVariants(str: string): Array { const variants: Array> = [[], []]; const lines = str.split(/\n/g); + let skip = false; while (lines.length) { const line = lines.shift(); @@ -333,7 +335,11 @@ export function extractConflictVariants(str: string): Array { while (lines.length) { const line = lines.shift(); if (line === MERGE_CONFLICT_SEP) { + skip = false; break; + } else if (skip || line.startsWith(MERGE_CONFLICT_ANCESTOR)) { + skip = true; + continue; } else { variants[0].push(line); } From 6eef7da5c9a8ea8839e6561403a8bbbc4cd5e916 Mon Sep 17 00:00:00 2001 From: cpojer Date: Thu, 6 Jul 2017 15:34:11 +0100 Subject: [PATCH 3/4] Small code cleanups --- src/lockfile/parse.js | 47 +++++++++++++++++-------------------------- 1 file changed, 18 insertions(+), 29 deletions(-) diff --git a/src/lockfile/parse.js b/src/lockfile/parse.js index b1ecf42094..96bce098fe 100644 --- a/src/lockfile/parse.js +++ b/src/lockfile/parse.js @@ -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 = { @@ -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 { let lastNewline = false; let line = 1; @@ -322,9 +327,8 @@ const MERGE_CONFLICT_START = '<<<<<<<'; /** * Extract the two versions of the lockfile from a merge conflict. */ - -export function extractConflictVariants(str: string): Array { - const variants: Array> = [[], []]; +function extractConflictVariants(str: string): [string, string] { + const variants = [[], []]; const lines = str.split(/\n/g); let skip = false; @@ -360,42 +364,32 @@ export function extractConflictVariants(str: string): Array { } } - return variants.map(lines => lines.join('\n')); + return [variants[0].join('\n'), variants[1].join('\n')]; } /** * Check if a lockfile has merge conflicts. */ - -export function hasMergeConflicts(str: string): boolean { +function hasMergeConflicts(str: string): boolean { return str.includes(MERGE_CONFLICT_START); } /** * Parse the lockfile. */ - function parse(str: string, fileLoc: string): Object { const parser = new Parser(str, fileLoc); parser.next(); return parser.parse(); } -type ParseResult = { - type: 'merge' | 'none' | 'conflict', - object: Object, -}; - /** * Parse and merge the two variants in a conflicted lockfile. */ - function parseWithConflict(str: string, fileLoc: string): ParseResult { const variants = extractConflictVariants(str); - try { - const obj = Object.assign({}, parse(variants[0], fileLoc), parse(variants[1], fileLoc)); - return {type: 'merge', object: obj}; + return {type: 'merge', object: Object.assign({}, parse(variants[0], fileLoc), parse(variants[1], fileLoc))}; } catch (err) { if (err instanceof SyntaxError) { return {type: 'conflict', object: {}}; @@ -407,10 +401,5 @@ function parseWithConflict(str: string, fileLoc: string): ParseResult { export default function(str: string, fileLoc: string = 'lockfile'): ParseResult { str = stripBOM(str); - - if (hasMergeConflicts(str)) { - return parseWithConflict(str, fileLoc); - } else { - return {type: 'none', object: parse(str, fileLoc)}; - } + return hasMergeConflicts(str) ? parseWithConflict(str, fileLoc) : {type: 'none', object: parse(str, fileLoc)}; } From 2c8a0b2d809505b018faa3bd230cedb715b2bd17 Mon Sep 17 00:00:00 2001 From: cpojer Date: Fri, 7 Jul 2017 13:23:41 +0100 Subject: [PATCH 4/4] Nits --- __tests__/lockfile.js | 30 ++++++++++++++++++------------ src/lockfile/parse.js | 16 ++++++++-------- src/registries/npm-registry.js | 1 - 3 files changed, 26 insertions(+), 21 deletions(-) diff --git a/__tests__/lockfile.js b/__tests__/lockfile.js index 35a7de103f..2abae7f40a 100644 --- a/__tests__/lockfile.js +++ b/__tests__/lockfile.js @@ -213,10 +213,12 @@ d: const {type, object} = parse(file); expect(type).toEqual('merge'); - expect(object.a.no).toEqual('yes'); - expect(object.b.foo).toEqual('bar'); - expect(object.c.bar).toEqual('foo'); - expect(object.d.yes).toEqual('no'); + expect(object).toEqual({ + a: {no: 'yes'}, + b: {foo: 'bar'}, + c: {bar: 'foo'}, + d: {yes: 'no'}, + }); }); test('parse multiple merge conflicts', () => { @@ -246,12 +248,14 @@ f: const {type, object} = parse(file); expect(type).toEqual('merge'); - expect(object.a.no).toEqual('yes'); - expect(object.b.foo).toEqual('bar'); - expect(object.c.bar).toEqual('foo'); - expect(object.d.yes).toEqual('no'); - expect(object.e.foo).toEqual('bar'); - expect(object.f.bar).toEqual('foo'); + 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', () => { @@ -286,7 +290,9 @@ c: const {type, object} = parse(file); expect(type).toEqual('merge'); - expect(object.b.foo).toEqual('bar'); - expect(object.c.bar).toEqual('foo'); + expect(object).toEqual({ + b: {foo: 'bar'}, + c: {bar: 'foo'}, + }); expect(object.d).toBe(undefined); }); diff --git a/src/lockfile/parse.js b/src/lockfile/parse.js index 96bce098fe..d987e729ad 100644 --- a/src/lockfile/parse.js +++ b/src/lockfile/parse.js @@ -337,25 +337,25 @@ function extractConflictVariants(str: string): [string, string] { if (line.startsWith(MERGE_CONFLICT_START)) { // get the first variant while (lines.length) { - const line = lines.shift(); - if (line === MERGE_CONFLICT_SEP) { + const conflictLine = lines.shift(); + if (conflictLine === MERGE_CONFLICT_SEP) { skip = false; break; - } else if (skip || line.startsWith(MERGE_CONFLICT_ANCESTOR)) { + } else if (skip || conflictLine.startsWith(MERGE_CONFLICT_ANCESTOR)) { skip = true; continue; } else { - variants[0].push(line); + variants[0].push(conflictLine); } } // get the second variant while (lines.length) { - const line = lines.shift(); - if (line.startsWith(MERGE_CONFLICT_END)) { + const conflictLine = lines.shift(); + if (conflictLine.startsWith(MERGE_CONFLICT_END)) { break; } else { - variants[1].push(line); + variants[1].push(conflictLine); } } } else { @@ -371,7 +371,7 @@ function extractConflictVariants(str: string): [string, string] { * Check if a lockfile has merge conflicts. */ function hasMergeConflicts(str: string): boolean { - return str.includes(MERGE_CONFLICT_START); + return str.includes(MERGE_CONFLICT_START) && str.includes(MERGE_CONFLICT_SEP) && str.includes(MERGE_CONFLICT_END); } /** diff --git a/src/registries/npm-registry.js b/src/registries/npm-registry.js index 02d0added4..63b971d3c2 100644 --- a/src/registries/npm-registry.js +++ b/src/registries/npm-registry.js @@ -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');