composer icon indicating copy to clipboard operation
composer copied to clipboard

RFC: Automatically resolve conflicts in composer.lock

Open hkdobrev opened this issue 8 years ago • 31 comments

When merging/rebasing branches it's not that rare that you may encounter merge conflicts in composer.lock. Most of the time the conflict would be in the hash or in a timestamp or in some cases you'd need to choose both blocks on the same lines.

However, it's quite rare you'd actually need to manually resolve something in composer.lock in a custom way.

The usual workflow:

  1. You have a conflict in composer.lock
  2. You run your git-mergetool and chose one of the hashes - doesn't matter which one.
  3. composer update --lock
  4. git add composer.lock
  5. git rebase --continue or git commit (if you're merging).

How about automatically fix the merge conflicts when running composer install or composer update --lock if possible?

This was already implemented in Yarn:

  • Pull request: https://github.com/yarnpkg/yarn/pull/3544
  • Announcement: https://code.facebook.com/posts/274518539716230
  • Screenshot: https://user-images.githubusercontent.com/13352/27958995-8104ab7e-631d-11e7-8050-a36cbb48e75d.png

What do you think?

hkdobrev avatar Dec 21 '17 00:12 hkdobrev

Maybe not very clean but - separate folder like '.composer.lock' having files separate per library or just remove content-hash. In bigger projects with large teams, this becomes a real pain.

adanilowski avatar Dec 21 '17 15:12 adanilowski

@adanilowski I think this is quite a drastic change compared to how the lock file currently works and is a breaking change. My suggestion is around the current implementation and it is similar to how other package managers work. I really appreciate having all dependencies locked into a single file and the hash of dependencies for the warnings it produces.

hkdobrev avatar Dec 21 '17 16:12 hkdobrev

The content-hash serves a real purpose and removing it is not a solution, it might resolve some merge conflicts but would introduce other issues.

Adding a merge functionality that checks the two lock files and sees if they are compatible and still match the current requirements though sounds like something that's feasible and definitely should be looked.

Seldaek avatar Dec 22 '17 10:12 Seldaek

This will be harder than in Yarn though, because our composer.lock file is a JSON file, but a merge conflict makes it unparseable, and so would require to build a special parser for conflicting files (not impossible though).

stof avatar Dec 22 '17 11:12 stof

I suppose we need to detect just the following:

  • is there a merge conflict
  • is the merge conflict just in the hash

If it is, we just need to apply a merge strategy to choose one of the sides without parsing the JSON (it doesn't matter which is in this case). Then run composer update --lock and add the updated file to the index. (I'm assuming Git in all this)

hkdobrev avatar Dec 22 '17 12:12 hkdobrev

Maybe composer could also automatically inject git hook to run this post merge?

ostrolucky avatar Dec 28 '17 22:12 ostrolucky

I have created issue WI-39466. In PHPStorm tracker, I think solving this problem in editor might be more proper place.

ostrolucky avatar Jan 06 '18 20:01 ostrolucky

I have created a custom git merge driver that can be used to minimize merge conflicts in composer.json and composer.lock files. Handling this at the merge step (instead of grappling with the merge conflict markers after the fact) allows for more graceful handling of merges within these files; this only results in conflicts when a dependency's version has changed in multiple branches, which is a legitimate conflict that requires a conscious resolution.

https://github.com/balbuf/composer-git-merge-driver

Please check it out!

balbuf avatar Mar 08 '18 16:03 balbuf

thanks @balbuf, I also think that handling it in as git merge driver is the way to go. I was about to write my own, but luckily found this issue thread first!

glensc avatar Apr 15 '19 08:04 glensc

@Seldaek would you accept a PR which incorporates https://github.com/balbuf/composer-git-merge-driver into the composer codebase (solving the problem vcs independently)?

IMO solving merge conflicts of composer.json/lock is pain point for composer usage and should not be something the composer user should need additional setup for (setup the git merge-driver etc). I guess a lot of composer users dont know git merge drivers at all and therefore deal with those problems manually. additionally the git merge driver thing doesnt work for mercurial or svn etc.

staabm avatar Sep 06 '19 08:09 staabm

@staabm tbh I am medium interested in spending time on this right now (even if you do a PR..) as the way I understand it it still require setting up the git merge driver and so most people still won't benefit from it. Or how do you imagine this?

Seldaek avatar Sep 17 '19 09:09 Seldaek

I think he means solving this problem without the user having to set up anything / not depending on the version control system used.

vierlex avatar Dec 16 '19 13:12 vierlex

@vierlex how would that work ? The previous discussion concluded that things are much easier to handle when implement a git merge driver than when trying to resolve conflicts after running the default git merge. You are asking to give up that solution. So what would you suggest otherwise ? Thus, if they don't setup anything, how would things work out of the box ? Git will simply report the conflict as today as it does not know about composer.

stof avatar Dec 16 '19 16:12 stof

@adanilowski @hkdobrev

Maybe not very clean but - separate folder like '.composer.lock' having files separate per library or just remove content-hash. In bigger projects with large teams, this becomes a real pain.

@adanilowski I think this is quite a drastic change compared to how the lock file currently works and is a breaking change. My suggestion is around the current implementation and it is similar to how other package managers work. I really appreciate having all dependencies locked into a single file and the hash of dependencies for the warnings it produces.

Having the lock data in a directory with distinct file per package could be opt-in, then we would not have to worry about BC as much.

Imo these files could also be more slim, I think the version number is really the only relevant piece of information, right? Perhaps if it was downloaded from an alternative repo..

donquixote avatar Mar 30 '20 22:03 donquixote

The secure solution when having conflicts is to keep other developpers changes, then you replay your changes on top of this.

GOSTANYAN-S avatar Jun 09 '20 08:06 GOSTANYAN-S

I came here to also suggest this :) My problem is usually:

  1. Branch installs/updates a dependency.
  2. During rebase on master, there's a conflict in composer.lock because master also installed/updated a dependency.

I usually solve this by removing composer.lock and running composer install/update. But that unavoidably updates more stuff than I want to - because when using loose constraints like ^1.0 of a library which version I didn't want to touch, it will also update that library.

It looks like Composer could use the current installed.json to reconstruct the old composer.lock file as much as possible, changing only the stuff the user intends to?

ondrejmirtes avatar Jun 22 '20 09:06 ondrejmirtes

I'm also aware this can be solved with a little bit of git-fu:

  1. When the rebase conflicts, do whatever you want with the composer.lock.
  2. You now have your branch on top of the branch you're rebasing on.
  3. Rebase again (interactively) and edit the commit with the composer.lock change
  4. Reset the composer.lock file to the same state as in on top of the main branch.
  5. You can now run your composer require or composer update package command and the composer.lock will end up in the state you want without any other unwanted updates.
  6. Finish the rebase.

I wonder if there's a way for Composer to do the same thing out of the box...

ondrejmirtes avatar Jun 22 '20 09:06 ondrejmirtes

Imo these files could also be more slim, I think the version number is really the only relevant piece of information, right? Perhaps if it was downloaded from an alternative repo..

That's wrong. the composer.lock file is designed so that an install from lock has all the necessary metadata to run the solver and the installer. It will not load metadata from Packagist anymore (which is fortunate, otherwise locking dev versions would not actually lock them as their metadata gets updated to newer commits on packagist).

stof avatar Jun 22 '20 18:06 stof

That's wrong. the composer.lock file is designed so that an install from lock has all the necessary metadata to run the solver and the installer. It will not load metadata from Packagist anymore

But could this not be cached in some place that is not tracked with git? Either on system level or on project level. (the answer to this depends on the questions below)

(which is fortunate, otherwise locking dev versions would not actually lock them as their metadata gets updated to newer commits on packagist).

So are we afraid that a package maintainer would force-push and change the metadata for a specific release tag? Or what else could cause the upstream metadata to change, if we target a specific version number?

to newer commits

if we target a specific version number, then newer commits should not matter, right? unless there is a force-push.

donquixote avatar Jun 22 '20 19:06 donquixote

if we target a specific version number, then newer commits should not matter, right? unless there is a force-push.

Even without a force push, git allows deleting a tag and publishing the same version on a new commit. Also, force pushes in libraries do happen. But I think this is beside the point in this conversation. This is mostly about multiple developers working on the same project having conflicts in composer.lock when merging and rebasing across branches. This is happening way more often. Any remedy should work with existing composer.lock format for BC.

hkdobrev avatar Jun 22 '20 19:06 hkdobrev

I've been using the Composer Git merge driver for some time and it works well for simple cases. It however causes more confusion with more complex cases and it can lead you to think things are OK when they are not. For example, when there are new and removed libraries between the two versions of composer.lock touching the same lines.

If this could be improved in a Git merge driver and if that could be installed in one step, I'm all for resolving it as a Git merge driver instead of as part of Composer. If these cases require an in-house handling in Composer, let's discuss that.

In terms of developer experience, I particularly like the Yarn approach as you'd normally need to run install after switching branches, rebasing or merging anyway. It's a good habit and I've even built a small tool to automate commands after changes to locks - https://github.com/hkdobrev/run-if-changed.

If the capabilities of a Git merge driver are limited by Git, we could also handle that as a Composer plugin hooking to composer install or composer update --lock.

To summarise - potential approaches I can see:

A. Git merge driver for Composer - needs improvements on installation and complex cases. B. Composer plugin running on install or a new command. Install and setup are easy. Discoverability is not. C. Composer itself checking for conflicts on install or a new command. Included with Composer.

Which one do you see most viable and acceptable to Composer core? /cc @Seldaek @balbuf

hkdobrev avatar Jun 22 '20 19:06 hkdobrev

But could this not be cached in some place that is not tracked with git? Either on system level or on project level. (the answer to this depends on the questions below)

being tracked by git is precisely what allows reproducible installs in all environments. Other environments must have the metadata too.

Composer plugin running on install or a new command. Install and setup are easy. Discoverability is not.

This one might be hard to implement, because composer loads the composer.json file quite early (as it contains some config stuff), and would fail there in case of an invalid file.

A. Git merge driver for Composer - needs improvements on installation and complex cases.

For that, I suggest opening issues on the merge driver repo for the different cases causing issues. Regarding new and removed libraries for instance, we could easily imagine that the driver would not perform a dumb deep merging of objects, but would treat the name of packages in a special way, treating the 2 objects as different.

stof avatar Jun 22 '20 19:06 stof

This one might be hard to implement, because composer loads the composer.json file quite early (as it contains some config stuff), and would fail there in case of an invalid file.

Conflicts in composer.lock would happen quite more often compared to composer.json. Wouldn't the plugin be able to run if composer.json is valid, but composer.lock is not?

hkdobrev avatar Jun 22 '20 19:06 hkdobrev

Right. Conflicts on the lock file should be something that a plugin can handle.

stof avatar Jun 22 '20 19:06 stof

@hkdobrev, echoing @stof, go ahead and open an issue on the repo if you can (and even better if you can open a PR, because I'm not a full-time dev anymore and don't have a lot of bandwidth to work on these projects).

But re:

new and removed libraries between the two versions of composer.lock touching the same lines

I'm not sure why there would be any issue here, because this was the primary use-case I had in mind when writing the merge driver. The merge driver parses the JSON files and merges the contents as assoc arrays, rather than (and having no concept of) lines from the files. As far as comparing lockfile changes, packages are keyed by package name and compared rudimentarily as JSON encoded strings before being converted back.

The only issue I'm aware of is an edge case where you can be left with invalid JSON file because of an errant comma after resolving a conflict at the end of the package list, as described here: https://github.com/balbuf/composer-git-merge-driver/issues/14

balbuf avatar Jun 22 '20 19:06 balbuf

And just to clarify, just because you are left with merge conflicts after the driver runs doesn't mean the driver is not properly handling complex cases: when there are merge conflicts that means (or should mean) that a given package was changed in multiple development histories involved in the merge. The driver doesn't know which of the two legitimate, conflicting changes of a given dependency you'd like to keep, thus you must manually resolve them. However, the advantage of this merge driver over the line-based comparison of the default merge driver is that the conflicts are much easier to resolve because the markers are placed around the whole chunk of JSON corresponding to the conflicting versions of a given package.

The readme touches on what I've described above:

A merge conflict is only triggered when the version constraint, locked version number, or presence / absence of the same dependency has been modified in multiple development histories involved in the merge.

For instance, if a certain dependency is updated in one branch and removed in another, a merge conflict is triggered because it is unclear which change for that dependency is desired following the merge. However, if a new, different dependency is appended to the require section in both branches, the merge driver will understand that both should be kept, whereas the standard git merge driver would trigger a merge conflict because the same line has been edited in both branches.

(Forgive me if this is not the situation you are alluding to--but perhaps someone else reading this will find it useful to understand what the merge driver does and doesn't do. There, of course, remains a distinct possibility that there is a bug in which certain cases are not handled properly.)

balbuf avatar Jun 22 '20 20:06 balbuf

What I've been recently doing to overcome composer.lock conflicts in feature branches is:

  1. I create a feature branch and update some dependencies in it. Both composer.json and composer.lock files are modified. I note the CLI composer command I used for this update.
  2. Someone pushes a deps update to the main branch.
  3. Rebasing the feature branch on top of main branch would create a composer.lock conflict.
  4. What I do first is that I change the feature branch and remove the composer.lock change. So the commit that updates dependencies in feature branch only contains composer.json changes.
  5. I can now rebase the feature branch on top of the main branch without composer.lock conflict.
  6. This now allows me to replay my noted composer command from 1) on top of the latest composer.lock file from the main branch.

If Composer would let me do this in an easier way without doing all the git-fu, that would be great!

ondrejmirtes avatar Oct 04 '21 08:10 ondrejmirtes