Hi guys, On Thu, May 03, 2018 at 12:58:55AM +0200, PiBa-NL wrote: > Hi Tim, > > Op 3-5-2018 om 0:26 schreef Tim Düsterhus: > > Pieter, > > > > Am 02.05.2018 um 23:54 schrieb PiBa-NL: > > > If commit message needs tweaking please feel free to do so :). > > > > > obviously not authoritative for this, but I noticed directly that the > > first line of your message is very long. It should generally be about 60 > > characters, otherwise it might get truncated. The following lines should > > be no more than 76 characters to avoid wrapping with the common terminal > > width of 80. > > > > Also after the severity and the component a colon should be used, not a > > comma. > > > > I suggest something like this for the first line. It is succinct and > > gives a good idea of what the bug might me. But please check whether I > > grasped the issue properly. > > > > BUG/MINOR: lua: Put tasks to sleep when waiting for data > > > > Best regards > > Tim Düsterhus > > Thanks, valid comments, the colons i should have noticed could have sworn i > did those correctly.. but no ;). i thought i carefully constructed the > commit message looking at a few others, while cramming in as much 'info' > about the change/issue as possible on a line (better than 'fixed #123' as > the whole subject&message as some committers do in a other project i > contribute to., but thats off-topic.). As for line lengths, it didn't even > cross my mind to look at that. (i also didn't lookup the contributing either > sorry, though it doesn't seem to specify specific line lengths.?.)
Tim is absolutely right. I'm picky about the formating of the subject line because it helps us a lot to do backports to stable branches. Just to give you an idea, here's a typical command run in 1.8 branch : haproxy-1.8$ ./scripts/git-show-backports -q -m -r tip/master HEAD | grep BUG/ abeaff2d - | BUG/MINOR: fd/threads: properly dereference fdcache as volatile 1ff91041 - | BUG/MINOR: fd/threads: properly lock the FD before adding it to the fd cache. 41ccb194 - | BUG/MEDIUM: threads: fix the double CAS implementation for ARMv7 f161d0f5 - | BUG/MINOR: pools/threads: don't ignore DEBUG_UAF on double-word CAS capable archs 6e8a41d8 - | BUG/MINOR: cli: Ensure all command outputs end with a LF 26fb5d84 - | BUG/MEDIUM: fd/threads: ensure the fdcache_mask always reflects the cache contents 1a1dd606 - | BUG/MINOR: h2: remove accidental debug code introduced with show_fd function b7426d15 - | BUG/MINOR: spoe: Register the variable to set when an error occurred (...) Here you can easily see that it's important to have the most info and context in the fewest words, and it's very convenient to quickly evict commits we're not interested in for a given version. Writing optimal commit messages is a difficult exercise first, but over time it becomes natural and you quickly figure that it also helps for other projects because "git log --oneline" and "git rebase -i" start to give you a lot of information that help do the best work. By the way I've just noticed while running this command that your subject now mistakenly contains "BUG/MINOR, BUG/MINOR" and I didn't notice it before pushing. Bah I should have looked closer, too late now :-) Regarding your fix, good job! You're right, it's a mistake. At first I thought your fix was wrong until I figured that the current code results in not updating an expired timeout, hence the 100% CPU loop. It's not surprising we ended up with this given that the scheduler semantics have changed in 1.8 and this part got broke after the change. Now merged, thank you! Willy
