Skip to content

Some minor fixes#92

Merged
Roasbeef merged 1 commit into
lightningnetwork:masterfrom
cjamthagen:fixes
Jan 11, 2017
Merged

Some minor fixes#92
Roasbeef merged 1 commit into
lightningnetwork:masterfrom
cjamthagen:fixes

Conversation

@cjamthagen
Copy link
Copy Markdown
Contributor

Break out of loop once txIndex is found.
Get correct block height for txConfirmation.

Copy link
Copy Markdown
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

As I pointed out in the comments section of the review, the second aspect of the commit has already landed in the latest version of master in an identical form. So if you rebase to the latest master, it should disappear.

One final item to be addressed before I merge this:

  • To match the style of most of the commits within the project, the commit body+header should adhere to the guidelines outlined in the contribution guidelines.

  • A suitable commit header would be something like:

    chainntfns: add missing break statements in search for txindex

    Or something along those lines.

Comment thread chainntnfs/btcdnotify/btcd.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent catch, here my off-by-one senses were tingling when I originally wrote this fragment. Good to see the bug corrected. However, the same error was noticed by @bryanvu and fixed in his latest PR. If you rebase to the latest version of master, the part of the diff should disappear.

@Roasbeef
Copy link
Copy Markdown
Member

Roasbeef commented Jan 6, 2017

I recently committed a pretty large change in master catching up some of our btcsuite libraries to their latest upstream versions within my fork. As a result, you'll need to rebase to master again before I can get this PR merged.

First, ensure that you've pulled the latest version of master into the master branch you have locally. I'm not sure how you've set up your git environment locally, but typically with another project, I'll work from that project's root (in the case: $GOPATH/github.com/lightningnetwork/lnd), then create a new remote which points to my forks on github. From my PoV this would entail executing:

  • git remote add roasbeef git@github.com:roasbeef/lnd.git

So then anytime I want to push a new branch to possibly be a PR to the mainline, or modify an existing PR, I'd do:

  • git push roasbeef new-branch-for-PR

Then if I needed to catch up my changes to the latest version of master, I'd execute:
* git checkout new-branch-for-PR
* git rebase -i master

An interactive rebase will pop up in your default text editor, if you see any lingering commits that are left over from a failed attempt (like the few that are on this branch now), simply deleting those lines will remove them from your rebased branch. After finishing the interactive rebase, you'll then force push to your remote branch: git push --force roasbeef new-branch-for-PR. It's typically a good idea to first back up your local branch by creating a new branch off of it before you mess with any rebase shenanigans.

If you need any help with the above process, or have any more questions, we now have the #lnd channel on Freenode.

Copy link
Copy Markdown
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good now, the extra commits have been discarded.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants