Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/build/maintner: GerritCL.Subject doesn't include entire subject if it's split across multiple lines #30101

Closed
dmitshur opened this issue Feb 6, 2019 · 3 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Feb 6, 2019

According to git behavior, the subject of a commit message is separated from the body a blank line. Most commit message subjects are short and fit in one line (hopefully under 50 characters). In some cases, they end up being split into multiple lines.

As one example, CL 8204 has a subject that is split across two lines.

Git reports the subject using both lines:

tools $ git log -1 --format='%s' 1e216dc2e33abac72c076ca48ead2dcb476c2ddc
goimports: create a var to permit custom implementations of flag parsing and startup work.

maintner.GerritCL.Subject cuts it off short:

fmt.Println(cl.Subject())

// Output:
// goimports: create a var to permit custom implementations of flag parsing and

/cc @bradfitz

Here's a complete list of CLs in the Go maintner corpus where the current Subject method cuts off the subject:


WARNING: cl 9467: cl.Subject() != firstParagraph(cl.Commit.Msg):
"image/gif: don't encode local color tables if they're the same as the"
vs
"image/gif: don't encode local color tables if they're the same as the\nglobal color table."

WARNING: cl 1538: cl.Subject() != firstParagraph(cl.Commit.Msg):
"fmt: fix bug in scanning of hex strings"
vs
"fmt: fix bug in scanning of hex strings\nCouldn't handle a hex string terminated by anything\nother than spaces. Easy to fix."

WARNING: cl 8907: cl.Subject() != firstParagraph(cl.Commit.Msg):
"image/color: have Palette.Index honor alpha for closest match, not just"
vs
"image/color: have Palette.Index honor alpha for closest match, not just\nred, green and blue."

WARNING: cl 132780: cl.Subject() != firstParagraph(cl.Commit.Msg):
"Ignore / close / delete."
vs
"Ignore / close / delete.\nThis CL is superseded by:\nhttps://go-review.googlesource.com/c/go/+/132781"

WARNING: cl 9238: cl.Subject() != firstParagraph(cl.Commit.Msg):
"image/jpeg: have the LargeImageWithShortData test only allocate 64 MiB, not 604"
vs
"image/jpeg: have the LargeImageWithShortData test only allocate 64 MiB, not 604\nMiB."

WARNING: cl 2753: cl.Subject() != firstParagraph(cl.Commit.Msg):
"unicode: added Merge func for creating RangeTables that are the union of"
vs
"unicode: added Merge func for creating RangeTables that are the union of\nother range tables or just an optimized table from a non-compacted\nuser-created table."

WARNING: cl 2480: cl.Subject() != firstParagraph(cl.Commit.Msg):
"math/big: faster \"pure Go\" addition/subtraction for long vectors"
vs
"math/big: faster \"pure Go\" addition/subtraction for long vectors\n          (platforms w/o corresponding assembly kernels)"

WARNING: cl 1251: cl.Subject() != firstParagraph(cl.Commit.Msg):
"cmd/gc: logical operators should produce untyped bool for untyped"
vs
"cmd/gc: logical operators should produce untyped bool for untyped\noperands"

WARNING: cl 8841: cl.Subject() != firstParagraph(cl.Commit.Msg):
"image/jpeg: don't assume that an ensureNBits failure implies that we can"
vs
"image/jpeg: don't assume that an ensureNBits failure implies that we can\ncall unreadByteStuffedByte."

WARNING: cl 1575: cl.Subject() != firstParagraph(cl.Commit.Msg):
"The problem is :"
vs
"The problem is :\nOnce new comers who never installed golang on his pc (or laptop)\nThey need to know how to configure GOPATH and GOROOT."

WARNING: cl 14171: cl.Subject() != firstParagraph(cl.Commit.Msg):
"time: correct handling of \"5\" in layout string by Format() and"
vs
"time: correct handling of \"5\" in layout string by Format() and\nAppendFormat()"

WARNING: cl 151981: cl.Subject() != firstParagraph(cl.Commit.Msg):
"fs: add download attribute to HTML of dirList"
vs
"fs: add download attribute to HTML of dirList\nUpdate the dirList HTML output to include the download attribute in link tags."

WARNING: cl 1850: cl.Subject() != firstParagraph(cl.Commit.Msg):
"net/url: Fixed url parsing with invalid slashes"
vs
"net/url: Fixed url parsing with invalid slashes\n         after scheme"

WARNING: cl 9712: cl.Subject() != firstParagraph(cl.Commit.Msg):
"image/gif: allow encoding a single-frame image whose top-left corner"
vs
"image/gif: allow encoding a single-frame image whose top-left corner\nisn't (0, 0)."

WARNING: cl 9465: cl.Subject() != firstParagraph(cl.Commit.Msg):
"image/gif: check that individual frame's bounds are within the overall"
vs
"image/gif: check that individual frame's bounds are within the overall\nGIF's bounds."

WARNING: cl 3953: cl.Subject() != firstParagraph(cl.Commit.Msg):
"reflect: for struct tags, reject control chars (including tabs) in keys,"
vs
"reflect: for struct tags, reject control chars (including tabs) in keys,\nand empty keys. Also reject malformed (quoted) values."

WARNING: cl 5006: cl.Subject() != firstParagraph(cl.Commit.Msg):
"image: change Rectangle.Eq to return true for all empty rectangles, even"
vs
"image: change Rectangle.Eq to return true for all empty rectangles, even\nif their nominal Min and Max points differ."

WARNING: cl 8073: cl.Subject() != firstParagraph(cl.Commit.Msg):
"image/color: have YCbCr.RGBA work in 16-bit color, per the Color"
vs
"image/color: have YCbCr.RGBA work in 16-bit color, per the Color\ninterface."

WARNING: cl 1926: cl.Subject() != firstParagraph(cl.Commit.Msg):
"cmd/gc: Add more-descriptive error message for when the programmer uses"
vs
"cmd/gc: Add more-descriptive error message for when the programmer uses\na type as an instance of itself"

WARNING: cl 1767: cl.Subject() != firstParagraph(cl.Commit.Msg):
"syscall: syscall.Readlink doesn't handle junction on windows"
vs
"syscall: syscall.Readlink doesn't handle junction on windows\nFixes #9190"

WARNING: cl 2591: cl.Subject() != firstParagraph(cl.Commit.Msg):
"spec: extend type omission rules for composite literal element values"
vs
"spec: extend type omission rules for composite literal element values\n      to map element keys"

WARNING: cl 5251: cl.Subject() != firstParagraph(cl.Commit.Msg):
"image/jpeg: support 16-bit quantization tables and Extended Sequential"
vs
"image/jpeg: support 16-bit quantization tables and Extended Sequential\nframes."

WARNING: cl 9480: cl.Subject() != firstParagraph(cl.Commit.Msg):
"debug/dwarf: Add function EntryLocation(entry, cfa) in location.go which"
vs
"debug/dwarf: Add function EntryLocation(entry, cfa) in location.go which\nreturns the address of an object, given its DWARF entry.  If the second\nparameter is non-nil, it contains the address of the stack frame the\nobject occurs in.  This is used when the object location is given as an\noffset relative to the stack frame address."

WARNING: cl 9270: cl.Subject() != firstParagraph(cl.Commit.Msg):
"image/png: don't silently swallow io.ReadFull's io.EOF error when it"
vs
"image/png: don't silently swallow io.ReadFull's io.EOF error when it\nlands exactly on an IDAT row boundary."

WARNING: cl 2267: cl.Subject() != firstParagraph(cl.Commit.Msg):
"encoding/json: encode large or unbounded structures to JSON"
vs
"encoding/json: encode large or unbounded structures to JSON\n- json.Encoder periodically flushes to its io.Writer\n- support JSON encoding of channel types"

WARNING: cl 6393: cl.Subject() != firstParagraph(cl.Commit.Msg):
"image/jpeg: distinguish between FormatError and UnsupportedError when"
vs
"image/jpeg: distinguish between FormatError and UnsupportedError when\nencountering unknown markers."

WARNING: cl 6421: cl.Subject() != firstParagraph(cl.Commit.Msg):
"image/jpeg: when following component selectors, only consider valid"
vs
"image/jpeg: when following component selectors, only consider valid\ncomponents."

WARNING: cl 9482: cl.Subject() != firstParagraph(cl.Commit.Msg):
"debug/dwarf: add SliceType, StringType, InterfaceType, MapType,"
vs
"debug/dwarf: add SliceType, StringType, InterfaceType, MapType,\nChanType.  These implement the dwarf.Type interface for the\ncorresponding Go types."

WARNING: cl 9463: cl.Subject() != firstParagraph(cl.Commit.Msg):
"draw: disable the image.Rectangle DstMask fast path until Go 1.5 is"
vs
"draw: disable the image.Rectangle DstMask fast path until Go 1.5 is\nreleased."

WARNING: cl 10072: cl.Subject() != firstParagraph(cl.Commit.Msg):
"vp8: limit all other partitions to 1<<24 bytes in size, not just N-1 of"
vs
"vp8: limit all other partitions to 1<<24 bytes in size, not just N-1 of\nthem."

WARNING: cl 7612: cl.Subject() != firstParagraph(cl.Commit.Msg):
"draw: make example_test more example-like, by being outside \"package"
vs
"draw: make example_test more example-like, by being outside \"package\ndraw\"."

WARNING: cl 4210: cl.Subject() != firstParagraph(cl.Commit.Msg):
"draw: new package, a superset of the standard library's image/draw"
vs
"draw: new package, a superset of the standard library's image/draw\npackage, including the ability to scale an image."

WARNING: cl 6914: cl.Subject() != firstParagraph(cl.Commit.Msg):
"math/f32, math/f64: add a comment that \"Aff\" stands for affine"
vs
"math/f32, math/f64: add a comment that \"Aff\" stands for affine\ntransformation."

WARNING: cl 9277: cl.Subject() != firstParagraph(cl.Commit.Msg):
"cmd/webp-manual-test: add a \"build ignore\" tag, so that \"go install"
vs
"cmd/webp-manual-test: add a \"build ignore\" tag, so that \"go install\ngolang.org/x/image/...\" doesn't install the manual testing program."

WARNING: cl 3051: cl.Subject() != firstParagraph(cl.Commit.Msg):
"webdav: make the memFS (not the memFSNode) the canonical source of a"
vs
"webdav: make the memFS (not the memFSNode) the canonical source of a\nfile name."

WARNING: cl 2535: cl.Subject() != firstParagraph(cl.Commit.Msg):
"webdav: implement parseDepth; restrict LockDetails' depth to only zero"
vs
"webdav: implement parseDepth; restrict LockDetails' depth to only zero\nor infinity."

WARNING: cl 1354: cl.Subject() != firstParagraph(cl.Commit.Msg):
"icmp: add support for windows"
vs
"icmp: add support for windows\nFixes #9253"

WARNING: cl 10394: cl.Subject() != firstParagraph(cl.Commit.Msg):
"webdav: Return HTTP 404 for PROPFIND/PROPPATCH requests on an inexistent"
vs
"webdav: Return HTTP 404 for PROPFIND/PROPPATCH requests on an inexistent\nwebdav.Dir resource."

WARNING: cl 3567: cl.Subject() != firstParagraph(cl.Commit.Msg):
"webdav: set the Lock-Token HTTP header when LOCKing existing files, not"
vs
"webdav: set the Lock-Token HTTP header when LOCKing existing files, not\njust new files."

WARNING: cl 10395: cl.Subject() != firstParagraph(cl.Commit.Msg):
"webdav: Simplify handling of Etag and Content-Type headers for GET, HEAD,"
vs
"webdav: Simplify handling of Etag and Content-Type headers for GET, HEAD,\nPOST and PUT requests."

WARNING: cl 4341: cl.Subject() != firstParagraph(cl.Commit.Msg):
"webdav: factor out a moveFiles function, and make the tests call that"
vs
"webdav: factor out a moveFiles function, and make the tests call that\ninstead of FileSystem.Rename directly."

WARNING: cl 3835: cl.Subject() != firstParagraph(cl.Commit.Msg):
"webdav: return 500 Internal Server Error and not 404 Not Found or 403"
vs
"webdav: return 500 Internal Server Error and not 404 Not Found or 403\nForbidden during copyFiles."

WARNING: cl 2302: cl.Subject() != firstParagraph(cl.Commit.Msg):
"webdav: add memFS, an initial implementation of an in-memory"
vs
"webdav: add memFS, an initial implementation of an in-memory\nfilesystem."

WARNING: cl 45931: cl.Subject() != firstParagraph(cl.Commit.Msg):
"DO NOT REVIEW"
vs
"DO NOT REVIEW\nDO NOT SUBMIT"

WARNING: cl 7531: cl.Subject() != firstParagraph(cl.Commit.Msg):
"goimports: add imports.Init to permit custom implementations of flag parsing and"
vs
"goimports: add imports.Init to permit custom implementations of flag parsing and\nstartup work."

WARNING: cl 8204: cl.Subject() != firstParagraph(cl.Commit.Msg):
"goimports: create a var to permit custom implementations of flag parsing and"
vs
"goimports: create a var to permit custom implementations of flag parsing and\nstartup work."

WARNING: cl 1680: cl.Subject() != firstParagraph(cl.Commit.Msg):
"build/androidtest.bash: script to run all tests in a repository on an"
vs
"build/androidtest.bash: script to run all tests in a repository on an\nandroid device."

WARNING: cl 1851: cl.Subject() != firstParagraph(cl.Commit.Msg):
"Fixed x11/egl with nvidia binary-only driver."
vs
"Fixed x11/egl with nvidia binary-only driver.\nStarted breaking the render commands out of the event pump.\nNeeds feedback."

WARNING: cl 16021: cl.Subject() != firstParagraph(cl.Commit.Msg):
"exp/sprite/portable: revert for /x/image/draw api"
vs
"exp/sprite/portable: revert for /x/image/draw api\nFix broken build that was broke by \"fix[ing] broken build\""

WARNING: cl 92737: cl.Subject() != firstParagraph(cl.Commit.Msg):
"Adding a unique error when no authentication method has been passed in"
vs
"Adding a unique error when no authentication method has been passed in\nyet."
@dmitshur dmitshur added Builders x/build issues (builders, bots, dashboards) NeedsFix The path to resolution is known, but the work has not been done. labels Feb 6, 2019
@dmitshur dmitshur added this to the Unreleased milestone Feb 6, 2019
@dmitshur dmitshur self-assigned this Feb 6, 2019
@gopherbot
Copy link

Change https://golang.org/cl/161222 mentions this issue: maintner: support multi-line commit message subjects

@dmitshur
Copy link
Contributor Author

dmitshur commented Feb 6, 2019

According to git behavior, [...]

I could've been more convincing and brief by referencing git documentation, rather than trying to demonstrate behavior. It's documented at https://git-scm.com/docs/git-commit#_discussion:

Though not required, it’s a good idea to begin the commit message with a single short (less than 50 character) line summarizing the change, followed by a blank line and then a more thorough description. The text up to the first blank line in a commit message is treated as the commit title, and that title is used throughout Git.

@gopherbot
Copy link

Change https://golang.org/cl/161797 mentions this issue: maintner: handle multi-line subject edge-case

gopherbot pushed a commit to golang/build that referenced this issue Feb 11, 2019
This is a followup to CL 161222, where I forgot to handle the case
where a commit message has a multi-line subject and no body.

Updates golang/go#30101

Change-Id: I01222e8cf783bc3b0631a332cf379717aa85cebc
Reviewed-on: https://go-review.googlesource.com/c/161797
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@golang golang locked and limited conversation to collaborators Feb 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

2 participants