Better process for merging code into the upstream repository

[vinniefalco]

Currently, there seems to be no process for merging code into the upstream. Minimal discussion of proposed changes. As a result, frivolous commits are being merged. For example, a series of three trivial commits changing Contributing.md were placed where one would have sufficed: https://github.com/stellar/stellard/commit/b09197122c6ead640ea9d86a749f5fbf1ca46fd1

Suggest that only one or two people have access to push to the main stellard repository, and all changes go through pull requests with a well defined process. Had the changes to Contributing.md gone through a review process, there would not be the need to correct mistakes in two subsequent commits.

[graydon]

Agreed, we should be following a more structured review process. Currently integration of PRs is performed by a testing robot ( @latobarita ) but all team members retain direct push access, and trivial changes are still entering the revision history. Additionally we are not yet using github issues as effectively as we should, for documenting our activities or making work accessible to non-SDF contributors.

As far as this being a bug with an actionable fix, it really comes down to documentation and followthrough on our part. Ironically, the commit you point to is the documentation part: the contribution process is now outlined -- I'll amend that document with a note about @latobarita which is only active at present on this repository -- and we simply need to follow that procedure in the future. The commit that added that documentation did not; but that is now past and I assume you don't want us rewriting history to exclude it. So while I agree with your assessment of what we need to do, I'm not sure there's much more we can do except "try to follow procedure in the future" as far as resolving this bug.

Do you want to keep it open as a reminder, and decide at some point in the future whether we're meeting reasonable standards of process?

[vinniefalco]

I think, if there is a clear explanation of how the process will avoid what happened here, "three commits in a row for the same file", then its safe to close. Maybe the author forgot or is unaware of how to rebase and/or squash?

[bekkibolthouse]

Hi @vinniefalco thank you for the feedback. Very fair points! I will follow the suggestions/process you and @graydon suggest. This is a case of inexperience, but very good intentions.

[vinniefalco]

One possible rule of thumb is never to merge your own code into the upstream. But its up to the organization to find a workflow that suits the individual members. I'm closing this since it looks like its addressed. Thanks

[vinniefalco]

Apologies for reopening but there still seems to be a lack of process. Recently we've seen: force ledger close (https://github.com/stellar/stellard/commit/457786f5a082ee6cd1340c3fd067c10b5a291089) followed later by try another spot for force close (https://github.com/stellar/stellard/commit/529e90d1041b40dcbd90e0b04e46d635ff1fc84b). There is no benefit to merging a broken commit to master only to fix it in a subsequent commit. This would have been caught in a review process with the typical suggestion to squash them down.

There's also also the troubling problems of quality in commit messages. What is "force close?" It sounds like something having to do with ledger closing. While this is not my project, I feel that changing something core to the business logic like ledger closing (or anything consensus related) is deserving of more detail both in the change in behavior introduced by the commit and the rationale for doing so.

Especially perplexing are commit messages containing one word utterances such as grown (https://github.com/stellar/stellard/commit/2b0ec0895952e2fcea365ab5cdf0c16b81342add) or ug (https://github.com/stellar/stellard/commit/dad4b09e1b0363ae33a32c6a9a8a2fbd05e7abb5). While this informal style may be acceptable for an individual experimenting with their own project with a limited distribution it seems entirely inappropriate for financial software. A more formal review process, with more time required for changes to sit in pull requests for viewing, would go a long way to fixing this.

[graydon]

I appreciate your concerns here -- especially concerning the content of commit messages, I'll talk to Jed about trying to improve those -- but the commit sequence in question should be understood for what it is/was: an urgent set of attempts to restore the network to service during a complete (and persistent) system outage last saturday.

Time for a third party (few of whom were at work) to review the code, much less time to even have a clear sense of whether it was the "right change" to get the system back online and in service vs. another wrong one, was lacking at the time. Perhaps such changes should be made to a throwaway / hot-fix branch, rather than polluting the master history. I'll suggest this if it'd address some of your concerns. We can also rebase (history-edit) these changes out or squash them if you don't mind seeing master move in a non-fast-forward direction.

More generally: we have been trying to move away from firefighting to actually-constructive work for some time (including merging fixes in from rippled, many look promising!) but the fact is that the existing code has serious stability, convergence, performance and data-loss problems, and we have been spending most of our efforts lately trying to keep it behaving well in production.

[vinniefalco]

@graydon I think using a separate branch for production, with the understanding that history may be rewritten, makes perfect sense here.