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

Adjacent lines conflicts #719

Closed
JanVoracek opened this Issue Feb 15, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@JanVoracek
Copy link
Member

JanVoracek commented Feb 15, 2016

During the work on #588 we noticed that Git ends up with merge conflict when it tries to merge files with adjacent lines changed.

Example:
Original file (master, feature-branch):

line 1
line 2

Changed file (master):

line 1 changed on master
line 2

Changed file (feature-branch):

line 1
line 2 changed on feature-branch

Now git merge feature-branch fails on merge conflict. It's a correct Git behavior (see e.g. here), however it's not so useful for INI where different line = different field (except multi-line string).

A solution is (probably) to write our merge driver for INI files. However, some research is needed.

@JanVoracek JanVoracek added this to the 3.0 milestone Feb 15, 2016

octopuss added a commit that referenced this issue Feb 19, 2016

@octopuss octopuss referenced this issue Feb 19, 2016

Merged

719 custom ini merge driver #731

3 of 3 tasks complete

@octopuss octopuss added in review and removed in progress labels Feb 19, 2016

octopuss added a commit that referenced this issue Feb 28, 2016

octopuss added a commit that referenced this issue Feb 28, 2016

octopuss added a commit that referenced this issue Feb 29, 2016

[#719] - removed wrong assert message, renamed prepareTestData method…
… attribute to be more explicit what it does.
@octopuss

This comment has been minimized.

Copy link
Contributor

octopuss commented Feb 29, 2016

I've performed load tests on this version of driver (previous results, see #588 (comment)).

Linux / Mac OS X

Linux / Mac OS X PHP driver Bash driver
1000 files, full driver impl 36.6 s 39.7 s
100 files, full driver impl 3.6 s 3.7 s

Windows

(Performed by @borekb.)

Windows PHP driver Bash driver
1000 files, full driver impl 110 s
100 files, full driver impl 12.6 s

It's a different machine though than the previous test, although similarly spec'd (~2.3GHz Core i5 CPU, relatively recent SSD). The values are very similar to previous load test on Windows which is slightly suspicious because MacOS tests performed by @octopuss are about 40% slower.

octopuss added a commit that referenced this issue Feb 29, 2016

borekb added a commit that referenced this issue Mar 2, 2016

[#719] Refactored MergeDriverTest. Details:
- MergeDriverTests now use more narrative style – the sequence of steps is directly visible in the test methods
- Two new asserts for asserting clean and conflicting merges
- Some methods in MergeDriverTestUtils renamed / refactored

borekb added a commit that referenced this issue Mar 2, 2016

[#719] MergeDriverInstaller: the driver impl can now be forced and gl…
…obal constants were removed (refactored). This also influenced tests. Details:

- MergeDriverInstaller::installMergeDriver() method now has a $driver parameter through which the code can force the implementation.
- Tests use this $driver parameter so we could get rid of the brittle `switchDriverToBash()` and `switchDriverToPhp() methods in MergeDriverTestUtils.
- MergeDriverInstaller no longer uses global constants like VP_PROJECT_ROOT or VERSIONPRESS_MIRRORING_DIR. The code is more explicit and self-contained now.
- Test were slightly refactored again to remove some unnecessary code.

borekb added a commit that referenced this issue Mar 3, 2016

borekb added a commit that referenced this issue Mar 3, 2016

borekb added a commit that referenced this issue Mar 4, 2016

@octopuss octopuss closed this in #731 Mar 8, 2016

octopuss added a commit that referenced this issue Mar 9, 2016

[#719] - Updated missing VP_PROJECT_ROOT when uninstalling merge driv…
…er. Changed Constants from nonexistent VERSIONPRESS_MIRRORRING_DIR to new VP_VPDB_DIR

@borekb borekb changed the title Custom merge driver for INI files Adjacent lines conflicts Mar 9, 2016

@borekb borekb added the significant label Mar 9, 2016

@borekb

This comment has been minimized.

Copy link
Member

borekb commented Mar 9, 2016

How do we handle adjacent line conflicts in multiline strings? Am I right in thinking the following?

  1. post_content uses blank lines to separate paragraph so there will rarely be adjacent lines in the first place.
  2. Even if a multiline string contained adjacent lines, it's correct to handle them similarly to how we now handle INI keys, i.e., avoid the conflicts using our merge driver.

Just double-checking that we don't have a problem here..

@JanVoracek

This comment has been minimized.

Copy link
Member

JanVoracek commented Mar 9, 2016

I think it will be OK.

This INI:

[VPID]
post_title = "Some post"
post_content = "With a multiline 
content"

will be converted to (during the merge)

[VPID]
###VP###
post_title = "Some post"
###VP###
post_content = "With a multiline 
###VP###
content"
###VP###

That shouln't have any negative effect to the merge.

@borekb

This comment has been minimized.

Copy link
Member

borekb commented Mar 9, 2016

My concern was rather opposite, whether we're not auto-resolving conflicts that should be real conflicts. But I think we're not, or at least I couldn't think of a situation where our merge driver would cause any trouble.

@borekb borekb removed the in review label Apr 28, 2016

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