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

Date modified not handled properly #588

Closed
borekb opened this Issue Dec 8, 2015 · 13 comments

Comments

Projects
None yet
3 participants
@borekb
Copy link
Member

borekb commented Dec 8, 2015

In the early alphas and betas of VersionPress, we encountered an issue with "date modified" -- specifically the DB columns post_modified and post_modified_gmt -- which was causing issues in revert scenarios (and would later cause issues during sync). We then decided to compute the date from the Git history, see #73.

However, that was a bad idea. We missed one important scenario: posts that were created before VersionPress started tracking the site. We currently do not have any information about them in the Git repository so restoring them by wp vp restore-site command or in sync scenarios gives them wrong post modification dates.

@borekb

This comment has been minimized.

Copy link
Member

borekb commented Dec 8, 2015

Finding solution to this is not easy because by default, storing post_modified in our INI files will cause conflicts. For example, when I edit a text of the post in env A and the title in env B, the merge will throw a conflict on the line with post_modified.

We basically have two ways:

  1. Store the date modified info externally to Git
  2. Somehow force Git to resolve that partial conflict automatically.

2 would be much preferable because we don't want to depend on custom mechanisms and it seems that there is a way, by the means of merge drivers. I am not quite familiar with them so this needs to be explored further but here is some basic info.

@JanVoracek

This comment has been minimized.

Copy link
Member

JanVoracek commented Dec 9, 2015

This could be interesting https://github.com/BYVoid/Batsh.

@borekb

This comment has been minimized.

Copy link
Member

borekb commented Feb 15, 2016

We're eventually going down the route of writing a merge driver.

There's been a brief discussion on in which technology to write the script as we need to support both UNIX-like and Windows environments:

  1. Shell script + batch file, i.e, duplicate the logic but keep it native.
  2. Transpile from Batsh to both Bash and BAT.
  3. Script it in PHP (or, less likely, in some other technology)

2 isn't stable enough so we're ruling it out. The prototype will be written in Bash so we'll probably stick with option 1 for now but if this turns out to be a maintenance nightmare, we could think about 3. PHP implementation will likely be slower but at the same time, this merge driver will not run too often so it's not a big issue.

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

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

@octopuss octopuss referenced this issue Feb 16, 2016

Merged

588 date modified merge driver #725

2 of 2 tasks complete

@borekb borekb referenced this issue Feb 17, 2016

Closed

Bump required PHP version to 5.6+ #726

3 of 3 tasks complete
@octopuss

This comment has been minimized.

Copy link
Contributor

octopuss commented Feb 18, 2016

Batch implementation (*.bat) on Windows seems to be not doable without spending plenty of time. I've implemented merge driver in bash and also in php. Both implementations have some pros and cons:

PHP implementation:

  • PHP implementation can be run on both Windows as well as Linux using same implementation.
  • dependant on path to php binary path, which is correctly stated only in PHP_BINARY available from php 5.4+ (see #726 )

Bash implementation

  • No need for external binary
  • Only on Linux

Load tests

Before sticking to one or another, I've made some load tests. The load test itself consists of this scenario:

  1. prepare X files, commit them to master
  2. switch to branch, update files and commit them
  3. switch back to master, update files and commit them
  4. merge branch into master

In the tables below, there are a couple of drivers tested:

  • No driver: just pure Git.
  • "Full implementation" driver: what we have in the repo, fully working driver resolving date conflicts
  • "Dummy implementation" driver: the driver still exists but just delegates to built-in Git driver without any custom logic. For example, this is how the dummy impl looks like in PHP:
<?php

$O = $argv[1];
$A = $argv[2];
$B = $argv[3];

$mergeCommand = 'git merge-file -L mine -L base -L theirs ' . $A . ' ' . $O . ' ' . $B;

exec($mergeCommand, $dummy, $mergeExitCode);
exit ($mergeExitCode);

Linux / Mac OS X

Linux / Mac OS X No driver PHP driver Bash driver
1000 files, full driver impl 0.29 s (?) 26.5 s 18.65 s
1000 files, dummy driver impl 0.29 s (?) 25.3 s 7.1 s
100 files, full driver impl 💥 3.2 s 2.4 s
100 files, dummy driver impl 💥 💥 💥

Windows

Windows No driver PHP driver Bash driver
1000 files, full driver impl 9.5 s 109.4 s
1000 files, dummy driver impl 9.5 s 108.9 s
100 files, full driver impl 2.6 s 12.1 s
100 files, dummy driver impl 2.6 s 11.5 s

From the results I assume that merging using custom merge driver has linear time complexity. php implementation seems to be remarkably slower only on Windows environment, so I think we can use it in VersionPress.

EDIT: The complete merge driver load tests, incl. adjacent lines merges, are here: #719 (comment).

@octopuss octopuss added the discussion label Feb 18, 2016

@borekb

This comment has been minimized.

Copy link
Member

borekb commented Feb 23, 2016

I've added Windows load test numbers to #588 (comment) (version history here).

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

borekb added a commit that referenced this issue Feb 24, 2016

[#588] Fixing and refactoring test from 177ea42. Details:
- 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.

borekb added a commit that referenced this issue Feb 24, 2016

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

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

@JanVoracek

This comment has been minimized.

Copy link
Member

JanVoracek commented Mar 4, 2016

I found an issue related to PR #725. Some PagesTests and PostsTests fail on "Expected: 'VP-Post-UpdatedProperties: post_content,post_title', Actual: 'VP-Post-UpdatedProperties: post_content,post_title,post_modified,post_modified_gmt'". Also RevertTests fail.

Reopen? (CI! 🛂)

@octopuss

This comment has been minimized.

Copy link
Contributor

octopuss commented Mar 4, 2016

I think that it would be better to reopen this one, because i guess it will just be fix of the tests. When we merge #719 the tests should be same for both issues/PRs.

@JanVoracek JanVoracek reopened this Mar 7, 2016

@JanVoracek JanVoracek removed the in review label Mar 7, 2016

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

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

Merge pull request #770 from versionpress/588-date-modified-not-handl…
…ed-correctly

[#588] - post_modified and post_modified_gmt are updated to "NOW"

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

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