Zandronum Chat on our Discord Server Get the latest version: 3.0
Source Code

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0003321Doomseeker[All Projects] Suggestionpublic2017-10-30 02:172017-11-02 13:39
ReporterWubTheCaptain 
Assigned To 
PrioritynormalSeveritytextReproducibilityalways
StatusclosedResolutiondenied 
PlatformOSOS Version
Product Version 
Target VersionFixed in Version 
Summary0003321: Adopt a 50/72 and one-change-per-commit (merges) style for Mercurial commit messages
DescriptionI have found Doomseeker's current Mercurial commit logs difficult to use, due to practice of development/habit. The current structure of a commit message is a freeform text limited by a hyphen (-). This is my personal pet peeve.

The full story of changes is not currently told, and an unaware person reverting (undoing) one commit may accidentally create a side effect of reverting another change because multiple related or unrelated changes may appear in one commit.

I propose a new format to be adopted: A title of max. 50 characters on first line (title), an empty line, then a message explaining the change (what was changed in detail and why it was necessary) with lines wrapped at 72 characters.

A commit should change one important thing. If an implementation (e.g. a new feature) takes multiple sensible changes, instead of one big commit it should be multiple commits and merged with a merge commit. This way, the changes are easier to review, revert individually (an individual commit) or as a whole (reverting the merge commit) and test for bugs.

Ideally, this style should be documented in a file such as CONTRIBUTING in version control / source distribution root and enforced.
Steps To Reproduce
$ hg --quiet clonehttps://bitbucket.org/Doomseeker/doomseeker/ [^]
$ cd doomseeker/
$ hg log --patch -r 2121:2ee1fdeb200a
...


Notice how multiline commits such as 2ee1fde only display the first line as heading. This is no different with default hg log style.
Additional InformationAn example of a "bad" commit style this suggestion is attempting to fix:https://bitbucket.org/Doomseeker/doomseeker/commits/2ee1fdeb200abe873671ee368226fb9e284f20fe [^]

Some of the following links are about Git, but the same principle applies to Mercurial and patch files exported from it. They have examples of "good" commit messages (including from kernel Linux).


  1. http://hgbook.red-bean.com/read/a-tour-of-mercurial-the-basics.html#id347874

  2. https://chris.beams.io/posts/git-commit/

  3. https://github.com/erlang/otp/wiki/Writing-good-commit-messages

  4. https://robots.thoughtbot.com/5-useful-tips-for-a-better-commit-message

  5. http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

  6. https://www.openembedded.org/wiki/Commit_Patch_Message_Guidelines

  7. https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html



Previous Mercurial commit logs need not be rewritten. I work mostly with Git, so excuse me for only one example from Mercurial.
Attached Filestxt file icon mercurial-merge-flow.txt [^] (1,463 bytes) 2017-10-30 05:07 [Show Content]

- Relationships

-  Notes
User avatar (0018678)
Blzut3 (administrator)
2017-10-30 04:28

If we were a bigger project with multiple simultaneously maintained stable branches these kinds of things might provide a benefit. But since we're not the only thing they do is make it more annoying to work on.

I don't know what tool you use to review changes, but I disagree on multiple commits being easier to review. They may change the same part of a file (unless you split your work into multiple commits after the fact) so the only sane way to get a complete picture of a multi-commit change is to flatten it. This is why Torr prefers single commit pull requests for Zandronum.

As for the static word wrapping. TortoiseHg only shows a line at 80 characters and doesn't show the character count so definitely can't be bothered there.

In the case of the commit you mentioned I was looking at how lintian reported the ldconfig problem for different prefixes and noticed that the prefixes weren't applying to the deb files. So I fixed that simultaneously while working the ldconfig trigger. Sure I could go and separate it into two commits, but it's not worth my time.

So with a notable exception for typo fixes (which if we had branches to cherry pick into I would agree should be a separate commit) almost every commit from me is done at the conclusion of whatever the first thing I was working on. I then just itemize what was done while doing that thing.

I'm leaving this open in case Zalewa wants to add anything or disagree, but this is a pretty strong rejection from me.
User avatar (0018680)
WubTheCaptain (developer)
2017-10-30 04:59

Quote from Blzut3
If we were a bigger project with multiple simultaneously maintained stable branches these kinds of things might provide a benefit.


I argue it brings benefits even on default branch (master branch in git).

There's also a consideration of Mercurial-Git bridges, where the first line (title) of the commit message is used for git-send-email(1) etc (sharing patch files).

Quote from Blzut3
But since we're not the only thing they do is make it more annoying to work on.


I thought so too first about git, but it has actually helped my workflow. For every feature I write: I keep a seperate feature branch, commit often, squash as necessary, then merge it (usually fast-forward in git so there's no merge commit).

If I want to work on another feature, I switch branches and continue there. Rebase to HEAD of master (tip of default) to avoid merge conflicts, for any unpublished changes.

Quote from Blzut3
I don't know what tool you use to review changes


Mercurial on GNU/Linux. hg log --patch

Quote from Blzut3
TortoiseHg only shows a line at 80 characters


I'm not sure at all on this, but it seems to be configurable (long summary boolean option):https://tortoisehg.readthedocs.io/en/latest/settings.html#workbench [^]

Quote from Blzut3
This is why Torr prefers single commit pull requests for Zandronum.


Right before I posted this, I tried hard to find that recent comment from Zandronum tracker where Torr had commented on Leonard's(?) patch. I can't find it anymore 😢, but I felt like it was a compliment for multiple commit pull requests.

Quote from Blzut3
In the case of the commit you mentioned


Nothing personal, it was just the most recent commit to make an example of.
User avatar (0018681)
WubTheCaptain (developer)
2017-10-30 05:09

Quote from Blzut3
I disagree on multiple commits being easier to review. They may change the same part of a file (unless you split your work into multiple commits after the fact) so the only sane way to get a complete picture of a multi-commit change is to flatten it.


I'm not convinced on this argument.

I uploaded mercurial-merge-flow.txt, which shows a merge commit. This merge commits shows the "flat" version with hg log --patch. To visualize the tree:

$ hg log --graph --style compact                                            
@    3[tip]:0,2   652c87bd2e18   2017-10-30 05:06 +0000   wubthecaptain
|\     merge commit
| |
| o  2   ad30688f99b3   2017-10-30 05:05 +0000   wubthecaptain
| |    added exclamation mark
| |
| o  1   2c93680f2e3c   2017-10-30 05:05 +0000   wubthecaptain
|/     added "world" to hello world
|
o  0   9423171a5403   2017-10-30 05:04 +0000   wubthecaptain
     hello world
User avatar (0018682)
WubTheCaptain (developer)
2017-10-30 05:43
edited on: 2017-10-30 05:52

I installed tortoisehg 4.3.1 to see how it works.

Quote from Blzut3
I don't know what tool you use to review changes, but I disagree on multiple commits being easier to review.


Oddly enough, tortoisehg doesn't show the diffs with merge commits in the main window until you click "Browse at Revision" right click context menu option.

Quote from Blzut3
TortoiseHg only shows a line at 80 characters and doesn't show the character count so definitely can't be bothered there.


Seems like the summary line length is configurable. Visual Editor and CLI Editor are also configurable to do the line wrapping for you. (Notepad++, vi, emacs, whatever you'd like.) I think?

Curious to know what workflow/tools Zalewa has/uses. Personally, I found tortoisehg to slow me down with unexpected behavior (consider I'm completely new to both Mercurial and tortoisehg, but familiar with git).

User avatar (0018714)
Zalewa (developer)
2017-11-01 15:09

Revert is not only overrated in arguments such as these, it is also a tool that degrades over time. It becomes gradually more difficult to perform depending only, and only on the age of the commit and not on any other factor. How often do you really revert anything? The initial change was made for a reason and when things don't work out the way they were supposed to, you fix the current code instead of going back to the old one. Reverting is like hitting a panic button, why would you do that? Plan your change beforehand and notice when code goes awry even before you make the first commit. If you need to partially revert some old change, it either can be done manually or through 'git revert bigchange; git reset HEAD^; git commit' (and in Hg - by using whatever clumsy equivalent it has). When your commit is old, you will drown in conflict mess regardless of how elegant this commit was. If you create multiple intermediary commits during work, all closely related to each other, squash them before pushing.

That said, revert is useful to temporarily disable significant part of debug code that you want to use for the period of the work, but it affects the runtime singificantly and you need to disable it to perform proper/final testing.

Furthermore, on the topic of what should the commit consist of, I'm all in for keeping commits as self-contained to singular change if possible. If I work on something and discover something else that also needs work, I prefer to do the side-job, commit it and then return to the original task. While this approach is in accordance to the unsung "best practice" standards, I mainly do it because it gets the "done" part of the job "saved" in the repository and it no longer distracts me when viewing diff on current changes. I also know I won't lose it if I revert any of the files in the working tree. I don't have a problem with occasional commit that changes more than "one" thing, provided that this is clearly stated in the commit message and that the changes are small in overall. I saw some commits on other projects where the commit message relates only to the 30% of the actual change, and the rest of the code is WTF.

Quote from "Wub"
Curious to know what workflow Zalewa has.

If I discover that that an unpushed commit still needs some work, I don't shy from amending or 'hg histediting' it. I know that Hg principle is to always keep the "real" history of your work, but I prefer to not keep garbage in the repo. Torr applies the same principle to the pull requests for Zandronum and my office also uses the same principle for all software. It works.

All changes that do not break the master/default, should be on the master/default, even if work is partial. Of course, if master/default software needs to be broken to do the change, then branching is reasonable. Moreover, if we ever need a "stable" or "patch" branch, we should branch off from master/default in a sensible place and then cherry-pick/graft commits from master onto it. We also use the same principles at the office where there are many developers working on the same repo. Stable/patch branches are normally not merged back into master. It works.

Quote from "Wub"
Curious to know what tools Zalewa uses.

For git, the command line is the only user interface that is user friendly. You just need to Google 'git lg1 lg2' and add it to your ~/.gitconfig and set mergetool to kdiff3. The rest works correctly out of the box.

For Hg, I didn't bother to learn the command line, but whenever I have to use it, I find it horribly clumsy and uncooperative with other GNU tools. It's amazing they actually got 'hg histedit' to work in the same way as 'git rebase -i'. One would expect that they would screw it up too. TortoiseHg, in this case, is the best UI.

Quote from "Wub"
Adopt a 50/72 [message] style

If anything, I'd go with 80/80. TortoiseHg still doesn't allow to wrap lines in commit message viewer, so if you go overboard, you'll get a horizontal scroller, which is annoying. Manual formatting of the lines to correct lengths prevents that. When I occasionally need to use 'hg log', the shell or cmd.exe will wrap long lines for me.

As for the current format of the commit message: I have already proposed to make the same change years ago, but at the moment Blzut3 stated that he wants to keep the current format because its easier to create a changelog from it. We have eventually concluded that changelog != commit messages. Changelog entries like "- typo fixed" or "- some code change irrelevant to the end-user" or "- fix stuff that was broken in previous commit" are not helpful to the end-user at all.

Personally, in all new projects I go with the "best practice" standards recommendation of subject/body separation and use self-imposed limit of 80/80. For Doomseeker, I came to accept that the current format is the accepted format and I also accept the occasional horizontal scroll bar that happens because of it. In any way, I have no strong feelings one way or the other on keeping the current format or changing it.
User avatar (0018716)
Zalewa (developer)
2017-11-01 18:05

AOSP, could you comment why you oppose?
User avatar (0018718)
WubTheCaptain (developer)
2017-11-02 01:32
edited on: 2017-11-02 01:35

Thanks for the detailed explanation, Zalewa. I agree with most of it, though I have few comments on things I disagree.

Quote from Zalewa
Stable/patch branches are normally not merged back into master.


In gitworkflows(7), this is the opposite: Maintenance patches (e.g. 1.1.1) are committed to maint branch, then merged to master (e.g. 1.2). In this workflow, commits are only cherry-picked if they were forgotten. The reason? Both maint and master can reference to the same commit by same hash then, so a cherry picked commit is not under two hashes in the repository.

See The gitworkflows(7) illustrated by K. Tateishi for a detailed illustration.

Quote from Zalewa
If anything, I'd go with 80/80.


There's 13 characters of left-padding in hg log (command line interface, I know). I know the terminal emulator wrapping, but like to stay loyal to 80x24 terminal size. hg -v log doesn't have the padding at all.

50/78 or 50/80 sounds reasonable to me. Consider the proposal of 50 for title/summary to be a suggested soft maximum: If it needs to be longer, it will be longer (at hard max of 80 characters).

https://medium.com/@preslavrachev/what-s-with-the-50-72-rule-8a906f61f09c [^]

Quote from Zalewa
Changelog entries like "- typo fixed" or "- some code change irrelevant to the end-user" or "- fix stuff that was broken in previous commit" are not helpful to the end-user at all.


Agreed, but for the developer it stills leaves something to desire to explain why the changes were made and locating those changes quickly.

A changelog is a seperate summary, hand-crafted from commit messages. (Except if it's a GNU project. Then ChangeLog would be the commit log dump, and NEWS news would be a summary.)

User avatar (0018719)
Zalewa (developer)
2017-11-02 10:35

I have viewed this presentation and while it does sound reasonable and I understand what kind of problems it allows to fix, I have doubts if we have these problems in the first place.

We don't do topic branches because most features can be implemented in a single commit. Server browser is not rocket science. I see no benefit in keeping a railway in the repo history instead of a monorail.

If feature is huge, like IRC client, it can be argued that an iterative implementation of this feature is already useful to the end-user. So, the first commit with a primitive implementation can already serve as usable (though, naturally, useless) IRC client. It can be argued that such implementations can be done on separate branches to avoid decreasing the overall quality of the software, but in the long run we don't release that often and once release happens, it happens in a time when we know there's no unfinished work.

There's usually no rush to produce a release, therefore there's rarely unfinished work that prevents releasing. Doomseeker does not grow rapidly (as you probably have already noticed). We work on weekends or during holidays or vacation.

Since we control all code and we keep it in a single repo (thank Crom), there's been little incentive to maintain absolute stability of API/ABI. We still prefer to maintain it, but when its stability gets in the way of resolving an issue cleanly then we prefer to break it.

Usually serious bugs are revealed right after the release, which means that bugfix releases, if any, happen usually after the feature release. There isn't much new code accumulated. We don't have problems with sneaking a new feature into a bugfix release.

I also wonder who starts working on a new repo right from many topic branches, when an application must grow on a common basis - single build system, single database access layer, single API layer, single UI framework - this stuff must be resolved before anyone can think of building independent features. And even then, during the initial development, there's still lots of flux that will affect the common root of all features.
User avatar (0018720)
Zalewa (developer)
2017-11-02 10:37

This ticket itself breaks the rule it preaches about because commit message formatting and commit contents are two separate things.
User avatar (0018723)
WubTheCaptain (developer)
2017-11-02 13:39

Quote from gitworkflows(7)
Many ideas apply in general, though the full workflow is rarely required for smaller projects with fewer people involved.


In a small (git-hosted) project, master and maint have been enough for me.

Quote from Zalewa
We don't do topic branches because most features can be implemented in a single commit.


I've kept topic branches local to my computer, semi-throw away.

Quote from Zalewa
This ticket itself breaks the rule it preaches about because commit message formatting and commit contents are two separate things.


I'm aware, it seemed easier to have a central discussion though. Mentioning gitworkflows(7) also was not the intent. I always regret these things later.

There's no strong signals in favor of a change as of now, so I'll close this ticket (though I'd still personally like to see a change). I also now better understand why things are currently done the way they are, and there's now a public record of them. (For the lack of a better communication channel for Doomseeker.) Thank you everyone.

Issue Community Support
This issue is already marked as resolved.
If you feel that is not the case, please reopen it and explain why.
Supporters: No one explicitly supports this issue yet.
Opponents: AOSP

- Issue History
Date Modified Username Field Change
2017-10-30 02:17 WubTheCaptain New Issue
2017-10-30 02:17 WubTheCaptain Severity minor => text
2017-10-30 02:19 WubTheCaptain Description Updated View Revisions
2017-10-30 02:37 WubTheCaptain Steps to Reproduce Updated View Revisions
2017-10-30 02:40 WubTheCaptain Steps to Reproduce Updated View Revisions
2017-10-30 02:43 WubTheCaptain Steps to Reproduce Updated View Revisions
2017-10-30 03:03 WubTheCaptain Steps to Reproduce Updated View Revisions
2017-10-30 04:28 Blzut3 Note Added: 0018678
2017-10-30 04:43 WubTheCaptain Assigned To => Zalewa
2017-10-30 04:43 WubTheCaptain Status new => feedback
2017-10-30 04:59 WubTheCaptain Note Added: 0018680
2017-10-30 04:59 WubTheCaptain Status feedback => assigned
2017-10-30 04:59 WubTheCaptain Status assigned => feedback
2017-10-30 05:07 WubTheCaptain Assigned To Zalewa =>
2017-10-30 05:07 WubTheCaptain Status feedback => new
2017-10-30 05:07 WubTheCaptain File Added: mercurial-merge-flow.txt
2017-10-30 05:09 WubTheCaptain Note Added: 0018681
2017-10-30 05:13 WubTheCaptain Status new => feedback
2017-10-30 05:43 WubTheCaptain Note Added: 0018682
2017-10-30 05:43 WubTheCaptain Status feedback => new
2017-10-30 05:52 WubTheCaptain Note Edited: 0018682 View Revisions
2017-11-01 15:09 Zalewa Note Added: 0018714
2017-11-01 18:05 Zalewa Note Added: 0018716
2017-11-02 01:08 WubTheCaptain Status new => acknowledged
2017-11-02 01:32 WubTheCaptain Note Added: 0018718
2017-11-02 01:32 WubTheCaptain Note Edited: 0018718 View Revisions
2017-11-02 01:35 WubTheCaptain Note Edited: 0018718 View Revisions
2017-11-02 10:35 Zalewa Note Added: 0018719
2017-11-02 10:37 Zalewa Note Added: 0018720
2017-11-02 13:39 WubTheCaptain Note Added: 0018723
2017-11-02 13:39 WubTheCaptain Status acknowledged => closed
2017-11-02 13:39 WubTheCaptain Resolution open => denied






Questions or other issues? Contact Us.

Links


Copyright © 2000 - 2021 MantisBT Team
Powered by Mantis Bugtracker