Squash your commits

Re:Squash your commits by chris_wot on Saturday April 02, 2016 @02:57AM (#11410550)

I'm curious, what do you think of the commit history of the last few years of LibreOffice?

https://cgit.freedesktop.org/libreoffice/core/log/

(Don't look at the OpenOffice.org years, they literally took a whole bunch if development work from SVN branches and then merged them in as a single commit and put in single line descriptions with internal tracking numbers and odd project management codes... utter disaster! And of course the branches are now all lost...)

[ Parent | on HN ]

Re:Squash your commits by MaulingMonkey on Saturday April 02, 2016 @04:16AM (#11410719)

How about I just look at the latest ~33h directly on that page ;)

I like the scope of a lot of those commits, although a few are still chunkier looking than I'd like - take that with a grain of salt, though, as I don't have a good enough feel for the codebase to reasonably estimate how much more they could be chunked up. Pretty much everything has a review link, which is nice. I'd expect more back and forth in the comments, but perhaps that's handled out-of-band.

I have lots of nitpicks with the actual changelist descriptions where I'd want things to improve. "Clean up" could mean just about anything - I must go to diff (and expand the context) to understand e.g. https://cgit.freedesktop.org/libreoffice/core/commit/?id=945... properly. I'd be inclined to instead write:

officeipcthread: Cleanup RequestHandler::Enable: Early bail, remove aDummy (just use aUserInstallPath directly), move declarations.

Now I know scope, and the types of changes (refactoring worth reviewing if looking for breakage, not just ignorable whitespace / comment changes.)

No gripes with the overall style on this one, although interacting with a security component, I'd want multiple reviewers: https://cgit.freedesktop.org/libreoffice/core/commit/?id=2a9... .

I'd lean towards linking a screenshot of at least the new version of UI when the file being modified "isn't human readable" (read: is modified with something other than a text editor, even if I can totally read it) which would apply to e.g. https://cgit.freedesktop.org/libreoffice/core/commit/?id=72c... .

EDIT: Formatting, + rationale RE: changelist description.

[ Parent | on HN ]

Re:Squash your commits by chris_wot on Saturday April 02, 2016 @06:56AM (#11411062)

Ah... sberg's one commit message that could be more descriptive :-) That NSS commit has two very experienced reviewers, but fair point! wrt screenshot linking, I guess a link to the LibreOffice bugzilla would be nice.

Thanks for the info, that's really quite insightful. Appreciate you taking the time to give it!

[ Parent | on HN ]

Re:Squash your commits by MaulingMonkey on Saturday April 02, 2016 @04:18PM (#11413205)

> That NSS commit has two very experienced reviewers,

I must be blind - somehow I only saw one the first time around, but they're clearly both there.

> Thanks for the info, that's really quite insightful. Appreciate you taking the time to give it!

I'll take your word for it ;). And no problem.

[ Parent | on HN ]

Re:Squash your commits by chris_wot on Saturday April 02, 2016 @04:21PM (#11413219)

I'd love to see a system that can show reviewers as metadata within git itself.

[ Parent | on HN ]