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

Already on GitHub? Sign in to your account

Auto detect and merge lockfile conflicts #3544

Merged
merged 4 commits into from Jul 7, 2017

Conversation

Projects

Done in Yarn 1.0

Owner

kittens commented May 31, 2017 edited

Summary

Git merge conflicts are common with yarn.lock. This PR allows you to run yarn to automatically merge the two versions of yarn.lock which will run Yarn and regenerate a new lockfile.

Test plan

Added tests.

@cpojer cpojer requested a review from arcanis May 31, 2017

src/lockfile/parse.js
@@ -314,9 +314,80 @@ export class Parser {
}
}
-export default function(str: string, fileLoc: string = 'lockfile'): Object {
- str = stripBOM(str);
+const GIT_MERGE_CONFLICT_START = '<<<<<<<';
@cpojer

cpojer May 31, 2017

Owner

Drop the "GIT_", other tools will generate similar conflict markers.

src/lockfile/parse.js
+ if (line === MERGE_CONFLICT_SEP) {
+ break;
+ } else {
+ variantOne.push(line);
@arcanis

arcanis May 31, 2017

Owner

can't you just push directly into variants[0]? or at least use let variantOne = variants[0] so that the concat becomes unneeded

src/lockfile/parse.js
+ variants[0] = variants[0].concat(variantOne);
+
+ // get the second variant
+ let variantTwo = [];
@arcanis

arcanis May 31, 2017

Owner

same as above

src/lockfile/parse.js
+ let variantTwo = [];
+ while (lines.length) {
+ const line = lines.shift();
+ if (line.startsWith(MERGE_CONFLCIT_END)) {
Owner

kittens commented May 31, 2017

Addressed nits, thanks @arcanis!

Owner

arcanis commented May 31, 2017

@kittens The tests seem to break here, and in a few other place (which all seem related to the resolver .. which is weird since your PR doesn't alter it ôo)

src/lockfile/parse.js
+ // get the first variant
+ while (lines.length) {
+ const line = lines.shift();
+ if (line === MERGE_CONFLICT_SEP) {
@dustinspecker

dustinspecker May 31, 2017

This feature looks awesome.

Curious if you all are wanting to support git's diff3 merge conflict style (https://git-scm.com/docs/git-merge#_how_conflicts_are_presented). I think this code will break for users using diff3 because the first variant would include both the real variant and the common parent ancestor of both conflicts. I wouldn't mind making a PR in the future for supporting diff3.

Sorry if I'm overlooking something.

src/lockfile/parse.js
+ */
+
+function parseWithConflict(str: string, fileLoc: string): ParseResult {
+ const variants = extractConflictVariants(str);
@sejoker

sejoker May 31, 2017

We assume here that only 1 merge conflict exists. We could handle 2+ conflicts if call parseWithConflict recursively.

@arcanis

arcanis May 31, 2017

Owner

It supports multiple conflicts in the same file, but during a same conflict pass. I think it should be good enough for a first iteration :)

@sejoker

sejoker Jun 1, 2017

@arcanis cool, would be nice to add an extra test for in that case.

__tests__/lockfile.js
@@ -193,3 +193,44 @@ test('Lockfile.getLockfile (sorting)', () => {
expect(actual).toEqual(expected);
});
+
+test('parse merge conflicts', () => {
@sejoker

sejoker May 31, 2017

better name -> 'parse single merge conflict'

@kittens neat! Could you please add git diff3 conflictstyle support?
http://psung.blogspot.it/2011/02/reducing-merge-headaches-git-meets.html?m=1

@BYK BYK added to In flight (review/code) in Yarn 1.0 Jul 5, 2017

@arcanis arcanis moved from Awaiting Review to Active in Yarn 1.0 Jul 6, 2017

@cpojer cpojer self-assigned this Jul 6, 2017

@cpojer cpojer requested review from BYK and arcanis Jul 6, 2017

Owner

cpojer commented Jul 6, 2017

Rebased, fixed up the tests, made flow more strict. Added support for diff3 by discarding the common ancestor from the parsed result. It seems to me like the information shown is purely for humans to make better assumptions about what to do in case of a conflict, which doesn't apply here. Let me know if that is wrong.

Anything else needed here to get this merged?

@BYK BYK moved from Active to Awaiting Review in Yarn 1.0 Jul 6, 2017

BYK approved these changes Jul 7, 2017

Overall LGTM.

One question: should we make this behavior opt-in via a --merge flag?

@@ -193,3 +193,101 @@ test('Lockfile.getLockfile (sorting)', () => {
expect(actual).toEqual(expected);
});
+
+test('parse single merge conflict', () => {
+ const file = `
@BYK

BYK Jul 7, 2017

Owner

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

@cpojer

cpojer Jul 7, 2017

Owner

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.

__tests__/lockfile.js
+
+ const {type, object} = parse(file);
+ expect(type).toEqual('merge');
+ expect(object.a.no).toEqual('yes');
@BYK

BYK Jul 7, 2017

Owner

Why not

expect(object).toEqual({
    a: { no: 'yes' },
    b: { foo: 'bar' },
    c: { bar: 'foo' },
    d: { yes: 'no' },
});
+<<<<<<< HEAD
+b:
+ foo: "bar"
+=======
@BYK

BYK Jul 7, 2017

Owner

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

@cpojer

cpojer Jul 7, 2017

Owner

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.

src/lockfile/parse.js
+
+export function extractConflictVariants(str: string): Array<string> {
+ const variants: Array<Array<string>> = [[], []];
+ const lines = str.split(/\n/g);
@BYK

BYK Jul 7, 2017

Owner

Is this safe or should we do [\r\n] instead?

src/lockfile/parse.js
+ if (line.startsWith(MERGE_CONFLICT_START)) {
+ // get the first variant
+ while (lines.length) {
+ const line = lines.shift();
@BYK

BYK Jul 7, 2017

Owner

Better to not mask the outer variable?

src/lockfile/parse.js
+ * Check if a lockfile has merge conflicts.
+ */
+function hasMergeConflicts(str: string): boolean {
+ return str.includes(MERGE_CONFLICT_START);
@BYK

BYK Jul 7, 2017

Owner

Wouldn't it be better to check if all 3 markers exist?

kittens and others added some commits May 31, 2017

@cpojer cpojer merged commit 3bfa1e3 into master Jul 7, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Owner

cpojer commented Jul 7, 2017

I just tried this on Jest facebook/jest#3906 and it worked flawlessly, this feature is so awesome. @kittens thanks for the idea and getting us 99% there!

screen shot 2017-07-07 at 2 06 16 pm

@cpojer cpojer deleted the lockfile-conflicts branch Jul 7, 2017

@cpojer cpojer moved from Awaiting Review to Done in Yarn 1.0 Jul 7, 2017

@BYK BYK moved from Awaiting Review to Done in Yarn 1.0 Jul 10, 2017

BYK added a commit that referenced this pull request Aug 17, 2017

Fix: Lockfile should be updated when merge conflcits are resolved
**Summary**

Follow up to #3544. Currently, `yarn` happily keeps moving if it
detects merge conflicts and is able to resolve them in the lockfile.
That said it doesn't persist the resolution by saving the lockfile
to disk again. This patch ensures writing the lockfile if it is
"dirty".

This patch also causes `yarn` to throw an error if there are merge
conflicts in the file and `--frozen-lockfile` option is true.

**Test plan**

Existing unit tests. Also try running `yarn install` with the following
files:
`package.json`

```
{
  "name": "yarnlock-auto-merge",
  "version": "1.0.0",
  "main": "index.js",
  "author": "Burak Yigit Kaya <byk@fb.com>",
  "license": "MIT",
  "dependencies": {
    "left-pad": "^1.1.3",
    "right-pad": "^1.0.1"
  }
}
```

`yarn.lock`

```

<<<<<<< HEAD
left-pad@^1.1.3:
  version "1.1.3"
  resolved "https://registry.yarnpkg.com/left-pad/-/left-pad-1.1.3.tgz#612f61c033f3a9e08e939f1caebeea41b6f3199a"
=======
right-pad@^1.0.1:
  version "1.0.1"
  resolved "https://registry.yarnpkg.com/right-pad/-/right-pad-1.0.1.tgz#8ca08c2cbb5b55e74dafa96bf7fd1a27d568c8d0"
>>>>>>> right-pad
```

Without the patch, `yarn` won't update the lockfile. With the patch,
the lockfile is replaced with the merged version.

BYK added a commit that referenced this pull request Aug 18, 2017

Fix: Lockfile should be updated when merge conflcits are resolved (#4195
)

**Summary**

Follow up to #3544. Currently, `yarn` happily keeps moving if it
detects merge conflicts and is able to resolve them in the lockfile.
That said it doesn't persist the resolution by saving the lockfile
to disk again. This patch ensures writing the lockfile if it is
"dirty".

This patch also causes `yarn` to throw an error if there are merge
conflicts in the file and `--frozen-lockfile` option is true.

**Test plan**

Existing unit tests. Also try running `yarn install` with the following
files:
`package.json`

```
{
  "name": "yarnlock-auto-merge",
  "version": "1.0.0",
  "main": "index.js",
  "author": "Burak Yigit Kaya <byk@fb.com>",
  "license": "MIT",
  "dependencies": {
    "left-pad": "^1.1.3",
    "right-pad": "^1.0.1"
  }
}
```

`yarn.lock`

```

<<<<<<< HEAD
left-pad@^1.1.3:
  version "1.1.3"
  resolved "https://registry.yarnpkg.com/left-pad/-/left-pad-1.1.3.tgz#612f61c033f3a9e08e939f1caebeea41b6f3199a"
=======
right-pad@^1.0.1:
  version "1.0.1"
  resolved "https://registry.yarnpkg.com/right-pad/-/right-pad-1.0.1.tgz#8ca08c2cbb5b55e74dafa96bf7fd1a27d568c8d0"
>>>>>>> right-pad
```

Without the patch, `yarn` won't update the lockfile. With the patch,
the lockfile is replaced with the merged version.

karldanninger commented Sep 8, 2017 edited

A+ 👍 👍

wodin commented Sep 26, 2017

@cpojer, it's a bad habit to do this:
$ rm -rf packages/*/yarn.lock
Sorry. Pet peeve :) There's no recursion happening there so why use -r? Also it's unlikely you'd need the -f.
But I know of someone at a small ISP who was trying to delete a mailbox and typed "rm -rf /var/spool/mail/" and then pasted the username and pressed enter. When it took longer than expected he suddenly realised there was a space before the username. A lot of mail was lost. Would not have been a problem if he had not used "-rf".
I suspect someone (possibly your distribution) encouraged that habit by aliasing "rm" to "rm -i", but the fix for that is to remove the stupid alias! :) Not to use "-rf" as a matter of course.
Anyway, that's enough of a rant from me. Sorry for the off-topic comment.

raix commented Sep 27, 2017

Would be nice to update the # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. in yarn.lock adding a hint to guide the user?
(sorry about the late note)

Vanuan commented Sep 27, 2017

What are implications of using yarn install --pure-lock-file wrt resolving conflicts?

Owner

BYK commented Sep 27, 2017

@Vanuan I expect that to not change the file at all but use the merged version in-memory.

Vanuan commented Sep 27, 2017

So how do I resolve conflicts without upgrading packages? Only by hand?

Owner

BYK commented Sep 27, 2017

@Vanuan I don't really follow that reasoning. yarn install without --pure-lockfile wouldn't (or at least shoudn't) upgrade packages.

Vanuan commented Sep 27, 2017 edited

Hm. Somehow I was under impression that it would. I've seen 3 situations where yarn.lock ends up changed:

  1. edit a version in package.json
  2. run yarn add/remove
  3. remove yarn.lock to resolve merge conflicts and run yarn install

The 3rd situation is mitigated by this PR. But it isn't clear whether conflicted packages would be resolved again potentially upgrading the version of unrelated packages.
The 2nd situation might upgrade versions if there are requirements of installed/removed packages changed (and it doesn't conflict with package.json)
But the 1st situation I'm not sure about. Might be the same as 2nd.

Well, it looks like --pure-lock-file only helps if package.json constraints don't match yarn.lock. I.e. when yarn install would change yarn.lock. Probably that situation should be catched by CI:

  1. run yarn install
  2. check if git diff --exit-code fails and faild the build with message "yarn.lock isn't up to date"

In this case --pure-lock-file might not be needed at all.

Vanuan commented Sep 27, 2017 edited

The "upgrade" i'm referring to is when yarn install fetches new version that satisfies the version in package.json

  1. yarn install with package@^1.0.0 generates lock file with package@1.0.1
  2. package 1.1.0 is released
  3. yarn install with package@^1.0.0 generates lock file with package@1.1.0

So the question is whether package version gets resolved again in case of merge conflicts.

Owner

BYK commented Sep 29, 2017

@Vanuan it is quite hard to keep track of what needs to be done under a closed PR. Would you mind summarizing this in a new issue as a feature request or a bug so it is clear to anyone outside of this PR what is asked to be done. This way it would be asier to reason about and work on it.

Vanuan commented Sep 29, 2017

Thanks for you patience. This probably doesn't belong here on GitHub.

Owner

cpojer commented Sep 29, 2017

I'm locking this. The conversation that evolved isn't related to the original issue, if you have concerns about this issue, please send a pull request to fix it. Thanks!

@cpojer cpojer locked and limited conversation to collaborators Sep 29, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.