refactor: Cleanup clientversion.cpp #18616

This PR:

• removes unused macros and duplicated code
• renames macros in a way, that makes the code self-descriptive.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #18902 (Bugfix: Only use git for build info if the repository is actually the right one by luke-jr)

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.

Rebased 3335ab4b857c96999b390f92a9fe62a42d827f82 -> 098d26d0cc00cb1bca005a0cd289f4cae753c67b (pr18616.01 -> pr18616.02) due to the conflict with #18556.

@dongcarl Mind looking into this?

Concept ACK

ACK 098d26d0cc00cb1bca005a0cd289f4cae753c67b

Built, run and tested on macOS Catalina 10.15.4

068bcc017c695a450e132f060b936800954637ab

Perhaps let's separate the pure-rename changes into another commit from the other changes? (I think the changes to share/genbuild.sh are the only pure-rename ones). Also, is the DO_STRINGIZE a mistake from before? Why is it not needed anymore?

098d26d0cc00cb1bca005a0cd289f4cae753c67b

Not quite sure how this mechanism works here, and wasn't able to find a reference to it in the git-achive(1) man page, could you point me in the right direction?

@dongcarl

098d26d

Not quite sure how this mechanism works here, and wasn't able to find a reference to it in the git-achive(1) man page, could you point me in the right direction?

a) we set the export-subst attribute for src/clientversion.cpp in the .gitattributes file: https://github.com/bitcoin/bitcoin/blob/844d2070a2c0106bb7a54be5cad7d4da4d9cd55e/.gitattributes#L1

From the git docs:

If the attribute export-subst is set for a file then Git will expand several placeholders when adding this file to an archive. The expansion depends on the availability of a commit ID, i.e., if git archive has been given a tree instead of a commit or a tag then no replacement will be done. The placeholders are the same as those for the option --pretty=format: of git log, except that they need to be wrapped like this: $Format:PLACEHOLDERS$ in the file. E.g. the string $Format:%H$ will be replaced by the commit hash.

b) $Format:%n#define GIT_COMMIT_ID "%H"$ has two placeholders:

• %n, which is expanded to a newline that makes a new uncommented LOC just after the comment that states the same thing :)

//! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.

• %H, which is expanded to a commit hash

@dongcarl

Also, is the DO_STRINGIZE a mistake from before? Why is it not needed anymore?

It is still required: https://github.com/bitcoin/bitcoin/blob/844d2070a2c0106bb7a54be5cad7d4da4d9cd55e/src/clientversion.h#L17-L22

Updated 098d26d0cc00cb1bca005a0cd289f4cae753c67b -> 09ee167a6dd7b2d4ade828affc95a679acfaf6c6 (pr18616.02 -> pr18616.05, diff):

• the pure-rename changes to share/genbuild.sh are separated into their own commit (see @dongcarl's comment) @dongcarl Thank you for reviewing. All your comments have been addressed.

Code Review ACK 09ee167a6dd7b2d4ade828affc95a679acfaf6c6

Nit: personally, something like this in clientversion.cpp made it a bit more readable for me without sacrificing the DRY-ness:

#ifdef BUILD_GIT_TAG
    #define BUILD_DESC BUILD_GIT_TAG
#else
    #define BUILD_BASE_DESC "v" STRINGIZE(CLIENT_VERSION_MAJOR) "." STRINGIZE(CLIENT_VERSION_MINOR) \
                            "." STRINGIZE(CLIENT_VERSION_REVISION) "." STRINGIZE(CLIENT_VERSION_BUILD)
    #ifdef BUILD_GIT_COMMIT
        #define BUILD_DESC BUILD_BASE_DESC "-" BUILD_GIT_COMMIT
    #elif defined(GIT_COMMIT_ID)
        #define BUILD_DESC BUILD_BASE_DESC "-g" GIT_COMMIT_ID
    #else
        #define BUILD_DESC BUILD_BASE_DESC "-unk"
    #endif
#endif

const std::string CLIENT_BUILD(BUILD_DESC);

Updated 09ee167a6dd7b2d4ade828affc95a679acfaf6c6 -> c269e618cf53634d3cf270273ab1b0354dc3c119 (pr18616.05 -> pr18616.06, diff):

• addressed @dongcarl's comment

FYI: I tested a branch with both this and my changes in #18741, builds passed fine!

@dongcarl

FYI: I tested a branch with both this and my changes in #18741, builds passed fine!

Does it mean "re-ACK" from you? :)

Yup! ACK c269e618cf53634d3cf270273ab1b0354dc3c119