Import all changes made internally at AdaCore #12
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Change-Id: If427d4d91cf365e7d7b35eb5381a3d8c11cbc1ff TN: S425-012
The testsuite depends on git running with default configuration. Customizations in the user's global configuration ($HOME/.gitconfig) should be ignored. Change-Id: Iff4b3ba49294fd78bb1d5a78a908a9308ae0b5d8
This line of code was highlighted as uncovered by the testsuite's
coverage analysis, and reveals that we never call this method with
a CommitInfo object whose `parent_revs' attribute is None.
This attribute is first set during the class's initialization
(__init__) to the value of the parameter of the same name. And
the reason why we check for None here is because the __init__
routine documents this paramenter as allowing None, signifying
that the actual value hasn't been computed yet. The purpose of
this is to allow users who know that they don't need the parents
to skip computing them just for the sake of creating an object
of this class.
Upon investigation, it turns out that the only place where objects
of this class are instantiated is in function commit_info_list, where
the list of parents is actually computed and provided to the class.
This shows that, as of today, this uncovered code is actually
unreachable.
There were several options:
(a) It is conceivable that some callers might find it useful
that the parents get automagically computed on demand,
arguing in favor of keeping that code. If we keep the code,
we can cover it via Unit Testing.
(b) Document that the instantiators of this class must always
provide the list of parents at instantiation, thus making
the check for None moot, allowing us to simply remove it.
(c) Continue to allow the use of None for the list of parents,
but still removing the code, transfering the responsibility
to the users to be sure that it is not None before calling it.
In the end, it felt better to follow what is true today rather than
what "might be" tomorrow, so this commit rejects option (a).
Option (b) is simplest, but has the drawback of reducing flexibility
in terms of performance tweaking. This is why this commit chose
option (c), especially since it's trivial for users to handle this,
as it's just a matter of calling one function.
Change-Id: I3a01ab52c06ae36f249e0c01fbc7ea59616e6ded
When a test wants to verify the output of a given command, it normally
does this as follow:
self.assertRunOutputEqual(p, expected_out)
What this does is verify that the command's output matches, and if it
does not, then it not only reports and error, but also provides a diff
at the end, so as to facilitate the analysis.
However, while writing a test where that was unexpected failing
an output comparison, the diff in the error message was empty.
After investigation, it turned out that the function used to compute
that diff has a parameter ignore_white_chars set to True, which is
not what we want, here, and explains the discrepancy.
This commit fixes that by explicitly setting that parameter to False.
In the situation described above, this allowed the difference to
be shown in the diff section.
Change-Id: Ie5df10bcd7990eab928fc6f7fed3f41eaaa5d469
This lifts a limitation where non-fast-forward updates could only be allowed for references whose name start with 'refs/heads/' (the standard namespace for branches). As it happens, GCC is using branch names in a different namespace. So this commit changes this config option to match the entire reference name, rather than just the branch name. There is a break in compatibility for the repositories already using this configuration, but this change takes care of detecting this situation and provide an informative message explaining the likely situation. Change-Id: I330d15ccef448e815c94a620f008275f1e1914db TN: T209-002
Add a comment in the function implementing this feature explaining the reason behind the choices made for that email. Change-Id: I267f52ea651218b461e3d6c3c79882b887184c9b TN: T209-007
Just import the one symbol that's actually used by the various testcases in its place. This allows us to pass all current style checks at AdaCore for Python files. Change-Id: I458060771e18684e097e98131f292e09690a2e6d
Change-Id: I839ca83142d5a07d8e986c06656825aaad9822ea
This option allows users to specify a script to be run at update time, for each reference being updated. This script can then be used to perform additional checks on top of the standard checks already provided by our git-hooks. Change-Id: Iff527f1c9c0ba516ea5181c5f8c066c5175ef0ee TN: T209-001
This commit rewrites some code that was building a poor-man's version of an enum type by an enum.Enum subclass. No functional change. Seen while working on T209-003 (allowing custom ref naming) Change-Id: I5dbe9b69a421c980b98f52896b6048d53dc1f764
…arer This commit is only there to simplify the logic behind handing the data passed when calling the post_receive.py script. In particular, this script receives information: - via the command-line, with the optional --submitter-email option; - via stdin, with one line per reference being updated. I don't know what I was thinking at the time, but somehow, the way I chose to handle this was by appending to each entry from stdin the arguments from the command-line, and then use an ArgumentParser to pass each stdin entry. It works, but in practice, the it makes the code more difficult to understand. In fact, when I read this code again recently, I missed the tiny "+ sys.argv[1:]" and so it made me (wrongly) believe that the command-line arguments were entirely ignored! So, this commit simplifies this by clearly separating the phase of parsing the command-line, and the phase of processing each entry passed via stdin by git. Change-Id: I2e01a99e7c1931a56e9b95f196a2f73f9be70a2e TN: T223-004
Found (by Thomas Quinot) while working on T227-003 Change-Id: Ibe15874a39193328ad13814033b84c7be6c088ed
The main purpose of that pre-receive hook is to ensure that any push updating the refs/meta/config reference only updates that one reference. A large comment was added to the code to explain why (in short, this is a pre-requisite for being able to use the git-hooks config at the same time the config changes are being pushed). While at it, to be consistent with the other hooks, support for a repository-specific pre-receive hook is added. One side-effect of this is that we end up trying to query the hooks configuration, which triggers a check for the existence of the refs/meta/config branch. As a result, an extra copy of the same warning gets generated when the reference doesn't exist. This is not super elegant, but isn't a problem in practice, since this is part of handling a deprecated feature that we'll try to remove support for hopefully soon. In the meantime, this isn't blocking to adding support for the pre-receive option. Since this commit increased the number of times `refs/meta/config` was spelled out (aka "hardcoded"), this commit introduces a new constant to avoid duplicating that special reference name. Change-Id: Ib4fc0a32f639fe693e4d43269baf8c0838fc9214 TN: T227-003
…epo) When initially created, these hooks were reading the repository's configuration from an untracked file, installed in the repository. In December 2013, this method of configuring the hooks became obsolete, in favor of getting the information from a file which is checked in. However, to ease the transition, support for the untracked config file was kept as a fallback, with a large warning asking the user to contact the repository's admin to perform the transition. At this point, it is believed that all repositories have now transitioned, and therefore keeping this fallback is no longer necessary. This commit therefore removes support for that functionality. A couple of testcases are adjusted accordingly. As a side-effect, this caused the testing of branch creations for branches whose name does not follow the standard naming scheme to no longer be tested. This commit therefore adds one using a reference name which is not refs/meta/config, thus ensuring that testing coverage remains at 100%. Change-Id: I3e9c176f46e831ea82cdc930d32f7f22913a4fbc TN: T314-004
This commit enhances the hooks to read the repository's configuration
from the most recent version. In particular, if an update is updating
the refs/meta/config reference, then read the configuration from that
update, instead of reading from the existing refs/meta/config reference.
For updates of all other references, the behavior remains unchanged.
A testcase (T227-003__meta_config_is_immediate) is added to verify
that refs/meta/config reference creation works as expected.
Following that change, various testcases also needed to be adapted
as follow:
- MC27-006__meta_config_branch_create: Creating the refs/meta/config
reference now works;
- QC13-022__checker_config_on_its_own: This testcase was pushing
a local branch called "meta-config" to refs/meta/config. This
update introduced a style-checker-config-file configuration. Since
the configuration is now taken into account right away, the update
is was rejected due to the file not existing.
The branch "meta-config" is renamed to "meta-config-missing",
and a new branch "meta-config" was created, containg the missing
file in addition to the configuration changes above.
We first try to push the "meta-config-missing" to verify
that this is rejected. We then try to push the (new) "meta-config"
branch, verifying that the presence of the config file allows
the push to be accepted.
- T209-001__update_hook_not_found: A similar situation to the above,
but with the update hook. The added comments in the testcase
should make the changes self-explanatory.
- T209-001__update_hook_reject: Modify the update-hook, which is
called during reference updates, to not reject the refs/meta/config
update the testcase is doing as part of the preliminary setup phase.
Change-Id: I0bd7bc0801ba08cd281a751759bba8c48ba0c454
TN: T227-003
This is a followup on T227-003, where we enhanced the hooks to take
refs/meta/config updates into account right away. A small side-effect
of that change is that the error message when trying to delete the
refs/meta/config reference changed from...
| This type of update (refs/meta/config,delete) is currently unsupported
... to ...
| Unable to find the file project.config in refs/meta/config.
|
| Your repository appears to be incorrectly set up. Please contact
| your repository's administrator to set your project.config file up.
The former wasn't all that great, but understandable. The new one
is not incorrect, but not very helpful. This patch therefore enhances
the hooks to provide a clear error message:
| Deleting the reference refs/meta/config is not allowed.
|
| This reference provides important configuration information
| and thus must not be deleted.
A new testcase is added to make sure we continue verifying
the error message in that situation.
Change-Id: I5178b40c0d003fbf8ffeb456592750ec38a9a047
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
Up until recently, it was thought that it was not possible for the hooks to be called given a reference name with an old_rev and a new_rev both null. As it happens, it actually is possible, and so this commit adds better handling of this situation (instead of failing on an assertion that they aren't both null). A couple of testcases already testing the deletion of branches and tags have been expanded to try the same, but using a full reference name, instead of just the branch or tag's name. There was no need to add a test for git notes, because there is no "short" reference name in this case. So the test verifying git notes deletion was already using the full reference name... A test has been added to verify the behavior in the case when a full reference name is used, and that reference does not exist in the remote. Change-Id: If6e657312611ecbb0a66d0a8ae3763379d97e96f TN: T329-003
This commit fixes an oversight in the implementation of the AbstractUpdate.human_readable_ref_name method, where the default_namespace parameter was unintentionally ignore because a hardcoded value was used instead. This currently has no visible effect for now, because the only references for which we have been using this method are currently always branches. This will make a difference as soon as we use this method for other types of references, such as tags for instance. Change-Id: I94fe39523dd591f387990aa6884f06c993af1c1c TN: T209-003
This is preparation work towards providing support for tag references with a name that's outside the standard refs/tags/.* namespace. By using the full reference name, it works regardless of the namespace used for the tag. Change-Id: Iacfd539d33de28754abdcab873db3dea0e63e152 TN: T209-003
This is preparation work for allowing users to push tags outside of the standard refs/tags/.* namespace. For that, the intent is to use the AbstractUpdate.human_readable_ref_name method instead of using the short_ref_name when printing the tag name, with 'refs/tags' being the default_namespace. To make this functionality more convenient to use for tags, we introduce a new class called AbstractTagUpdate, inheriting from from AbstractUpdate and providing a new method called human_readable_tag_name. As a bonus, this makes the printing of tag names completely consistent across the various types of tag updates, but also consistent with branch updates, as shown by the changes in output in the various tests we had to modify. Change-Id: I8b0909001cc751d576a4c9cb4d17545acedb3462 TN: T209-003
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 is a simple cleanup commit, so as to make sure that, for all options listed has being a "tuple" option, the default value for the option is actually a tuple. In practice, the code is such that it doesn't matter (it works with None, or the empty string). But it's not the cleanest approach, and there is no reason for it. It's just as easy as using an empty tuple. Doing so actually reduces coverage by one line, inside function "to_tuple" in type_conversion.py. The uncovered code is actually useful outside of in the handling of the default values extracted from the GIT_CONFIG_OPTS dict. So, a test is added specifically in order to help cover this line of code as well. Seen while working on T512-052 Change-Id: I7e9a09c5514b29c18cdf7d92f2c5d3eabf0fe82b
This commit puts in place the necessary infrastructure to replace
replace the hooks.bcc-file-ci configuration in favor of a new
configuration option called hooks.filer-email. The difference
is that the latter is a tuple/list.
For this commit, the default for the new option is such that, for
repositories that don't use the hooks.bcc-file-ci, nothing changes.
This default will be changed in a subsequent commit, and a comment
in the code explain why we're doing it this way.
This change affects two tests we have in the testsuite, which both
were using the hooks.bcc-file-ci config option:
- "no_file_ci_bcc": We could have made this test pass by replacing
the hooks.bcc-file-ci option by an empty hooks.filer-email.
However, this would not be representative of the configuration
of a real repository. So, instead, we just adjust the repository's
configuration to match what it would typically look like, and
accept (XFAIL) that this test is failing until we change
the default for hooks.filer-email.
- "to_file-ci_only": This test is in a similar situation as the test
above, except that it has the additional distinction of now really
becoming pointless. However, pointless or not, it's a test which
requires little maintenance (if at all), so the decision was to
keep that test and apply the same treatment as above.
A new test is also added, to verify that it's possible to provide
more than one email address in hooks.filer-email.
Change-Id: I1733e57b204c49cd189c5a6e1d3f4c47846b40d6
TN: T512-052
This updates all the testcases where the hooks.filer-email is implicit,
so as to now be explicitly part of the test repository's configuration.
While we're making this change, we take this opportunity to use
a bogus email address instead of using an email address in one of
AdaCore's domains.
In most testcases, the test repositories' configuration was adjusted
using the following shell script:
| #/usr/bin/env zsh
|
| test_dir=$1
|
| if [ ! -d $test_dir ]; then
| exit 1
| fi
|
| cd $test_dir
| if [ ! -f hooks_config ]; then
| exit 1
| fi
|
| ../../bin/unpack-test-repos
| (
| cd repo
| current_branch=`git rev-parse --abbrev-ref HEAD`
|
| git fetch origin refs/meta/config
| git co FETCH_HEAD
| git config -f project.config hooks.filer-email 'filer@example.com'
| git ci -a -m 'project.config: add hooks.filer-email'
| git push origin HEAD:refs/meta/config
|
| git co $current_branch
| ) >/dev/null 2>&1
| ../../bin/pack-test-repos
And the test.py scripts have then been adjusted with the following perl
one-liner:
$ perl -pi -e "s/file-ci\@gnat.com/filer\@example.com/g" */test.py
A number of testscases (13, to be exact), had to be adjusted manually,
for a couple of reasons:
- The test repository had a branch that the test tries to push to
the refs/meta/config reference -- that push was being rejected
because it was no longer up to date (non-fast-forward update).
The branch was rebased, which changed the SHA1-s, and the diffs.
- The test was creating a commit on the fly that changes the
project.config file in the refs/meta/config reference. For those,
the change to make was to simply update the hooks_config file
in that test.
Change-Id: I626a29f77c3c2bb5dc13ebd2f11542a6fa066930
TN: T512-052
Now that the default is changed, we can remove a couple of XFAILs that were temporarily added. Change-Id: Id15e0b5ccdf83e82329e02327773687b2c94be42 TN: T512-052
pmderodat
approved these changes
Jun 10, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This cherry-picks all the changes made internally at AdaCore. The intention is to make the master branch on GitHub usable in the AdaCore context. With this change, the two versions are almost
identical, with the exception of: