ef-labs / stash-hook-mirror

An Atlassian Stash repository hook for mirroring to one or more remote git repositories.
MIT License
77 stars 58 forks source link

Mirror only specific branches #53

Closed christiangalsterer closed 6 years ago

christiangalsterer commented 6 years ago

Hi,

we have some use cases where we want to mirror only a few branches, e.g. based on a branch pattern and not overwrite remote branches. I think your add-on would be a good baseline for this but would requires some additional features.

Would you in general interest in this feature (could potentially also help in #48) and if so would you be open for a contribution via a PR?

Thanks in advance Christian

christiangalsterer commented 6 years ago

I had some time and have already an implementation available. I will submit a PR tomorrow.

There is one open question. When the user specifies a branches pattern and saves the configuration the correct implementation still mirrors everything and not only the branches which matches the pattern.

Shall we keep this or shall we also change it in a way that it only mirrors the branches with matches the branch pattern? The second option would require an additional git command call to get all available branches (this would be my preference).

christiangalsterer commented 6 years ago

Just submitted PR #54 for your review. Please let me know of any comments, changes, etc.

adrianluisgonzalez commented 6 years ago

Thanks for the contribution, this is definitely something I have thought about and see value to adding. The code looks good, just missing some unit test coverage for your changes.

christiangalsterer commented 6 years ago

Thanks for the fast reply. Will add some unit tests tomorrow.

christiangalsterer commented 6 years ago

Unit tests are added, please let me know in case of questions.

adrianluisgonzalez commented 6 years ago

Sorry, will try to look at this and merge tomorrow.

christiangalsterer commented 6 years ago

np

adrianluisgonzalez commented 6 years ago

@christiangalsterer I was just running the mirror plugin after merge and I noticed everything is still mirrored when first set up:

        // if an empty list of RefChanges was provided we assume this was caused by triggering after a config change.
        // same if no branch pattern was specified we sync all branches
        if(refChanges.isEmpty() || settings.branchesIncludePattern.isEmpty()) {
            builder.argument("+refs/heads/*:refs/heads/*");
        } else {
            .....

Rather than using regular expressions, would it be easier to just specify the refspecs to mirror? That would ensure only the appropriate branches are ever mirrored. For example, if I only want to mirror master and feature/* branches I could have the following:

+refs/heads/master:refs/heads/master
+refs/heads/feature/*:refs/heads/feature/*

Thoughts?

See this feature branch commit (https://github.com/ef-labs/stash-hook-mirror/tree/feature/mirror-ref-spec)

christiangalsterer commented 6 years ago

Hi,

thanks for merging the PR. W.r.t your first question, this is also where I was not sure about how to deal with it, see my second comment in this issue. W.r.t. regex vs. refspec, I was also thinking about this and based on my experience, I see regex in add-ons more often than refspecs, therefore I went for regex. But refspec is also fine and feature branch looks ok for me. Maybe the only change to consider to rename the "SETTING_REFSPEC" variable to something like "SETTING_BRANCHES_INCLUDE_REFSPEC" to allow a potential extension in the future for excluding some refspecs.

adrianluisgonzalez commented 6 years ago

Thanks for being understanding.

I see a few benefits to supporting refspec such as it allows you to do mapping to different namespaces on the remote mirror. Additionally you can control mirroring tags, pull requests, notes, etc.

christiangalsterer commented 6 years ago

sure makes sense. Do you already have an idea when you can provide a next release including the changes?

christiangalsterer commented 6 years ago

Please ignore my last comment, just saw that you already the releases.