On Wed, Oct 25, 2017 at 2:17 PM, Vinnie Falco via Boost < boost@lists.boost.org> wrote:

On Wed, Oct 25, 2017 at 11:03 AM, Fernando Cacciola via Boost wrote:

...

but even though I'm logged with my Github account, it appears I don't have write-access to merge this PR.

I'm not a fan of using the GitHub web interface to merge pull requests because this needlessly creates a "merge commit" (usually with one side of the merge having just one commit) which interferes with a linear history (I prefer a linear history except for when there are two ongoing topical branches that are merged at some point).

Vinnie, have you tried using the "Rebase and Merge" option in github? It eliminates the merge, but it does make a new commit ID. If you're not working on a commit pipeline, this option will make things a bit easier. - Jim

On Wed, Oct 25, 2017 at 2:50 PM, Vinnie Falco via Boost < boost@lists.boost.org> wrote:

On Wed, Oct 25, 2017 at 11:26 AM, James E. King, III via Boost wrote:

...

Vinnie, have you tried using the "Rebase and Merge" option in github? It eliminates the merge, but it does make a new commit ID.

I've tried all of them and I don't like any of the automated means of merging pull requests. I don't merge code until I have brought it on to my machine and compiled / ran tests. Then once it is on my machine there is no need to use the GitHub interface because I can simply push it.

Doesn't this defeat the purpose of using Appveyor and Travis, or do you have additional tests that take too long for CI builds to be useful? If you keep some tests to yourself, the process of vetting new code changes from outside becomes unmaintainable. One of the beautiful things about having CI based unit testing is that it enables anyone to safely work on making changes to your project without you needing to lift a finger until all the builds pass. That's efficient. - Jim

On Wed, Oct 25, 2017 at 3:36 PM, Vinnie Falco via Boost < boost@lists.boost.org> wrote:

On Wed, Oct 25, 2017 at 12:12 PM, James E. King, III via Boost wrote:

...

Doesn't this defeat the purpose of using Appveyor and Travis, or do you have additional tests that take too long for CI builds to be useful? If you keep some tests to yourself, the process of vetting new code changes from outside becomes unmaintainable.

My motto is "Trust in God, but always run the tests locally." There are security considerations here, so I make sure that my workflow always brings external submissions through my own machine where I can inspect it with a debugger and my own two eyes. I am strongly opposed to any "farm to market" system since that could lead to being lazy.

Sounds like, "write once, debug everywhere". :) This development philosophy makes sense to me for a new project. It is not scalable into maintenance mode during which other folks are typically enabled. For example, if the CMT picked up a project which had no CI builds and no measurement of code coverage on the unit tests, and they don't have the secret formula that the maintainer was using to vet code changes, they have to reinvent the wheel. If however the original author sets up a robust CI environment with code coverage metrics, the CMT would be able to understand what expectations for quality submissions are at a minimum. This can be further improved with static code analysis (cppcheck, Coverity Scan) as well as -fsanitize type builds and valgrind-type builds. What I said previously is not intended as a replacement for a thorough code review. Someone should always inspect every line of code change for security, practicality, and performance related issues. If we all make as much of our baseline quality requirements as automated as possible, the maintainability of the entire codebase going into the future becomes easier. We are certainly aligned on providing quality product, no matter what the method. I think everyone would agree this is good for any project. - Jim

On Wed, Oct 25, 2017 at 4:36 PM, Vinnie Falco via Boost < boost@lists.boost.org> wrote:

On Wed, Oct 25, 2017 at 12:50 PM, James E. King, III via Boost wrote:

...

If however the original author sets up a robust CI environment with code coverage metrics, the CMT would be able to understand what expectations for quality submissions are at a minimum. This can be further improved with ... -fsanitize type builds and valgrind-type builds.

Beast has code coverage:

https://codecov.io/gh/boostorg/beast/branch/master

And per-commit sanitizer builds on CI:

https://travis-ci.org/vinniefalco/beast/jobs/292759959#L2812

And per-commit valgrind builds on CI:

https://travis-ci.org/vinniefalco/beast/jobs/292759958#L1399

On Travis the tests are run with gdb so in the event of a failure a stacktrace is generated.

Nice! It would be neat to have valgrind as a standard bjam build option, as a test wrapper. I haven't used codecov.io yet, but I would like to. I tried to use coveralls but it didn't generate any coverage for header-only libraries? Thanks, - Jim

Vinnie Falco wrote:

My motto is "Trust in God, but always run the tests locally."

What I do is click the "view command-line instructions" and apply only the "git pull" line to my "develop" branch, then run my local tests. The instructions as they appear make you create a separate local branch and merge that instead, but I find this to be of little value as I don't mind the merge commits.

On Wed, Oct 25, 2017 at 11:29 AM, Stefan Seefeld via Boost wrote:

That is no longer true (and hasn't for quite a while). Nowadays the web interface allows you to select different PR merge methods, including the traditional merge commit, a "squash and merge", or a "rebase and merge". It sounds like what you'd prefer is the latter, to make the history linear.

I do like the linear history where practical but that is not the only reason to avoid using the GitHub interface. Assembling the branch locally has an additional benefit. You can preview exactly how the branch will look after the merge. This is not possible with the GitHub interface. You also cannot sign commits with the GitHub interface or make adjustments. Everyone who has ever submitted a pull request always forgets to update CHANGELOG.md. Furthermore I have a policy in my repository that the tip of the master and develop branches that the top commit always sets an internal version number. Merging a pull request won't accomplish that.

On Wed, Oct 25, 2017 at 12:16 PM, Stefan Seefeld via Boost wrote:

Again, why not just use SHAs for that ? What additional information do you expect such a strictly defined version number to carry ? (And again, for anything higher level there are tags...)

You cannot compare two SHA values to determine which one is more recent. Beast by default builds a string of the form "Boost.Beast/<version>" which it uses for the User-Agent and Server fields in HTTP messages. This makes it easier to diagnose problems with remote servers and also to implement workarounds for specific versions. And it is easier for a user to respond with "version 123" than it is for them to say "the tip of my branch has digest 1e9fcbad61dd9e6dc1bc0be1b5882c2a9a8d1b4d".

On 25 October 2017 at 20:05, Daniel James wrote:

Or if you want to configure git to fetch all pull requests (including closed pull requests), do this in your repo:

git config --add remote.origin.fetch "+refs/pull/*/head:refs/remotes/origin/pr/*"

Sorry, that word wrapped, it should be one line.