588 date modified merge driver by octopuss · Pull Request #725 · versionpress/versionpress

octopuss · 2016-02-16T21:39:53Z

This PR closes #588 by adding custom merge driver which selects more recent significant dates from both merged files and saves them. Then traditional git merge feature-branch is made.

Please review :

Thank you

…ate (+gmt version) from merged files

borekb · 2016-02-16T22:19:44Z

plugins/versionpress/src/Cli/vp-internal.php

+        $initializationDir = VERSIONPRESS_PLUGIN_DIR . '/src/Initialization';
+        MergeDriverConfigurer::installGitattributes($initializationDir);
+        MergeDriverConfigurer::installGitMergeDriver($initializationDir);
+        WP_CLI::success("Git merge driver for VersionPress files added.");


Maybe just "Git merge driver added".

borekb · 2016-02-16T22:39:31Z

Overall great work, my line comments are smaller things, maybe just two other things to discuss are:

I think we should have tests for this. We have CloneTest as the only workflow test now but maybe this could be simpler, working inside a single repo (not across clones), creating local test branch and merging it back.
Wasn't there a .sh variant of the driver?

JanVoracek · 2016-02-16T22:42:34Z

plugins/versionpress/src/Git/MergeDriverConfigurer.php

+        $gitconfigVariables = array(
+            'wp-plugins' => rtrim(ltrim(PathUtils::getRelativePath(VP_PROJECT_ROOT, WP_PLUGIN_DIR), '.'), '/\\'),
+            'merge-driver-script' => 'ini-merge.php',
+            'bin-dir' => PHP_BINDIR . '/php'


This isn't a directory but an executable. It's confusing in gitconfig.tpl.

JanVoracek · 2016-02-16T23:39:16Z

plugins/versionpress/ini-merge.php

+    $matches = array();
+    preg_match($dateMatchPattern, $aFile, $matches);
+    if (count($matches) == 0) {
+        echo "not found";


Perhaps some leftover of debugging?

Remove echo

…ore.

…t (Windows -> PHP /Linux -> Bash)

JanVoracek · 2016-02-22T09:19:55Z

Waiting for completing measurements in #588 (comment).

borekb · 2016-02-22T10:10:59Z

A couple of questions / suggestions about this:

Is this for 100 or 1000 files? Maybe this row should be merged into the two tables below, e.g., repeat the test for both 100 and 1000 files?
Just to make sure, was this run on Mac OS or was it a Linux VM? (Assuming Mac OS, I'm OK with calling that "Linux" :) )
Same note as 1.

borekb · 2016-02-22T12:23:14Z

plugins/versionpress/tests/LoadTests/MergeDriverLoadTest.php

+        $mergeEC = MergeDriverTestUtils::runProcess(MERGE_CMD);
+        $time_end = microtime(true);
+        $execution_time = ($time_end - $time_start);
+        echo 'Php Execution Time: '.$execution_time.' Sec';


octopuss · 2016-02-22T13:41:01Z

The problem is missing replacement of slashes here https://github.com/versionpress/versionpress/blob/588-date-modified-merge-driver/plugins/versionpress/tests/Utils/MergeDriverTestUtils.php#L97

I think that if you install VersionPress from scratch the driver script path will contain correct slashes.

borekb · 2016-02-22T13:43:18Z

Now that you mention it, it seems to me that there should also be a MergeDriverInstaller::uninstallMergeDriver() method and the ..TestUtils class should use it rather than duplicating logic.

octopuss · 2016-02-22T13:45:02Z

You are right. It seems correct to remove driver when VersionPress will be uninstalled.

…ich is called when VersionPress is deactivated.

JanVoracek · 2016-02-23T10:05:56Z

plugins/versionpress/tests/GitRepositoryTests/MergeDriverTest.php

+
+        $this->assertEquals(1, MergeDriverTestUtils::runProcess(self::$mergeCmd));
+
+        copy(self::$repositoryDir . '/file.ini', '/Users/Ivan/expected-merge-conflict.ini');


Wrong absolute path (with username).

…of asserted string)

…a CLI. Added workflow tests

- The test was not actually testing anything as the posts in both test and clone sites have likely been modified in the same second. `sleep()` was added. - Test renamed from `updatedArticleCanBeMergedFromClone()` to `dateModifiedMergesAutomatically()` to better reflect what id does. - Some other things slightly refactored.

…namespace

…not read correctly from the cloned site.

borekb · 2016-02-24T14:32:42Z

plugins/versionpress/tests/GitRepositoryTests/MergeDriverTest.php

+
+    private static $masterDate;
+
+    private static $branchDate;


Many of these variables are used in one place only, they are unnecessary noise in this class and should be inlined.

borekb · 2016-02-25T07:57:43Z

Good job, seems ready to merge to me. 👍

588 date modified merge driver

octopuss added 2 commits February 16, 2016 20:32

#588 - added custom merge driver which takes "bigger" post modified d…

e8e2a04

…ate (+gmt version) from merged files

#588 formatting changes :)

741da0b

borekb reviewed Feb 16, 2016
View reviewed changes

JanVoracek reviewed Feb 16, 2016
View reviewed changes

JanVoracek added this to the 3.0 milestone Feb 16, 2016

JanVoracek reviewed Feb 16, 2016
View reviewed changes

octopuss added 2 commits February 17, 2016 09:58

[#588] Implemented first part of PR reviews

678cdbf

[#588] post_modified and post_modified_gmt are not excluded any m…

3cd6d80

…ore.

octopuss force-pushed the 588-date-modified-merge-driver branch from 0d8f7fc to 3cd6d80 Compare February 17, 2016 08:59

octopuss added 2 commits February 17, 2016 10:02

#588 removed uncecessary vpdb , fixed newline adding.

0c81bb4

#588 git merge-file exit code not passed to calling script.

12f9c53

octopuss force-pushed the 588-date-modified-merge-driver branch from 22fb6c6 to 12f9c53 Compare February 17, 2016 12:33

octopuss added 6 commits February 17, 2016 14:32

#588 added tests for merge driver, conflict labels into git merge-file.

22b5531

#588 fixed PHP_BINARY path, and tests refactor.

5b2b1b2

#588 Merge driver load test, testing both implementations

ecaf148

#588 bash merge driver adjustment to handle \r\n line endings

5fc41f4

#588 Mergge driver implementation is selected according to environmen…

8d29c50

…t (Windows -> PHP /Linux -> Bash)

#588 fix failing chmod on php implementation which is not necessary

aeb5289

octopuss mentioned this pull request Feb 19, 2016

719 custom ini merge driver #731

Merged

3 tasks

borekb reviewed Feb 22, 2016
View reviewed changes

#588 - refactor of test classes, added uninstallMergeDriver method wh…

6e4fcd3

…ich is called when VersionPress is deactivated.

JanVoracek reviewed Feb 23, 2016
View reviewed changes

octopuss self-assigned this Feb 23, 2016

octopuss added the in review label Feb 23, 2016

octopuss and others added 8 commits February 23, 2016 11:42

#588 - unncecessary copying removal, test assert adjustment (crlfize …

ce6fe09

…of asserted string)

Merge branch 'master' into 588-date-modified-merge-driver

df7c7e6

#588 Added commands to vp-internal to get vp_id or id of an entity vi…

177ea42

…a CLI. Added workflow tests

#588 - removed unncecessary direct sql changing. Formatting

724e841

[#588] Renaming src/Git/MergeDrivers to merge-drivers as it is not a …

6b85ae1

…namespace

[#588] Fixed CloneTest::dateModifiedMergesAutomatically() - date was …

a719dda

…not read correctly from the cloned site.

[#588] Minor: removed unnecessary --dbprefix parameter

9da23fe

borekb reviewed Feb 24, 2016
View reviewed changes

borekb and others added 2 commits February 24, 2016 16:49

[#588] Changed uninstall merge driver regex.

9bb2133

#588 - test classes refactor

cf015b1

octopuss force-pushed the 588-date-modified-merge-driver branch from 8f1be93 to cf015b1 Compare February 24, 2016 16:12

octopuss added a commit that referenced this pull request Feb 25, 2016

Merge pull request #725 from versionpress/588-date-modified-merge-driver

77de2a8

588 date modified merge driver

octopuss merged commit 77de2a8 into master Feb 25, 2016

JanVoracek removed the in review label Feb 25, 2016

octopuss deleted the 588-date-modified-merge-driver branch February 25, 2016 13:20

JanVoracek mentioned this pull request Mar 4, 2016

Date modified not handled properly #588

Closed

petrescugit mentioned this pull request Feb 29, 2024

[Snyk] Security upgrade archiver from 3.0.0 to 7.0.0 petrescugit/versionpress#10

Open


		$this->assertEquals(1, MergeDriverTestUtils::runProcess(self::$mergeCmd));

		copy(self::$repositoryDir . '/file.ini', '/Users/Ivan/expected-merge-conflict.ini');

Conversation

octopuss commented Feb 16, 2016

Uh oh!

borekb Feb 16, 2016

Choose a reason for hiding this comment

Uh oh!

borekb commented Feb 16, 2016

Uh oh!

JanVoracek Feb 16, 2016

Choose a reason for hiding this comment

Uh oh!

JanVoracek Feb 16, 2016

Choose a reason for hiding this comment

Uh oh!

JanVoracek commented Feb 22, 2016

Uh oh!

borekb commented Feb 22, 2016

Uh oh!

borekb Feb 22, 2016

Choose a reason for hiding this comment

Uh oh!

octopuss commented Feb 22, 2016

Uh oh!

borekb commented Feb 22, 2016

Uh oh!

octopuss commented Feb 22, 2016

Uh oh!

JanVoracek Feb 23, 2016

Choose a reason for hiding this comment

Uh oh!

borekb Feb 24, 2016

Choose a reason for hiding this comment

Uh oh!

borekb commented Feb 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants