Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add missing CAutoFile::IsNull() check in main #5437

Closed
wants to merge 1 commit into from

Conversation

@Diapolo
Copy link

commented Dec 7, 2014

  • also use func in error messages related to block files
  • minor comment changes and remove space after function name in log
    messages in touched functions
@sipa

View changes

src/main.cpp Outdated
}
hashBlock = header.GetHash();
if (txOut.GetHash() != hash)
return error("%s : txid mismatch", __func__);
return error("%s: Txid mismatch", __func__);

This comment has been minimized.

Copy link
@sipa

sipa Dec 7, 2014

Member

Why?

This comment has been minimized.

Copy link
@Diapolo

Diapolo Dec 7, 2014

Author

Just because the space after function name is not needed, I had a discussion with @laanwj about that. Uppercase just because of consistency... Deserialize is also uppercase.

This comment has been minimized.

Copy link
@laanwj

laanwj Dec 9, 2014

Member

+1 on keeping txid in lowercase

This comment has been minimized.

Copy link
@Diapolo

Diapolo Dec 9, 2014

Author

I reverted that, no problem :).

Philip Kaufmann
add missing CAutoFile::IsNull() check in main
- also use __func__ in error messages related to block files
- minor comment changes and remove space after function name in log
  messages in touched functions

@Diapolo Diapolo force-pushed the Diapolo:CAutoFile_open branch to ccd056a Dec 9, 2014

@Diapolo

This comment has been minimized.

Copy link
Author

commented Dec 12, 2014

@sipa Getting an ACK now ;)?

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 16, 2014

Good catch.

But you add one missing check and you make changes all over the file. Can you please change this to just make the one change suggested in the pull title?

ACK in that case.

@laanwj laanwj added the Refactoring label Dec 16, 2014

@Diapolo

This comment has been minimized.

Copy link
Author

commented Dec 16, 2014

All changes are mentioned in the commit-msg, so you also choose to remove the space after : when logging stuff. This doesn't need a discussion at all if you could just credit what I'm doing and tell me please use 2 commits for example or that you will NACK all minor stuff I try to get in recently ^^.

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 16, 2014

  1. Isolated commits are good. If you can solve a problem with a one-line change, you're a ninja
  2. I'm indeed trying to tone down on the minor stuff. I'm getting a bit overworked, so I'm trying hard to focus on what matters
  3. Ancillary changes can unnecessarily break other pulls, there has been a bit of annoyance about that lately

So: please, do only the mentioned fix, and leave the rest of the file as-is.

laanwj added a commit that referenced this pull request Dec 19, 2014
@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 19, 2014

Merged via 84857e8

@laanwj laanwj closed this Dec 19, 2014

laanwj added a commit that referenced this pull request Dec 19, 2014

@Diapolo Diapolo deleted the Diapolo:CAutoFile_open branch Dec 20, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.