CChainState -> Chainstate #24513

pull jamesob wants to merge 1 commits into bitcoin:master from jamesob:2022-03-chainstate-rename changing 33 files +154 −154
  1. jamesob commented at 5:56 pm on March 9, 2022: member

    Alright alright alright, I know: we hate refactors. We especially hate cosmetic refactors.

    Nobody knows better than I that changing broad swaths of code out from under our already-abused collaborators, only to send a cascade of rebase bankruptcies, is annoying at best and sadistic at worst. And for a rename! The indignation!

    But just for a second, imagine yourself. Programming bitcoin/bitcoin, on a sandy beach beneath a lapis lazuli sky. You go to type the name of what is probably the most commonly used data structure in the codebase, and you only hit shift once.

    What could you do in such a world? You could do anything. The only limit is yourself.

    So maybe you like the idea of this patch but really don’t want to deal with rebasing. You’re in luck!

    Here’re the commands that will bail you out of rebase bankruptcy:

    0git rebase -i $(git merge-base HEAD master) \
1  -x 'sed -i "s/CChainState/Chainstate/g" $(git ls-files | grep -E ".*\.(py|cpp|h)$") && git commit --amend --no-edit'
2# <commit changed?>
3git add -u && git rebase --continue

    Anyway I’m not sure how serious I am about this, but I figured it was worth proposing. I have decided I am very serious about this.

    Maybe we can have nice things every once in a while?

  2. jamesob force-pushed on Mar 9, 2022
  3. jamesob force-pushed on Mar 9, 2022
  4. jamesob force-pushed on Mar 9, 2022
  5. jamesob force-pushed on Mar 9, 2022
  6. jonatack commented at 6:24 pm on March 9, 2022: contributor
    Ok, where is @jamesob and what have you done to him.
  7. DrahtBot added the label Block storage on Mar 9, 2022
  8. DrahtBot added the label Mempool on Mar 9, 2022
  9. DrahtBot added the label Mining on Mar 9, 2022
  10. DrahtBot added the label RPC/REST/ZMQ on Mar 9, 2022
  11. DrahtBot added the label Utils/log/libs on Mar 9, 2022
  12. DrahtBot added the label UTXO Db and Indexes on Mar 9, 2022
  13. DrahtBot added the label Validation on Mar 9, 2022
  14. DrahtBot commented at 6:45 am on March 11, 2022: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25977 (refactor: Replace std::optional<bilingual_str> with util::Result by ryanofsky)
    • #25971 (refactor: Use std::string for thread and index names by stickies-v)
    • #25740 (assumeutxo: background validation completion by jamesob)
    • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
    • #25667 (assumeutxo: snapshot initialization by jamesob)
    • #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)
    • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
    • #23599 ([WIP] DRAFT NOMERGE Tidy up RPCTxSerializationFlags by MarcoFalke)
    • #23319 (rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  15. glozow commented at 5:18 pm on March 11, 2022: member
    Concept ACK, but need song-and-dance interpretation of the PR description
  16. jonatack commented at 5:16 pm on March 13, 2022: contributor
    I’m sort of tepid on whether this is worth doing, but supposing it were, I think I’d prefer ChainState over Chainstate. The reason/question is, are expressions/words like “chain state”, “fee rate”, and “block hash” each one word or two? For me, and maybe for people not closely familiar since years with Bitcoin development, they are two words–at least originally, which I guess is why there are classes in the codebase like CChainState and CFeeRate. I usually grep case-indifferently anyway, so this wouldn’t change anything (if it doesn’t break anything). Just one weakly held light opinion.
  17. jb55 commented at 11:24 pm on March 13, 2022: contributor
    ACK 5977234384da1f52475fc8165c52830750bf7d2a
  18. michaelfolkson commented at 2:12 pm on March 15, 2022: contributor

    I was reading through some of @jonatack’s notes and found this old quote from Greg Maxwell:

    I reiterate: the background level of refactors, style changes, cleanups, and other related activity is actively repelling multiple long term contributors, myself included. I beg: give it a bit of a rest, we have so many other things that are crying for our attention and our resolve. We should try to find some initiatives that we all feel more excited about, success with them would make it easier to work on other things… but right now a big style change is just not a unifying effort.

    I don’t have a strong enough opinion to NACK this PR but the above seems to add some historical context behind previous attempts to do renames like this which may be helpful for other reviewers.

  19. ajtowns commented at 2:22 pm on March 15, 2022: contributor
    Introducing “Stockholm syndrome: the PR” ? We have ChainstateManager already, so while I agree with @jonatack that it’s weird treating “chainstate” as one word, at least this is slightly reducing an inconsistency. Don’t really see the benefits outweighing the costs here.
  20. ryanofsky approved
  21. ryanofsky commented at 2:28 pm on March 15, 2022: contributor

    Code review ACK 5977234384da1f52475fc8165c52830750bf7d2a

    https://www.youtube.com/watch?v=j7OHG7tHrNM

  22. ryanofsky commented at 2:31 pm on March 15, 2022: contributor
    Also “Chainstate” looks better than “ChainState”, establishes it as a noun, and is confusing to no one.
  23. jb55 commented at 5:48 pm on March 15, 2022: contributor

    On Tue, Mar 15, 2022 at 07:12:29AM -0700, Michael Folkson wrote:

    I was reading through some of @jonatack’s notes and found this old quote from Greg Maxwell:

    I reiterate: the background level of refactors, style changes, cleanups, and other related activity is actively repelling multiple long term contributors, myself included. I beg: give it a bit of a rest, we have so many other things that are crying for our attention and our resolve. We should try to find some initiatives that we all feel more excited about, success with them would make it easier to work on other things… but right now a big style change is just not a unifying effort.

    I don’t have a strong enough opinion to NACK this PR but the above seems to add some historical context behind previous attempts to do renames like this which may be helpful for other reviewers.

    I ACK’d because I think this irrational fear of refactors is holding bitcoin’s code quality back. Not saying this PR is the best example, I agree that style changes aren’t as productive, but trivial refactors and improvements that improve code quality in non-consensus critical code shouldn’t take years to merge.

    I get that we are being conservative, but I think the bigger factor to the brain drain is the fact that people put in effort to do small code quality improvements just to wait years to even see it merged.

    Now people don’t even bother to improve code quality because of this weird meme that refactors don’t matter, and only new features matter. Perhaps I’m off base here since I’m not a fulltime core dev but that’s just been my perspective watching core dev over the past decade.

  24. MarcoFalke commented at 6:00 pm on March 15, 2022: member

    (Not a comment on the changes here, just my approach to renames)

    Personally I try to bundle such rename refactors with other changes that make sense on their own. When there is a large or otherwise breaking code change, scripted-diff renames can be appended at minimal extra cost. Sometimes they are even the better choice, to create compile failures for silent merge conflicts that may otherwise sneak in unnoticed. They wouldn’t increase the number of conflicts (as the code is touched anyway for other reasons) and the review spent on the scripted-diff is negligible compared to the review spent on the other changes.

  25. promag commented at 6:22 pm on March 15, 2022: member

    Code review ACK 5977234384da1f52475fc8165c52830750bf7d2a.

    Feel free to rename all remaining C-prefixed classes 💣

  26. jamesob commented at 7:52 pm on March 15, 2022: member

    found this old quote from Greg Maxwell

    There’s a major difference in review requirements/risk/annoyance between “I shuffled a bunch of code around and changed some software engineeringy design patterns/C++ features because it just feels cleaner” vs. “here’s a scripted-diff that is trivial to review.” The former is unadvisable, and I wouldn’t propose something in that vein unless it was a part of significant feature work which had been motivated with a decent prototype of the end-state.

    I well remember the sentiment that Greg is talking about in the quote; there was good reason for what he said, both at the time and now. But the effort required to review this struck me as negligible, and I can’t help but indulge some contrarianism now and again.

    I won’t be heartbroken if we close this thing but man do I hate typing CChainState.

  27. DrahtBot added the label Needs rebase on Mar 16, 2022
  28. jamesob force-pushed on Mar 25, 2022
  29. DrahtBot removed the label Needs rebase on Mar 25, 2022
  30. DrahtBot added the label Needs rebase on Mar 28, 2022
  31. laanwj removed the label UTXO Db and Indexes on Apr 22, 2022
  32. laanwj removed the label RPC/REST/ZMQ on Apr 22, 2022
  33. laanwj removed the label Mining on Apr 22, 2022
  34. laanwj removed the label Validation on Apr 22, 2022
  35. laanwj removed the label Mempool on Apr 22, 2022
  36. laanwj removed the label Block storage on Apr 22, 2022
  37. laanwj removed the label Utils/log/libs on Apr 22, 2022
  38. laanwj added the label Refactoring on Apr 22, 2022
  39. jamesob force-pushed on May 24, 2022
  40. DrahtBot removed the label Needs rebase on May 24, 2022
  41. jamesob force-pushed on May 24, 2022
  42. DrahtBot added the label Needs rebase on Jun 7, 2022
  43. jamesob force-pushed on Jun 23, 2022
  44. DrahtBot removed the label Needs rebase on Jun 23, 2022
  45. ryanofsky approved
  46. ryanofsky commented at 7:14 pm on June 28, 2022: contributor
    Code review ACK e2c9115af2db4b5aa1f1d2333946fb22912e500d. Just rebased since last review
  47. DrahtBot added the label Needs rebase on Jun 29, 2022
  48. jamesob force-pushed on Sep 7, 2022
  49. jamesob commented at 7:52 pm on September 7, 2022: member
    image
  50. DrahtBot removed the label Needs rebase on Sep 7, 2022
  51. MarcoFalke commented at 6:09 am on September 8, 2022: member

    I sometimes see myself running into compile errors when typing ChainStateManager, or CChainstate, so I think it makes sense to somehow improve the consistency here. Also, the merge conflicts right now are currently as minimal as they can get.

    Any reason why there are two commits and not a single one? A sed ... $(git grep -l CChainState ':(exclude)doc/release-notes*') should be sufficient.

  52. scripted-diff: rename CChainState -> Chainstate 
    -BEGIN VERIFY SCRIPT-
sed -i 's/CChainState/Chainstate/g' $(git grep -l CChainState ':(exclude)doc/release-notes*')
-END VERIFY SCRIPT-

Co-authored-by: MacroFake <falke.marco@gmail.com>
    00eeb31c76
  53. jamesob force-pushed on Sep 9, 2022
  54. jamesob commented at 3:50 pm on September 9, 2022: member
    @MarcoFalke thanks! Pushed.
  55. MarcoFalke commented at 8:39 am on September 10, 2022: member
    cr ACK 00eeb31c7660e2c28f189f77a6905dee946ef408
  56. hebasto commented at 10:47 am on September 10, 2022: member
    Concept ACK.
  57. hebasto approved
  58. hebasto commented at 8:33 pm on September 10, 2022: member
    ACK 00eeb31c7660e2c28f189f77a6905dee946ef408
  59. glozow commented at 9:20 am on September 12, 2022: member
    ACK 00eeb31c7660e2c28f189f77a6905dee946ef408, thanks for being the one to propose this
  60. w0xlt approved
  62. jamesob commented at 8:39 pm on September 12, 2022: member
    RFM?
  63. glozow merged this on Sep 13, 2022
  64. glozow closed this on Sep 13, 2022
  65. sidhujag referenced this in commit be7ad884ef on Sep 13, 2022
  66. Fabcien referenced this in commit 54112d1894 on Feb 15, 2023
  67. bitcoin locked this on Sep 13, 2023


jamesob jonatack DrahtBot glozow jb55 michaelfolkson ajtowns ryanofsky MarcoFalke promag hebasto w0xlt

Labels
Refactoring

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me