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

Atlassian stash plugin #56

Closed
wants to merge 11 commits into from

Conversation

newren
Copy link
Contributor

@newren newren commented May 28, 2014

This pull request includes 5 commits from pull request #52 (email for only a subset of refs) and 1 from pull request #53 (StashEnvironment), because it builds on the StashEnvironment. The relevant commits for this pull request are the 5 at the end.

These five commits provide the source for an Atlassian Stash Plugin, which adds various GUI configuration knobs and such to the webpage for the local stash instance, as well as hooking up git_multimail to the relevant Stash specific hook. (While Stash can use normal git post-receive hooks, there are two problems with doing so: (1) merges of pull requests are done on the server and hence bypass those, (2) those hooks don't have access to the correct username of the pusher nor the user-visible repository name that is involved.)

This pull-request is probably not quite ready to be merged as there are a few things we're fixing up locally. We could fix those in subsequent pull requests, but the biggest question right now for me is whether the glue for hooking Stash up to git-multmail (i.e. this Atlassian Stash Plugin) even makes sense to include as part of git-multimail, or whether that glue belongs in its own separate project.

newren and others added 11 commits May 27, 2014 11:09
Some deployments may have refs for which they don't want emails sent out
when they are updated.  However, if commits pushed to these refs are later
pushed to another ref, we do want emails for those commits at that time.
This means that these refs should be ignored both in terms of which
changes we should send out emails for, and when computing what commits are
considered new to the repository.

We set the default for this in the base Environment to ["refs/notes/"].
A default of [] would make sense if git_multimail had a specialized
formatting for commits made by git notes, but it currently doesn't.  In
particular, the commit messages of git notes are essentially meaningless
and "filenames" in git notes commits are merely an implementational
detail.  As such, sending emails for these commits, with our current
format, is suboptimal.

Notes are not the only reason for an exclusion, though.  For example,
Gerrit uses refs/changes/ to store commits that have not yet been vetted.
Users think of commits stored under refs/changes/ as being new to the
repository only when they are finally merged into a branch under
refs/heads/.  (There are a variety of reasons for this: (a) these commits
haven't undergone code review and thus are viewed as not yet having been
accepted, (b) Gerrit has a separate mechanism to handle things pushed to
refs/changes/ (emailing just a few relevant folks with a message along
the lines of "You have a Change Review that you need to look at; look at
this nifty web page to add your comments."), (c) only a small subset of
users are interested in any particular refs/changes/ push, whereas
hundreds of developers want to see what enters refs/heads/, (d), gerrit's
standard ref-updated hook, which is similar to git's update or
post-receive hooks, is not invoked for these refs.  It all adds up to
reasons to ignore this particular ref namespace.)  Another example is
Gerrit's refs/cache-automerge/, which is essentially just an
implementational detail that we clearly don't want to spam users about.

Signed-off-by: Elijah Newren <newren@palantir.com>
Many projects use a pull-request model with a central repository, where
new changes are pushed to branches named e.g. feature/whizbang.  However,
users really don't view these branches as "part of the repository" until
they are merged into a 'real' branch, such as master.  And lots of users
are very annoyed at getting email notifications for pushes to these
branches; they only want to be notified when some subset of branches are
updated.

There's a slight complication, though: traditionally git-multimail has
only sent out commit emails for commits that are entirely new to the
repository, but this doesn't play well with this filtering feature.  So,
we need to instead have git-multimail only send out commit emails for
commits that are entirely new to the *relevant subset of the* repository.

Add multimailhook.refFilterInclusionRegex and
multimailhook.refFilterExclusionRegex configuration options, with related
--ref-filter-inclusion-regex and --ref-filter-exclusion-regex command
line options, which can be used to configure which refs are considered
relevant.  Mark ^refs/notes/ as irrelevant by default as git-multimail
emails make no sense for git notes.

Signed-off-by: Elijah Newren <newren@palantir.com>
Signed-off-by: Elijah Newren <newren@palantir.com>
Signed-off-by: Elijah Newren <newren@palantir.com>
Signed-off-by: Elijah Newren <newren@palantir.com>
Atlassian Stash can utilize a standard post-receive hook, and thus we
could use git_multimail as-is.  However, that is slightly suboptimal
because: (a) when pull requests are merged in stash, they are done on the
server and thus the normal post-receive hook is not triggered, (b)
everything runs as a single user similar to gitolite, but without a
GL_USER environment variable to rely on, and (c) although stash has
reasonable clone urls that having meaningful names for repositories, the
actual on-disk directories in which the repositories are stored are given
positive integers for names -- that results in a rather meaningless email
subject for the users ("[2093] branch master updated..." rather than
"[PROJ/REPO] branch master updated..."), since they will have no idea what
magic integer happened to be selected for their repository.

We can avoid these problems by making use of Stash's
AsyncPostReceiveRepositoryHook instead of a standard post-receive hook,
since it is triggered in all the right cases and has access to the
appropriate user and repo name.  Add a StashEnvironment for holding this
passed information, and add command line flags that can be passed with
the user and repo name.

Signed-off-by: Elijah Newren <newren@palantir.com>
This adds a plugin for Atlassian Stash that will run git_multimail as an
AsyncPostReceiveRepositoryHook (similar to a standard post-receive hook,
but also fires for merges of pull-requests; further, via Stash's Java API
such a hook has access to important data like user and repo name).

This plugin currently provides a single textbox entry of configuration at
the time that a repository owner enables the plugin, in which the
repository owner can provide a comma-separated list of email addresses
that they want the emails sent to.  Other multimail configuration options
are unexposed through the web UI, though they can be configured on a
repository-by-repository basis on the server if an admin sets the
appropriate multimailhook.* configuration setting in the git config file.

Signed-off-by: Elijah Newren <newren@palantir.com>
Signed-off-by: Carl Myers <cmyers@palantir.com>
Signed-off-by: Elijah Newren <newren@palantir.com>
Signed-off-by: Carl Myers <cmyers@palantir.com>
Signed-off-by: Elijah Newren <newren@palantir.com>
Signed-off-by: Elijah Newren <newren@palantir.com>
Signed-off-by: Elijah Newren <newren@palantir.com>
@moy
Copy link
Contributor

moy commented May 28, 2015

I have mixed feelings about including this in git-multimail. I'm not an Atlassian Stash user, so I cannot test, and I can't even review it efficiently because I don't know the Stash API. I consider merging code as a (possibly weak) promise to maintain it afterwards, and I can't make this promise at the moment.

OTOH, having an all-in-one ready to use package to plug git-multimail into Stash may be nice for Stash users.

I'd say that a prerequisite to merge would be that someone (@newren?) steps-in as a subsystem maintainer.

Another option is to reverse the dependency: have the Stash plugin in a separate repo, but let the plugin download git_multimail.py automatically on install or so. If there's an easy technical way to do this, that would be the best option to me.

@moy moy added the incomplete label May 28, 2015
@newren
Copy link
Contributor Author

newren commented May 29, 2015

Stash plugins, the most straightforward way I know to write them, have internal storage for their settings (per repo) and provide automagic REST API end points and such for modifying them. It was certainly easiest to just keep the values there and pass them via command line flags to git-multimail, and perhaps there are technical challenges to trying to grab the values from the config for the REST API, though maybe I was just being excessively lazy not trying make sure these values were also stored in the local git config file of the relevant repo. I'll look into it.

Also, reversing the dependency and having git-multimail as a submodule seems fine.

@moy
Copy link
Contributor

moy commented May 29, 2015

Nice to see you back!

About command-line arguments, another option would be to allow

git-multimail.py -c multimailhook.foo=bar

just like

git -c config.option=value

to set an arbitrary Git configuration value. Internally, this basically sets an environment variable so it should be rather easy to implement. That would solve once and for all the problem of CLI options Vs Config options.

@newren
Copy link
Contributor Author

newren commented May 30, 2015

Ooh, I like the -c option idea. Let me take a look at that.

@moy
Copy link
Contributor

moy commented May 31, 2015

In git, the -c option works by setting the environment variable GIT_CONFIG_PARAMETERS (an environment variable is needed because not all Git commands are builtin). For example,

git -c foo.bar=boz -c x.y=z\' 

will set:

GIT_CONFIG_PARAMETERS='foo.bar=boz' 'x.y=z'\'''

@moy
Copy link
Contributor

moy commented May 31, 2015

BTW, one way to use -c without modifying git_multimail.py is to rename it like git-multimail, put in in your $PATH or Git's exec-path, and run git -c foo.bar=boz multimail. But that's not really convenient: in a typical installation, I don't have git_multimail.py in my $PATH and I hardcode its path in the hook.

@moy
Copy link
Contributor

moy commented Jun 2, 2015

OK, I'm tagging this "not-for-merge" as reversing the dependency from stash plugin to git_multimail.py seems the way to go. I'm keeping this open as a reminder that there's something to be done though.

@moy
Copy link
Contributor

moy commented Jun 23, 2015

The git-multimail -c idea is now in issue #102, so I think no more action is needed on my side for this PR. @newren : Can I close it?

@moy
Copy link
Contributor

moy commented Jun 29, 2015

No news => I'm closing the PR. Feel free to add comments and reopen if needed.

@moy moy closed this Jun 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants