www-infrastructure-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Davis <paul.joseph.da...@gmail.com>
Subject Re: @apache.org commit address requirement (Was: Git hosting is go)
Date Wed, 14 Dec 2011 17:04:11 GMT
On Wed, Dec 14, 2011 at 7:19 AM, Jukka Zitting <jukka.zitting@gmail.com> wrote:
> Hi,
>
> The asfgit pre_receive hook currently checks that each commit being
> pushed has its committer email (%ce) set to the @apache.org address of
> one of committers of the respective project.
>
> This seems too strict and makes it quite cumbersome to handle things
> like pull requests where you'd want to take in incoming commits as-is
> and just merge them to the main branch instead of rebasing or
> rewriting the commits. We can't even rely on the %ce information being
> correct since it's set on the client side.
>
> Instead of this %ce check I'd leave the commit messages as-is and
> rather augment the ref-updates.log with information about who pushed
> each individual commit. That gives us a more reliable record for tying
> individual changes to ICLAs than the %ce addresses in commit messages.
>
> Alternatively, if we do want to include explicit and properly verified
> ICLA references in the actual Git commit messages (instead of an
> out-of-band mechanism like ref-updates.log), then I'd rather use
> separate Signed-Off-By labels that the committer who pushes a set of
> commits needs to add to each commit being pushed.
>
> WDYT? I can make the changes if we can reach consensus on this.
>
> [See below for the relevant context from callback-dev@]
>
> BR,
>
> Jukka Zitting
>
> ---------- Forwarded message ----------
> From: Jukka Zitting <jukka.zitting@gmail.com>
> Date: Wed, Dec 14, 2011 at 1:58 PM
> Subject: Re: Git hosting is go
> To: callback-dev@incubator.apache.org
>
>
> Hi,
>
>
> On Wed, Dec 14, 2011 at 12:05 PM, Jukka Zitting <jukka.zitting@gmail.com> wrote:
>> I'm not sure how strict the backend is about commits pulled from
>> elsewhere. Ideally it should only record the currently authenticated
>> user or at most require a Signed-Off-By entry from the committer who's
>> pushing the commits. I'll see what I can find out.
>
> The backend check that the committer (not author) address of each
> commit being pushed is an @apache.org address of one of the committers
> authorized to push to the repository.
>
> Checking the committer address seems way too strict as it would
> require breaking history on all pull requests from outside the core
> development group. I'll bring this up on infra-dev@ [1], to see how we
> could best address the issue.
>
> [1] http://mail-archives.apache.org/mod_mbox/www-infrastructure-dev/
>
> BR,
>
> Jukka Zitting

The ref updates are already logged with the change, the ldap
authorized id, time and ip of who pushed so that should be sufficient
there.

As to the committer email checks, I was a bit uncertain when
implementing this part. One the one hand I wanted to be very specific
that every commit was committed by someone acceptable for the project
but this has the already pointed out draw back that it requires
committers to rebase commits after reviewing them which doesn't work
so hot with some peoples' work flow.

In a perfect world what I'd really like to see here is the ldap
self-service have email fields and an ICLA/CCLA check boolean so that
we could just check that the email address is connected to a CLA of
some sort. I'm quite hesitant to remove all checks from this part
because it's quite easy for those fields to be garbage when someone
forgets to set their name or if someone accidentally pulls unreviewed
code in a fat finger branch update.

Also I do realize that this isn't a security thing by any means.
Anyone could just fake these fields but at that point they'd have to
go out of their way to lie about who wrote it and the onus is still on
the person that triggered the push command. But on the other hand
checking that the committer is valid seems beneficial enough to not
drop it entirely.

Mime
View raw message