Note that, as part of the implementation, this commit introduces
a couple of calls to Git for each commit meant to retrieve the entire
list of files in a given commit. Luckily, one of these two calls
was already part of another check (filename collisions), and
the result of that call is cached in a CommitInfo object, so
the delta is only one extra Git call. Some performance measurements
seem to indicate that this is a fast-enough operation that users
should not notice the difference in performance, even for repositories
with a very large number of files [1]. If real-life usage proves
this assessment wrong, however, we'll revisit the documented behavior
so as to allow the use of a cheaper approach (e.g perform the check
on all modified files, rather than just the new ones).
[1]: Tested for instance on the GCC repository, which has more than
87,000 files in total; the first call took roughly 100ms, and
the next ones took about 60-70ms.
Change-Id: If0eea2c4990a945a3006cdd74d7a9aca2fc770d9
TN: T811-015
This commit enhances the CommitInfo class by adding a new method that
returns the list of all files in the repository for the given commit.
This factorizes some code which already exists, and allows for the
result to be cached in this object for reuse, something we intend
to do on a followup commit. In the meantime, this commit should be
a nop in practice.
Change-Id: I3f9328dd9e89f8d82ad2ad6fdac69b5e2fbc0992
TN: T811-015
This commit introduces a new class (ThirdPartyHook), which centralizes
the handling of third-party hooks (scripts that project set up via
the configuration file). Most of the code is simply a move of the code
from maybe_call_thirdparty_hook, with a couple of enhancements:
- Ability to specify the directory from which to call the hook;
- Handling of the situation when calling the hook itself fails.
We can already see the benefits of this in function style_check_files,
where the code now necessary to call the style_checker is both clearer
and more compact.
In the future, the intent is to push this class one step further to
allow users to specify hooks to be fetched from the repository, rather
than requiring that the hooks be installed (usually by an admin) on
the machine hosting the repository. The current class' API should
allow us to implement this in a way that's transparent to all users.
Change-Id: Ie8bf8accbbfd75a91b914628fad27d780532dac4
TN: T209-005
This is preparatory work for being to pass a certain amount of data
to user-defined hooks. The intent of this change is to limit the number
of times we compute that information to at most once.
Generally speaking, the main part of this change consists in adding
the following new methods to class CommitInfo:
- raw_revlog;
- raw_revlog_lines; and
The rest of this change is mostly adjustments to the code that needs
to access commits' rev logs to get them from a shared CommitInfo object
rather than from a play revision (SHA1).
Additionally, the function is_revert_commit in git.py, which took
a commit revision as a paramenter and needed a call to "git log"
to get the commit's body, has been replaced by a new method in
class CommitInfo. An alternative approach might have been to
keep the function, and change its parameter to be a CommitInfo object.
But it seemed more natural to make this a method of the CommitInfo
class instead, so this is what this change does.
Change-Id: Ia4bf23f24226d1e9eddafc61afd37db37f0f5287
TN: T209-005
There was a bug when returning an error message indicating that
the revision contains non-ISO-8859-15, that would always show
the first line has having the unwanted character, even when
that characters was actually on a different line.
And while at it, I improved the error message to include the line
number as well. Just in case showing the text itself is either not
sufficient or just not as efficient.
Credit for finding the bug: Thomas Quinot (thank you!).
Followup on QB25-001.
This is a performance enhancement which reduces the number of calls
to style_checker to, at most, once per commit. For repositories which
are configured to combine the commits for style-checking, the
style_checker should only be called once at most.
In terms of performance gains, the new approach increases
the performance by a factor of about 5 to 8 (measured using
a set of 10, 100, 1000 and 4500 files, the latter being the typical
commit in the GDB repository when adjusting the copyright year in
all GDB sources).
Roughly speaking, on an Intel(R) Core(TM) i7-7820HQ CPU @ 2.90GHz,
style-checking 4,500 text files, for which no check is actually
performed, took 31 seconds instead of 226 seconds. Using 4,500 Ada
files, for which we actually run the GNAT compiler and perform some
rulific checks, the new approach took 61 seconds instead of 230
seconds. The relative gain in the Ada case may appear to decrease
with the number of files to check, but 61 secs instead of 230 secs
is the difference between completing before the gerrit timeout
(around 2 minutes) or being aborted by the gerrit timeout.
For Q530-009.
This is preparation work towards calling the style checker only once
for an entire commit, rather than once per file.
This commit also adds a new testcase, allowing the code coverage to
remain at 100%.
For Q530-009.
Thanks to the transition to the new style checker, we can now
call it directly with the module name, without having to fake
the SVN path to that module.
Part of NA17-007.
The team felt that requiring the extra '(' and ')' around the keywords
we can use in revision logs aren't really all that necessary, so
this patch makes it all consistent within the git-hooks, but
the svn hooks as well.
This patch also adjusts the two corresponding tests
(M424-008__no_rh_check_keyword, and P426-035__no_precommit_check_in_rh)
to verify that the relaxed requirement is actually relaxed.
Part of OA23-034.
For a commit for which there are 4,500 files being modified and
for which all files have the no-precommit-check attribute set,
this reduces the amount of time it takes for the update hook
to process that commit from 20-25 seconds down to less than a second.
For P531-036.
Before this patch, we were generating one syslog entry for each
and every file that had the no-precommit-check attribute set -
meaning no style violation cvs_check. This not only generated
a lot of syslog entries, but also generated them for situations
that are not error situations. Since it is easy to determine
that information at any time in time, having that syslog trace
is really not necessary.
For P524-001.
This commit enhances the hooks to recognize the '(no-precommit-check)'
keyword in commit revision logs, and when found, disable the precommit
checks.
For P426-035.
Trying to push a simple non-merge commit on a branch for which
hooks.reject-merge-commits matches fails, with the hooks rejecting
the update saying that merge commits are not allowed.
This is due to an obvious bug in reject_commit_if_merge which
tests the number of parents of a commit and causes an error
if the number of parents is at least 1. But the correct test
is for the number of parents to be at least 2 (that's the definition
of a merge commit).
Fixes NC27-001.
