Re: merge commit authorization
Sitaram Chamarty
<heiko.c...@de.ibm.com> wrote:
> Hi Sitaram,
>
> I want to enforce a "no-merge-commits-allowed" policy on some development
> branches for all users except the repo admins.
What exactly defines a merge commit?
Is it that the tip being pushed has 2 parents? Or is it that between
the old and the new ref there should be a straight line only?
> I have already an update hook script that will do all that, however I think
> naturally that would fit more into gitolite's access control logic.
>
> E.g. adding an extra "M" character to the RW+CD series with the meaning
> "merge commits allowed".
I'm reluctant to add anything at that level in core right now.
At this point, gitolite has come to the stage where, unless quite a
lot of people are asking for it, it probably won't go in.
Also remember that almost any unusual need can be met by a combination
of ADCs, custom 'config' variables, secondary hooks, and the (perl
only) API for checking if "this user is allowed to write to this
branch". It may not be as elegant as finding it in core, but that's
life.
regards
sitaram
Sitaram Chamarty
<heiko.c...@de.ibm.com> wrote:
> Yes, as I said, I already have a working solution, but as this means
> adding and deleting authorizations at two places if a member comes or
> or leaves it is not very elegant. But of course we can live with that :)
Tell me in more detail how you do it, and I'm sure we can manage to do
it so that you add auth in only one place (the conf file).
Send me a sample conf file with the hypothetical RW+CDM, and I will
see if I can make it happen without changing that syntax, but using
something else. I'm pretty sure something can be done to make it all
work without double entry of auth info, depending on how complex your
needs are of course.
regards
sitaram
Sitaram Chamarty
> repo test
> RW+CDM = admin
> R = @all
> RWM devel = user1 user2
> RW devel = @all
OK how about a config that looks like this:
repo foo
RW+ = u1
RW+ MERGE/ = u1
RW ab/cd = u2 u3 u4 u5
RW MERGE/ab/cd = u2 u3
# ^^^^^^ -- hey! we got branches with slashes in them!
We denote merge permissions by allowing an additional refex that
starts with MERGE/.
Now use the code I attached (you may have to upgrade; I always
test with the latest and right now I don't have the time to dig
deeper).
Now you owe me at least one good, *non-PHB* reason for why you
need this feature and under what conditions it makes sense.
Sitaram Chamarty
> On Fri, Jan 13, 2012 at 08:27:51PM +0530, Sitaram Chamarty wrote:
> > OK how about a config that looks like this:
> >
> > repo foo
> > RW+ = u1
> > RW+ MERGE/ = u1
> > RW ab/cd = u2 u3 u4 u5
> > RW MERGE/ab/cd = u2 u3
> > # ^^^^^^ -- hey! we got branches with slashes in them!
> >
> > We denote merge permissions by allowing an additional refex that
> > starts with MERGE/.
>
> the new syntax would imply. For these two lines in the above config:
>
> 1) RW+ = u1
> 2) RW+ MERGE/ = u1
>
> I would have expected that if line 2) is missing user u1 may not push any
> merge commits anymore.
Oops I missed that; you're right line 1 makes line 2 useless.
But I was trying to replicate your example conf in a previous
email in which 'admin' can do everything, so now he can.
However, if you want admin to be able to write to any branch but
not push merge commits, do this:
- MERGE/ = u1
RW+ = u1
That should do it.
> However your script uses a special branch write access check to figure out
> if a user may push a merge commit to a certain branch. Since line 1) gives
> user u1 write access to all branches he may also push merge commits to all
> branches.
>
> > # the ref is like refs/heads/foo; change it to refs/heads/MERGE/foo
> > $ref =~ s(^refs/heads/)(refs/heads/MERGE/);
> >
> > # now check if the fellow has rights to push to this new ref
> > my $ret = check_access($ENV{GL_REPO}, $ref, 'W', 1);
>
> $ret would be refs/.* for this case. Is that fixable?
It's actually whatever the fully qualified form of every refex
matched, which, for a blank refex, is 'refs/.*'.
You need to only look for the word DENIED. The presence or
absence of that word is all you should rely upon, sorry!
>
> Thanks,
> Heiko
>
Sitaram Chamarty
> On Fri, Jan 13, 2012 at 08:27:51PM +0530, Sitaram Chamarty wrote:
> >
> > repo foo
> > RW+ = u1
> > RW+ MERGE/ = u1
> > RW ab/cd = u2 u3 u4 u5
> > RW MERGE/ab/cd = u2 u3
> > # ^^^^^^ -- hey! we got branches with slashes in them!
> >
> > We denote merge permissions by allowing an additional refex that
> > starts with MERGE/.
> >
> > Now use the code I attached (you may have to upgrade; I always
> > test with the latest and right now I don't have the time to dig
> > deeper).
> >
> > Now you owe me at least one good, *non-PHB* reason for why you
> > need this feature and under what conditions it makes sense.
>
>
> There are two scenarios that I would like to avoid:
>
> 1) developer works on his local clone but on the main development
> branch. After a while he commits somehting locally and thinks it
> would be good to push that into the "main" repository. But somebody
> was faster and pushed something else already. So he will do just a
> simple "git pull" and ends up with a merge commit which he pushes
> too.
> What I want is nice git history that is not completely flooded with
> mostly useless ~30-50% merge commits. Hence I want to enforce a
> "git pull --rebase" policy.
IMO it's very subjective when this is good and when it is bad.
I can think of a lot of situations where I want to see the other
development sort of separately. I've seen people argue for
making --no-ff the *default* (although I would not go so far!)
It seems like a small thing to enforce, and as such I'd rather
trust the devs to do the right thing even if they forget once in
a while, but that's probably only true for a corporate
environment.
> 2) Enforcing no-merge-commit policy also makes sure that there are no
> merge commits that resolve merge conflict. This can be an advantage:
> e.g. I work also as a gate keeper: collect patches from other people
> and send them to mailing lists. Now, if there are merge commits that
> resolve merge conflicts (non-trivial merges), then all of a sudden
> it's up to me to figure out what commits/patches had conflicts with
> each other and sort the patch hunks out accordingly. So that in the
> end I have again a nice series of patches that can be sent to a
> mailing list.
That does make more sense, but don't you think part of it is
because email-as-carrier loses something? (Think of this as an
academic question; I am not trying to ask you *why* you're using
emails...)
> These are the two reasons why I'd like to have this feature and both
> are non-PHB reasons ;)
<sheepish grin>
indeed... and now that you mentioned emailing commits around and
being a gatekeeper I know enough to know what you're talking
about.
And that I'm probably closer to being a PHB than you are <sigh>
;-)
regards
sitaram
Sitaram Chamarty
> On Sun, Jan 15, 2012 at 07:05:49PM +0530, Sitaram Chamarty wrote:
> > On Sun, Jan 15, 2012 at 01:41:15PM +0100, Heiko Carstens wrote:
> > > That looks very good! However it will not really do what I first thought
> > > the new syntax would imply. For these two lines in the above config:
> > >
> > > 1) RW+ = u1
> > > 2) RW+ MERGE/ = u1
> > >
> > > I would have expected that if line 2) is missing user u1 may not push any
> > > merge commits anymore.
> >
> > Oops I missed that; you're right line 1 makes line 2 useless.
> >
> > But I was trying to replicate your example conf in a previous
> > email in which 'admin' can do everything, so now he can.
> >
> > However, if you want admin to be able to write to any branch but
> > not push merge commits, do this:
> >
> > - MERGE/ = u1
> > RW+ = u1
> >
> > That should do it.
>
> came up with (using git config keys).
Thanks.
But I hope you're not committed to that in any way because
you're about to abandon it ;-)
It turns out that the actual code changes to do what you
originally asked were ridiculously low. It took me more time to
write the test script.
So it's there now, on pu. If you can take it for a spin and
tell me if it looks good, I'll even call it v2.3.
Note that I chose to ignore the creation/deletion issue. If a
repo has merge-check enabled (i.e., any of the applicable access
rules has the "M" suffix to the current "RW..." permissions),
and if someone tries to create/delete a ref, they will get a
warning message but no merge-check will happen.
I could have found some way (like maybe 'git rev-list -1
--merges $newsha --not --all') but I think that's overkill. As
far as I can tell, both your examples are really only relevant
to pushing to an existing branch, and don't translate well (if
at all) to new branch creation.
regards
sitaram
Heiko Carstens
> > That looks very good! However it will not really do what I first thought
> > the new syntax would imply. For these two lines in the above config:
> >
> > 1) RW+ = u1
> > 2) RW+ MERGE/ = u1
> >
> > I would have expected that if line 2) is missing user u1 may not push any
> > merge commits anymore.
>
> Oops I missed that; you're right line 1 makes line 2 useless.
>
> But I was trying to replicate your example conf in a previous
> email in which 'admin' can do everything, so now he can.
>
> However, if you want admin to be able to write to any branch but
> not push merge commits, do this:
>
> - MERGE/ = u1
> RW+ = u1
>
> That should do it.
It does. Thank you! I think your solution is much better than the one I
came up with (using git config keys).
Thanks a lot for helping out! :)
Heiko
Heiko Carstens
> On Fri, Jan 13, 2012 at 3:42 PM, Heiko Carstens
> <heiko.c...@de.ibm.com> wrote:
> > Hi Sitaram,
> >
> > I want to enforce a "no-merge-commits-allowed" policy on some development
> > branches for all users except the repo admins.
>
> What exactly defines a merge commit?
>
> Is it that the tip being pushed has 2 parents? Or is it that between
> the old and the new ref there should be a straight line only?
That means anything between old and new rev is supposed to have only one
parent:
git rev-list -n 1 --merges $oldrev..$newrev
should return nothing (new branches, where oldrev is zero need special
handling, but that's a different story).
> > I have already an update hook script that will do all that, however I think
> > naturally that would fit more into gitolite's access control logic.
> >
> > E.g. adding an extra "M" character to the RW+CD series with the meaning
> > "merge commits allowed".
>
> I'm reluctant to add anything at that level in core right now.
>
> At this point, gitolite has come to the stage where, unless quite a
> lot of people are asking for it, it probably won't go in.
Sure, no problem.
> Also remember that almost any unusual need can be met by a combination
> of ADCs, custom 'config' variables, secondary hooks, and the (perl
> only) API for checking if "this user is allowed to write to this
> branch". It may not be as elegant as finding it in core, but that's
> life.
Yes, as I said, I already have a working solution, but as this means
adding and deleting authorizations at two places if a member comes or
or leaves it is not very elegant. But of course we can live with that :)
Thanks,
Heiko
Heiko Carstens
>
> repo foo
> RW+ = u1
> RW+ MERGE/ = u1
> RW ab/cd = u2 u3 u4 u5
> RW MERGE/ab/cd = u2 u3
> # ^^^^^^ -- hey! we got branches with slashes in them!
>
> We denote merge permissions by allowing an additional refex that
> starts with MERGE/.
That looks very good! However it will not really do what I first thought
the new syntax would imply. For these two lines in the above config:
1) RW+ = u1
2) RW+ MERGE/ = u1
I would have expected that if line 2) is missing user u1 may not push any
merge commits anymore.
However your script uses a special branch write access check to figure out
if a user may push a merge commit to a certain branch. Since line 1) gives
user u1 write access to all branches he may also push merge commits to all
branches.
> # the ref is like refs/heads/foo; change it to refs/heads/MERGE/foo
> $ref =~ s(^refs/heads/)(refs/heads/MERGE/);
>
> # now check if the fellow has rights to push to this new ref
> my $ret = check_access($ENV{GL_REPO}, $ref, 'W', 1);
$ret would be refs/.* for this case. Is that fixable?
Thanks,
Heiko
Heiko Carstens
>
> repo foo
> RW+ = u1
> RW+ MERGE/ = u1
> RW ab/cd = u2 u3 u4 u5
> RW MERGE/ab/cd = u2 u3
> # ^^^^^^ -- hey! we got branches with slashes in them!
>
> We denote merge permissions by allowing an additional refex that
> starts with MERGE/.
>
> Now use the code I attached (you may have to upgrade; I always
> test with the latest and right now I don't have the time to dig
> deeper).
>
> Now you owe me at least one good, *non-PHB* reason for why you
> need this feature and under what conditions it makes sense.
Sorry, forgot to answer you question!
There are two scenarios that I would like to avoid:
1) developer works on his local clone but on the main development
branch. After a while he commits somehting locally and thinks it
would be good to push that into the "main" repository. But somebody
was faster and pushed something else already. So he will do just a
simple "git pull" and ends up with a merge commit which he pushes
too.
What I want is nice git history that is not completely flooded with
mostly useless ~30-50% merge commits. Hence I want to enforce a
"git pull --rebase" policy.
2) Enforcing no-merge-commit policy also makes sure that there are no
merge commits that resolve merge conflict. This can be an advantage:
e.g. I work also as a gate keeper: collect patches from other people
and send them to mailing lists. Now, if there are merge commits that
resolve merge conflicts (non-trivial merges), then all of a sudden
it's up to me to figure out what commits/patches had conflicts with
each other and sort the patch hunks out accordingly. So that in the
end I have again a nice series of patches that can be sent to a
mailing list.
These are the two reasons why I'd like to have this feature and both
are non-PHB reasons ;)
Thanks,
Heiko
Heiko Carstens
> On Sun, Jan 15, 2012 at 02:27:13PM +0100, Heiko Carstens wrote:
> > There are two scenarios that I would like to avoid:
> >
> > 1) developer works on his local clone but on the main development
> > branch. After a while he commits somehting locally and thinks it
> > would be good to push that into the "main" repository. But somebody
> > was faster and pushed something else already. So he will do just a
> > simple "git pull" and ends up with a merge commit which he pushes
> > too.
> > What I want is nice git history that is not completely flooded with
> > mostly useless ~30-50% merge commits. Hence I want to enforce a
> > "git pull --rebase" policy.
>
> IMO it's very subjective when this is good and when it is bad.
> I can think of a lot of situations where I want to see the other
> development sort of separately. I've seen people argue for
> making --no-ff the *default* (although I would not go so far!)
Sure, I agree with you that this is a matter of taste.
> > 2) Enforcing no-merge-commit policy also makes sure that there are no
> > merge commits that resolve merge conflict. This can be an advantage:
> > e.g. I work also as a gate keeper: collect patches from other people
> > and send them to mailing lists. Now, if there are merge commits that
> > resolve merge conflicts (non-trivial merges), then all of a sudden
> > it's up to me to figure out what commits/patches had conflicts with
> > each other and sort the patch hunks out accordingly. So that in the
> > end I have again a nice series of patches that can be sent to a
> > mailing list.
>
> That does make more sense, but don't you think part of it is
> because email-as-carrier loses something? (Think of this as an
> academic question; I am not trying to ask you *why* you're using
> emails...)
Well, part of our development tool chain/process allows to send mails
automatically whenever somebody pushed something to a list of recipients
in the hope the code gets some review. However I think reviewing merge
commits which resolved a merge conflict is very difficult (maybe it's
just me, don't know).
E.g. a recent example (from upstream) is this one here:
Just looking at the diff I would say it's very hard to tell if the result
is correct, even though it was my code that got changed :)
I'd like to avoid having to review such patches.
Heiko Carstens
> On Fri, Jan 13, 2012 at 5:35 PM, Heiko Carstens
> <heiko.c...@de.ibm.com> wrote:
>
> > Yes, as I said, I already have a working solution, but as this means
> > adding and deleting authorizations at two places if a member comes or
> > or leaves it is not very elegant. But of course we can live with that :)
>
> Tell me in more detail how you do it, and I'm sure we can manage to do
> it so that you add auth in only one place (the conf file).
Oh, I think I was not very clear (hard without a sample config).
Everything gets already configured only in the conf via new per repo
config keys.
So an example would be:
repo test
RW+CD = admin
R = @all
RW devel = @all
config hooks.allowmerge.admin = true
config hooks.allowmerge.branch.devel.user1 = true
config hooks.allowmerge.branch.devel.user2 = true
That is: admin may push merge commits everywhere. user1 and user2 only to
branch devel. Everbody else may not push any merge commits.
So, I simply use git config keys to configure if a user is allowed to
push merge commits. (default policy is: no merge commits allowed).
A secondary update script then evaluates the config variables and dependent
on that checks if there are any merge commits.
So the config keys look like this:
hooks.allowmerge.<user> # allow <user> to push merge commits everywhere
hooks.allowmerge # allow everybody to push merges everywhere if "true"
hooks.allowmerge.branch.<branch>.<user> # allow <user> to push merge commits
# to branch <branch>
hooks.allowmerge.branch.<branch> # allow everybody to push merge commits
# to branch <branch>
Yes, the logic is a bit too complex right now, and I will simplify it,
but this is just to give you an idea of what functionality is needed.
And of course there is drawback since e.g. "/" cannot be used in branch names
with this approach, but that is ok.
With a new M character the example from above would look like this:
repo test
RW+CDM = admin
R = @all
RWM devel = user1 user2
RW devel = @all
Hope that makes sense..
Thanks,
Heiko
Heiko Carstens
I want to enforce a "no-merge-commits-allowed" policy on some development
branches for all users except the repo admins.
I have already an update hook script that will do all that, however I think
naturally that would fit more into gitolite's access control logic.
E.g. adding an extra "M" character to the RW+CD series with the meaning
"merge commits allowed".
What do you think?
Thanks,
Heiko
Heiko Carstens
> On Sun, Jan 15, 2012 at 10:49:09PM +0100, Heiko Carstens wrote:
> > It does. Thank you! I think your solution is much better than the one I
> > came up with (using git config keys).
>
> Thanks.
>
> But I hope you're not committed to that in any way because
> you're about to abandon it ;-)
No problem, since now you implemented my original proposal! :)
> So it's there now, on pu. If you can take it for a spin and
> tell me if it looks good, I'll even call it v2.3.
I just did some testing and I think it works like it should.
Just as a side note: maybe you want to update the section
"## summary: permissions" in 'docs/gitolite.conf.mkd' to reflect
the new "M" character as well?
> Note that I chose to ignore the creation/deletion issue. If a
> repo has merge-check enabled (i.e., any of the applicable access
> rules has the "M" suffix to the current "RW..." permissions),
> and if someone tries to create/delete a ref, they will get a
> warning message but no merge-check will happen.
>
> I could have found some way (like maybe 'git rev-list -1
> --merges $newsha --not --all') but I think that's overkill. As
> far as I can tell, both your examples are really only relevant
> to pushing to an existing branch, and don't translate well (if
> at all) to new branch creation.
Actually I had that construct as well in my scripts but after
some time I though that whoever has enough permissions to create
a new branch my as well push merge commits in an initial push.
So I ended up with the same behaviour as your implementation:
if $old or $new is only zeros let the push pass.
Thanks again,
Heiko
Sitaram Chamarty
<heiko.c...@de.ibm.com> wrote:
> On Mon, Jan 16, 2012 at 11:37:27AM +0530, Sitaram Chamarty wrote:
>> On Sun, Jan 15, 2012 at 10:49:09PM +0100, Heiko Carstens wrote:
>> > It does. Thank you! I think your solution is much better than the one I
>> > came up with (using git config keys).
>>
>> Thanks.
>>
>> But I hope you're not committed to that in any way because
>> you're about to abandon it ;-)
>
> No problem, since now you implemented my original proposal! :)
>
>> So it's there now, on pu. If you can take it for a spin and
>> tell me if it looks good, I'll even call it v2.3.
>
> I just did some testing and I think it works like it should.
Thanks. I'll wait a few days just in case then tag it v2.3
> Just as a side note: maybe you want to update the section
> "## summary: permissions" in 'docs/gitolite.conf.mkd' to reflect
> the new "M" character as well?
Oh I missed that, didn't I? I'll fix it...
>> Note that I chose to ignore the creation/deletion issue. If a
>> repo has merge-check enabled (i.e., any of the applicable access
>> rules has the "M" suffix to the current "RW..." permissions),
>> and if someone tries to create/delete a ref, they will get a
>> warning message but no merge-check will happen.
>>
>> I could have found some way (like maybe 'git rev-list -1
>> --merges $newsha --not --all') but I think that's overkill. As
>> far as I can tell, both your examples are really only relevant
>> to pushing to an existing branch, and don't translate well (if
>> at all) to new branch creation.
>
> Actually I had that construct as well in my scripts but after
> some time I though that whoever has enough permissions to create
> a new branch my as well push merge commits in an initial push.
That too, I guess, although I was thinking more that, for the specific
problems you highlighted, your devs can only cause you trouble if they
push into a branch you manage, which means it already exists.
But perhaps I didn't grok the workflow as well as I thought I did.
Doesn't matter; we both came to the same end result anyway.