Skip to content
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

Implementation of array for repositories.tags-path and repositories.branches-path #5142

Closed
wants to merge 2 commits into from

Conversation

xylesoft
Copy link

@xylesoft xylesoft commented Apr 1, 2016

I've implemented array awareness to the repositories.tags-path and repositories.branches-path configuration clauses, because in SVN you are able to create more complex tags and branches directories, where you may have multiple sub-directories (as my employer does).

This relates to a issue I submitted to the "composer/satis": composer/satis#312

I'll also be submitting a JSON schema update for the satis.json definition.

$tagsPath = 'tags';
if (isset($packageConfig['branches-path'])) {
$branchesPath = (is_array($packageConfig['branches-path'])) ? implode('|', $packageConfig['branches-path']) : $packageConfig['branches-path'];
$branchesPath = preg_quote($branchesPath, '#');
Copy link
Contributor

Choose a reason for hiding this comment

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

using preg_quote here will not allow supporting multiple paths, as it will escape the | used when imploding above. you need to quote each path separately instead.

Copy link
Author

Choose a reason for hiding this comment

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

@stof Very good point, I will change this.

@Seldaek
Copy link
Member

Seldaek commented Apr 1, 2016

TBH, that's a lot of changes to review for adding more obscure features to the (somewhat legacy) SVN driver for an edge case that most likely only benefits you. I can't stay this perspective excites me a whole lot :/ Have you considered maybe switching to a more standard directory structure? This would be time better spent I feel.

@alcohol
Copy link
Member

alcohol commented Apr 19, 2016

@Seldaek on the other hand, since it only affects the SVN driver and repos, there isn't much harm in accepting these changes. I can see merit in these changes.

As a team (@composer), we can simply state that SVN support is provided by the community/contributors, and as such we do not directly support it ourselves. Any issues arising from this would either result in a new patch from contributors, or we simply revert the merge if no solution is provided in a timely fashion.

@Seldaek
Copy link
Member

Seldaek commented May 31, 2016

Yeah, I don't really mind as it's svn-only, but there is some feedback above that still wasn't addressed, which is the main reason this wasn't merged yet.

@Seldaek
Copy link
Member

Seldaek commented May 31, 2016

I also wonder if it's really that important if people manage to get by without such a patch for a few months..

@Seldaek Seldaek closed this Jul 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants