Bug 268024 - .hooks/pre-commit.d/check_portepoch broken on lines affecting PORTEPOCH comments
Summary: .hooks/pre-commit.d/check_portepoch broken on lines affecting PORTEPOCH comments
Status: New
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Ports Framework (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Port Management Team
URL:
Keywords: patch, regression
Depends on:
Blocks:
 
Reported: 2022-11-27 22:53 UTC by Matthias Andree
Modified: 2022-12-18 08:59 UTC (History)
4 users (show)

See Also:


Attachments
fix proposal for check_portepoch to fix various issues with it (1.86 KB, patch)
2022-11-27 22:53 UTC, Matthias Andree
mandree: maintainer-approval? (portmgr)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Andree freebsd_committer freebsd_triage 2022-11-27 22:53:34 UTC
Created attachment 238381 [details]
fix proposal for check_portepoch to fix various issues with it

I was editing dns/dnsmasq-devel 2.88rc3, to upgrade to rc5, and removed a PORTEPOCH comment. This tripped up check_portepoch, which cannot cope with a situation that git renders PORTEPOCH diffs that do not relate to ^PORTEPOCH=23 lines, and it printed 

[pre-commit] dropped PORTEPOCH  in dns/dnsmasq-devel/Makefile

So, because I was editing PORTEPOCH comments, git was emitting the lines, but grep was not picking them up. Right. And then the first check for the exit 1 path was simplistic and just assumed "no new portepoch, we're broken". Untrue.


To fix, only complain about an empty PORTEPOCH if we had one before. If both old and new are empty, we may be safe, so do not complain and do not error out.

Also note that GNU grep complains about the regexps, because it runs without -E option, but the basic regex has \- (you never escape -) and \+ (which has reverse meaning, and you do not want to match multiple ^ anchors instead of matching the literal +), so fix that, too, and be stricter to only look at ^PORTEPOCH lines that have a = somewhere.
Comment 1 Matthias Andree freebsd_committer freebsd_triage 2022-11-27 22:56:28 UTC
while here, should not we redirect such complaints to stderr with adding >&2 to echo?  Just add that when committing.
Comment 2 Tobias C. Berner freebsd_committer freebsd_triage 2022-11-28 09:13:57 UTC
hm, given comments are the issue maybe do 

"^-[^#]*PORTEPOCH"  

in the regex instead of the "=.*"?

Because if you had a comment ala
# previously this was PORTEPOCH=3 with the old orgin foo/bar
that would still trip it erroneusly.

mfg Tobias
Comment 3 Matthias Andree freebsd_committer freebsd_triage 2022-11-29 21:39:23 UTC
(In reply to Tobias C. Berner from comment #2)
A diff comment line would not be matched by a *WORKING* grep regexp. The old regexp was tripping grep up by ^\+PORTEPOCH, which caused GNU grep to ignore this whole stuff. A comment would be ^-#.*PORTEPOCH or ^+#.*PORTEPOCH as basic regular expression (grep without -E) and would not match.
Comment 4 Tobias C. Berner freebsd_committer freebsd_triage 2022-11-30 07:12:21 UTC
(In reply to Matthias Andree from comment #3)
No, that would only match a line that stats with a comment my suggestion would be to match lines starting with +- and containing PORTEPOCH=, but no '#' prior to the PORTEPOCH string.
Comment 5 Matthias Andree freebsd_committer freebsd_triage 2022-12-05 19:08:21 UTC
(In reply to Tobias C. Berner from comment #4)
How far away are we from "make -C ... -V PORTEPOCH" now?
Comment 6 commit-hook freebsd_committer freebsd_triage 2022-12-18 08:59:49 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=414b5c61645669a346fc817e0b5852c265b96187

commit 414b5c61645669a346fc817e0b5852c265b96187
Author:     Matthias Andree <mandree@FreeBSD.org>
AuthorDate: 2022-11-27 12:51:51 +0000
Commit:     Tobias C. Berner <tcberner@FreeBSD.org>
CommitDate: 2022-12-18 08:58:17 +0000

    .hooks/pre-commit.d: unbreak EPOCH checker

    dns/dnsmasq-devel as of 2.88rc3 contained a comment about PORTEPOCH,
    which I removed in the 2.88rc5. This tripped up the checker because
    it assumed that if git yielded lines containing PORTEPOCH, then it
    must have been PORTEPOCH= or similar lines. Untrue in my case.

    It was printing
    [pre-commit] dropped PORTEPOCH  in dns/dnsmasq-devel/Makefile

    To solve, only pick out PORTEPOCH diffs that are actual assignments,
    and if the new PORTEPOCH is empty, and the old one is also,
    ignore this condition (previously we would exit 1, which is bogus).

    Also, grep without -E should not have \ in front of - or +.
    FreeBSD 13.1 grep is fine, but GNU grep ignores those backslashes
    noisily (and I have prepended it to PATH) and emits warnings.

    PR:             268024

 .hooks/pre-commit.d/check_portepoch | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)