Skip to content

Preliminary work on refactoring svn into private functions #12

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 16, 2018
Merged

Preliminary work on refactoring svn into private functions #12

merged 4 commits into from
Oct 16, 2018

Conversation

eli-schwartz
Copy link
Contributor

Hopefully one step closer to the dbscripts-rewrite.

/cc @gbsf

@eli-schwartz eli-schwartz mentioned this pull request Jul 4, 2018
Closed
@eli-schwartz
Copy link
Contributor Author

eli-schwartz commented Jul 4, 2018 •
edited
Loading

Tests passed yesterday :/

Ah, this breaks because #9

gbsf
gbsf reviewed Jul 4, 2018
View reviewed changes
db-remove Outdated

if [[ -d ${WORKDIR}/svn/$pkgbase/repos/$svnrepo ]]; then
remove_pkgs+=($(. "${WORKDIR}/svn/$pkgbase/repos/$svnrepo/PKGBUILD"; echo ${pkgname[@]}))
if remove_pkgs+=($(. source_pkgbuild "${pkgbase}" "repos/${svnrepo}" && echo ${pkgname[@]})); then
Copy link

Choose a reason for hiding this comment

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

You have an extra . here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch. Does this mean we didn't have any testing of this whole script until yesterday :p

@eli-schwartz eli-schwartz requested a review from pierres July 6, 2018 23:09
@eli-schwartz
Copy link
Contributor Author

eli-schwartz commented Jul 6, 2018 •
edited
Loading

@pierres,

still needs more work, but what do you think of the general idea and as compared to #10?

@eli-schwartz
Copy link
Contributor Author

Pushed another commit refactoring lots of the remaining logic, the only thing left is the actual db-move bits for editing the VCS.

@eli-schwartz eli-schwartz force-pushed the svn-refactor branch 2 times, most recently from a66b632 to 6626b81 Compare October 7, 2018 16:58
@eli-schwartz
Copy link
Contributor Author

No longer preliminary.

@gbsf @pierres @Bluewind this is done now and everything svn-aware is in db-functions-svn.

@LukeShu you submitted a similar PR, but I think I prefer the approach I've taken here. What are your thoughts?

@LukeShu
Copy link
Contributor

LukeShu commented Oct 7, 2018

This has some of the same problems that older versions of #10 had. The first 5 commits of #10 are currently improvements to the test suite, checking for those issues. I'm seeing 7 test failures when I cherry pick them on top of this. Should I submit those as a separate PR, so that you can work on top of them?

@eli-schwartz
Copy link
Contributor Author

The only one which errors, is test: Don't use "! cmd" except as the last statement in a function.

See https://github.com/eli-schwartz/dbscripts/commits/lukeshu-pr10-plus-refactor

@LukeShu
Copy link
Contributor

LukeShu commented Oct 7, 2018 •
edited
Loading

So that means that there were several tests whose conditions were failing, but the tests erroneously passed because they were mistakenly written in a way that didn't trigger the err trap when their conditions failed.

Put another way, several of your changes broke test-cases that use checkRemovedPackage(), but checkRemovedPackage() was broken in a way that it didn't catch the breakage and let the test pass.

@eli-schwartz
Copy link
Contributor Author

My last commit was flawed, but should be fixed now.

@LukeShu
Copy link
Contributor

LukeShu commented Oct 7, 2018

Obviously, I prefer my approach. But this looks... fine. I have no real objections.

@eli-schwartz
Preliminary work to break out svn-specific code.
d6b48bd 
Introduce "db-functions-$VCS" which will eventually contain all
VCS-specific code, and make this configurable in config.

Move private arch_svn function and svn acl handling here, and introduce
a new source_pkgbuild function to handle discovering PKGBUILDs from the
configured VCS and sourcing them to extract metadata.

The PKGBUILD is the only file we ever check out from version control,
and only ever to scrape information from it, except for when we actually
want to db-move a whole directory (which is by necessity considerably
dependent on the VCS in use).

source_pkgbuild is inspired by commits from the dbscripts rewrite,
authored by Florian Pritz <bluewind@xinu.at>
@eli-schwartz
Factor out the exporting of files/folders from svn.
37a493d 
As of the source_pkgbuild rewrite, this is only ever done once.
@eli-schwartz
More work on refactoring svn moving into VCS-agnostic routines.
13e5708
@eli-schwartz
Finish the svn refactor by implementing helper functions to write to …
61c9cc6 
…the VCS
@eli-schwartz eli-schwartz merged commit 61c9cc6 into master Oct 16, 2018
@eli-schwartz eli-schwartz deleted the svn-refactor branch October 16, 2018 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierres pierres Awaiting requested review from pierres

1 more reviewer

@gbsf gbsf gbsf left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eli-schwartz @LukeShu @gbsf