Skip to content
This repository was archived by the owner on Jul 28, 2024. It is now read-only.

719 custom ini merge driver#731

Merged
octopuss merged 25 commits intomasterfrom
719-custom-ini-merge-driver
Mar 8, 2016
Merged

719 custom ini merge driver#731
octopuss merged 25 commits intomasterfrom
719-custom-ini-merge-driver

Conversation

@octopuss
Copy link
Copy Markdown
Contributor

Resolves #719. This PR is successor of PR #725 which should close #588. Important commits are only those which are related to issue #719. Other are merged PR/Branch #725.

Please review:

Thank you.

Restrictions:

@octopuss octopuss added this to the 3.0 milestone Feb 19, 2016
@octopuss octopuss self-assigned this Feb 23, 2016
@JanVoracek
Copy link
Copy Markdown
Contributor

Please merge master into this and resolve conflicts.

@octopuss octopuss force-pushed the 719-custom-ini-merge-driver branch from 3f7d076 to ff8eee7 Compare February 25, 2016 13:38
@octopuss octopuss force-pushed the 719-custom-ini-merge-driver branch from ff8eee7 to 02dd1be Compare February 25, 2016 13:58
@octopuss
Copy link
Copy Markdown
Contributor Author

Should I perform LoadTest statistics also for this version of driver?

@borekb
Copy link
Copy Markdown
Member

borekb commented Feb 26, 2016

@octopuss I think we should (you on Mac, I'll do it on Windows).


done

# Place placeholder between lines
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • "... to avoid merge conflicts on adjacent lines" (to make the comment a bit more descriptive)

@borekb
Copy link
Copy Markdown
Member

borekb commented Feb 26, 2016

(The commit history is messed up here but let's leave it like that, it's not a big deal.)

@borekb
Copy link
Copy Markdown
Member

borekb commented Feb 26, 2016

I'm looking at MergeDriverTest and MergeDriverLoadTest and think that some things there could be improved. I don't have time to fully dig into it so I'll leave just some line comments there and, @octopuss, please go through it and update wherever you think the comments are valid.

$branchDate = '17-02-16 19:19:23';

MergeDriverTestUtils::fillFakeFileAndCommit($originDate, 'file.ini', 'Initial commit to Ancestor');
MergeDriverTestUtils::fillFakeFileAndCommit('10-02-16 08:00:00', 'file.ini', 'Initial commit to Ancestor');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's fake about the file? Seems like a normal test file to me. Is it faking anything?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it is faking ini files created by VersionPress. There is no guid at the top of created file. But you are right that the file created is legit for the purpouse of this test.

  • change method name

@octopuss octopuss force-pushed the 719-custom-ini-merge-driver branch from b15c161 to 7a82a21 Compare February 29, 2016 07:18
octopuss and others added 6 commits February 29, 2016 08:24
- 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
Copy link
Copy Markdown
Member

borekb commented Mar 2, 2016

I'll do some more updates / refactorings:

  • Uninstall merge driver test fails on Windows.
  • Refactor MergeDriverTest to be easier to read
  • Refactor MergeDriverInstaller - get rid of global constants, document it
  • Make the ####### placeholder more random - this could happen in real post, e.g., Markdown headings

…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
Copy link
Copy Markdown
Member

borekb commented Mar 3, 2016

Guys, could you please review my recent commits when you have time? In the meantime I'll fix the remaining two issues (failing test on Windows and more unique placeholder).

@JanVoracek @octopuss

@borekb borekb force-pushed the 719-custom-ini-merge-driver branch from 2503090 to 2a81460 Compare March 3, 2016 13:30
} else {
$mergeDriverFile = 'ini-merge.php';
$mergeDriverScript = '"' . PHP_BINARY . '" "' . VERSIONPRESS_PLUGIN_DIR . '/src/Git/merge-drivers/' . $mergeDriverFile . '"';
if ($driver == 'php' || ($driver == 'auto' && DIRECTORY_SEPARATOR == '\\')) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you have decided not to use constants for merge driver types. I know that it is just for tests. Isn't it better to replace those strings with constants, so potential users will know exact values which can be used ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I treat test code the same as production code - it needs to have the same quality - so it's not an excuse here. I genuinely don't know what's better here, these are just three strings so I chose to use them directly without any "abstraction" but maybe that's just my taste. What would @JanVoracek do?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably use constants because of preventing typos.

Another note to the code: I would make the conditions simpler as the final driver can be decided much sooner.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll add constants. BTW string literal types are an amazing feature of TypeScript, exactly what I'd use here if PHP supported it.

About the conditions: I had a feeling they could be simplified but it didn't occur to me how. Can you draft a pseudo-code (or real code) here? I'll happily apply it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like this?

if ($driver === self::DRIVER_AUTO) {
    $driver = DIRECTORY_SEPARATOR == '\\' ? self::DRIVER_PHP : self::DRIVER_BASH;
}

if ($driver === self::DRIVER_BASH) {
    $mergeDriverScript = $pluginDir . '/src/Git/merge-drivers/ini-merge.sh';
    chmod($mergeDriverScript, 0750);
}

if ($driver === self::DRIVER_PHP) {
    $mergeDriverScript = '"' . PHP_BINARY . '" "' . $pluginDir . '/src/Git/merge-drivers/ini-merge.php' . '"';
}

@borekb
Copy link
Copy Markdown
Member

borekb commented Mar 7, 2016

@octopuss Please merge from master, resolve conflicts, do some final testing and merge.

@octopuss octopuss force-pushed the 719-custom-ini-merge-driver branch from bb10448 to f6ee3eb Compare March 8, 2016 17:52
octopuss added a commit that referenced this pull request Mar 8, 2016
@octopuss octopuss merged commit cab55d4 into master Mar 8, 2016
@octopuss octopuss deleted the 719-custom-ini-merge-driver branch March 8, 2016 17:57
@borekb
Copy link
Copy Markdown
Member

borekb commented Mar 8, 2016

🎉 this was a hell of a PR! Good job.

@octopuss octopuss restored the 719-custom-ini-merge-driver branch March 9, 2016 07:25
@octopuss octopuss deleted the 719-custom-ini-merge-driver branch March 9, 2016 08:07
@octopuss octopuss restored the 719-custom-ini-merge-driver branch March 9, 2016 09:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adjacent lines conflicts Date modified not handled properly

3 participants