This change is mostly a cleanup which simplifies a bit the iteration
over the list of commits for which to perform the various checks,
by having that information provided via an attribute whose name
is very specific about the semantics of what to use that list for.
This was motivated by an issue we discovered recently where we forgot
to exclude some commits when calling the commit-extra-checker hook.
This happened because, no matter how simple the algorithm for
computing the list of commits to check was, that simple algorithm
was duplicated in many places. This avoids that, in the hopes that
this will prevent this same kind of issues again in the future.
Another way to view the benefits of this cleanup is that this will
help ensure consistency in terms of the list of commits to which
the various checks are applied.
Note that we could have logically split this patch into two steps,
with the first step simply renaming the "added_commits" attribute
into "new_commits_for_ref", and then a second step introducing
the "commits_to_check" attribute. In the end, both are combined
in a single commit because it seems easier to review every location
where the "added_commits" attribute was used, and make sure from
the context that the correct list of commits was chosen in each
instance the "added_commits" list used to be referenced.
Otherwise, it's harder to review the second commit adding
the "commits_to_check" attribute, because the reviewer then has to
audit the entire set of sources himself in order to make sure
no spot was missed.
No actual behavior change should result from this change, and
therefore no test is being added.
Change-Id: I93c206968800dc738d3ebe4f5424f9201875383b
TN: T929-030
As part of determining which Update class it needs to instantiate
for the reference udpate being evaluated, the udpate factory
computes the following information:
- The kind of reference being updated (branch? notes? tag?)
- What operation on the branch is being performed (create? delete?
update?)
- The type of object the commit targeted by the reference
(commit? tag?)
In the context of enhancing the git-hooks to allow projects to provide
custom-checks on commits, the first and third items seem like these
could be useful information to pass to those hooks. In order to allow
this, without recomputing that information, this commit enhances
the AbstractUpdate class __init__ method to add those as additional
required paramaters, and then stores them as two new attributes.
As a bonus side-effect of this change, the new_rev_type attribute
is no longer necessary, which allows us to save one external call
to Git.
In the meantime, having these new attributes means that we can use
those to cross-check, within each AbstractUpdate child class'
self_sanity_check method, that the ref_kind and object_type values
correspond to each class' expectation (in other words, we cross-check
that the factory instantiated the correct class).
One other side-effect of that change is that we are no longer calling
the get_object_type function with a null SHA1 anymore. We could
modify the function's implementation to only accept non-null SHA1
revisions, but this would denature the function, in my opinion.
There is a genuine chance that perhaps, one day, we'll need that
again. So, instead, we cover that function by adding a new unit test
instead.
Change-Id: I8fd1ce180a6e17c0401b4dee07b9bc07d2abfdda
TN: T209-005
This is a followup on the previous commit, which introduced a feature
allowing projects to control which branches can be deleted.
This feature is added separately, as it is only a minor enhancement,
and this allows to keep the previous commit more focused.
Change-Id: I90d630e0a5b37d092db408379193e7a863201f8b
TN: T209-004
Note that tag deletion privileges are controlled via a different
and already existing option (hooks.allow-delete-tag). Also, notes
are not covered as notes deletion is currently not allowed at all.
Change-Id: I1ecfd83cf27861885f98c31bdc416f67d2dddb5e
TN: T209-004
The idea is to introduce a new set of configuration options allowing
repositories to set alternate naming up for branches and tags.
And, in order to have the default to recognize all standard names
while at the same time give users an option to restrict the name
of the branches/tags that one can have in the standard naming,
we are introducing another option called...
use-standard-{branch,tag}-ref-namespace
... whose default is True.
For repositories that want to add alternate naming schemes without
restrictions in the standard branch/tag namespaces, they just add
the alternate branch/tags names via (with "xxx" being a regular
expression):
{branch,tag}-ref-namespace = xxx
For repositories that want to restrict the branch/tag naming within
the standard namespaces, they start by dropping the standard
ref naming, and then add the ref-naming that they want to allow:
use-standard-{branch,tag}-ref-namespace = false
{branch,tag}-ref-namespace = xxx
{branch,tag}-ref-namespace = yyy
To support this feature, the engine for determining the type of
update we are facing has been made more dynamic. Because things
are more flexible, it can be harder to understand what is happening
when users are faced with an error. In that context, a reasonable
amount of effort is spent trying to provide helpful error messages
for the more common error cases. More far-fetched scenarios are
covered by testing, but no effort is made to provide detailed
diagnostic (at least not yet).
A fair amount of testing has been added, covering a matrix of scenarios.
Some existing tests also had to be adapted as follow:
- check_update_utest: Adapt to the new (more precise) error message;
- post_receive_one_utest: The scenario we were using prior to
this change now triggers an exception, and so we no longer
reach the code we wanted to cover with that scenario. So modify
the repository data, and try to push a commit which is a tag
instead, using a reference name that indicates a branch.
Change-Id: I0b2404c4c4b0d240e9ad29458fb75ddd1cd7dc7d
TN: T209-003
This commit centralizes the computation of the string image of
our update object's reference, so as to produce strings like
the following:
'branch_name'
... or ...
'branch_name' in namespace 'refs/namespace'
... or ...
'branch_name' was created in namespace 'refs/namespace'
... etc.
The benefits are not super obvious as of this moment, but these
will become more handy once we add support for custom reference
naming, where it can become more common for user to send update
requests of references that are outside the standard namespace
for branches.
Change-Id: Iecbb0830bd15283d3b17dde679e4b4230e698e94
TN: T209-003
The latter is really all-encompassing, and expected to disable all
precommit checks. And it would be used in the scenario that lead
to the creation of the hooks.no-gitreview-check config. So it should
be good enough for now to simply rely on hooks.no-precommit-check.
For Q802-008.
This patch rejects a branch creation if it looks like the user
forgot to update the gerrit.defaultbranch config in the .gitreview
file. This is to prevent some fairly confusing behavior when
we forgot..,
For P426-022
This patch enhances the get_update_email_contents method of all
child classes of AbstractUpdate which were returning email
subject and body to also return a distribution list. For now,
all cover emails are always sent to everyone, as this is what
seems to make the most sense.
At the moment, the distribution list is still completely static,
but, for cover emails, the computation of that list is centralized
in a new AbstractUpdate.everyone_emails method, which will be
enhanced later to handle scripts as well.
For NC07-001.
... to avoid having to call git to get the information. This is
a micro-of-all-micro optimizations (we expect a gain of 2 * 4ms
per reference), irrespective of the number of commits being pushed.
But it is such an easy optimization without any worsening of
the code maintenance, so it was implemented.
The primary motivation behind this patch is simplification of
the code, after having noticed that both the update and post-receive
hooks create the object. So we can make it an attribute computed
at instantiation time, and simplify many of the methods implemented
by this class.
The second motivation behind this patch is also a slight performance
improvement, measurable when the number of files changed by a push
is getting more important. This is the change in pre_commit_checks.py,
which now takes an extra argument providing the project/repo name,
instead of recomputing it (involving running git) for each and every
file being checked.
The performance gain is fairly modest, measured to be around 4ms per
file being checked, but worth taking, given the fact that it does not
complicate the code in pre_commit_checks.
The fact that utils.py imports config.py makes it difficult for
config.py to access the InvalidUpdate exception. So move this
exception to its own package, free of dependencies.
Preparation work for LC27-007.
... implemented as properties. This allows us to simplify a bit
several of the methods defined by this class.
This will also make this information available for the methods
which were not passed these lists. This should become useful
for methods such as NotesUpdate.validate, which now has access
to the list of notes being updated, and thus can verify that
the annotated commit exists in the repository.
Part of LC24-002.
