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

[ADAM-718] Use filesystem path to get underlying file system. #719

Merged
merged 1 commit into from Jun 25, 2015

Conversation

Projects
None yet
4 participants
@fnothaft
Copy link
Member

fnothaft commented Jun 25, 2015

Resolves #718. Uses @jfeala's proposed fix. @jfeala, let me know if you'd like credit on the commit and I'll modify the commit author.

@massie

This comment has been minimized.

Copy link
Member

massie commented Jun 25, 2015

+1 Looks good to me.

@jfeala

This comment has been minimized.

Copy link
Contributor

jfeala commented Jun 25, 2015

sure, my github cred could use a boost. Thanks @fnothaft!

@fnothaft fnothaft force-pushed the fnothaft:fix-get-fs branch from e2188e6 to a66b638 Jun 25, 2015

@fnothaft

This comment has been minimized.

Copy link
Member

fnothaft commented Jun 25, 2015

Done! Thanks @jfeala!

@laserson

This comment has been minimized.

Copy link
Contributor

laserson commented Jun 25, 2015

Manually merged in a66b638

@laserson laserson closed this Jun 25, 2015

@fnothaft

This comment has been minimized.

Copy link
Member

fnothaft commented Jun 25, 2015

@laserson are you sure this merged correctly? I don't see a66b638 in master.

@laserson

This comment has been minimized.

Copy link
Contributor

laserson commented Jun 25, 2015

oops, I think you're right.

@laserson laserson reopened this Jun 25, 2015

@fnothaft

This comment has been minimized.

Copy link
Member

fnothaft commented Jun 25, 2015

fnothaft@a66b638 is linearly based off of b0ff134 so it should be OK to just use the github merge button.

laserson added a commit that referenced this pull request Jun 25, 2015

Merge pull request #719 from fnothaft/fix-get-fs
[ADAM-718] Use filesystem path to get underlying file system.

@laserson laserson merged commit af129d2 into bigdatagenomics:master Jun 25, 2015

1 check passed

default Merged build finished.
Details
@laserson

This comment has been minimized.

Copy link
Contributor

laserson commented Jun 25, 2015

Done. Actually, is there an easy way to tell whether a PR is linearly branched off the latest upstream master?

@fnothaft fnothaft deleted the fnothaft:fix-get-fs branch Jun 25, 2015

@fnothaft

This comment has been minimized.

Copy link
Member

fnothaft commented Jun 25, 2015

@laserson I'd been frustrated that there wasn't an easy way to tell that in Github for the longest time, but I literally just realized how. If you look at a single commit in github (e.g., fnothaft@a66b638), it lists the parent commit(s). If there is a single parent, and that parent commit is master/HEAD, then the commit is good. If the PR has multiple commits, then you just need to walk the commits in succession.

E.g., for this PR, the parent (b0ff134) is immediately right to the long commit hash for the single commit.
screen shot 2015-06-25 at 3 50 03 pm

@laserson

This comment has been minimized.

Copy link
Contributor

laserson commented Jun 25, 2015

But does GitHub display anywhere that a particular commit is master/HEAD? Or do I still need to crossreference that with git log in my terminal to determine that?

@laserson

This comment has been minimized.

Copy link
Contributor

laserson commented Jun 25, 2015

@laserson

This comment has been minimized.

Copy link
Contributor

laserson commented Jun 25, 2015

Still not as simple as could be...

@fnothaft

This comment has been minimized.

Copy link
Member

fnothaft commented Jun 25, 2015

@laserson eh! It is what it is...

@fnothaft

This comment has been minimized.

Copy link
Member

fnothaft commented Jun 25, 2015

I think the best approach would be for Github to have an option that only allowed fast-forward merges via the PR page (which I think would disallow non-linear merges), but for whatever reason Github has chosen to only allow non-fast-forward merges via the PR page (which leads to a merge commit per PR merged via Github's merge button, even if the merge could've been a fast-forward).

@laserson

This comment has been minimized.

Copy link
Contributor

laserson commented Jun 25, 2015

Apparently that's gitflow best practices, though it's a little bizarre.

@fnothaft

This comment has been minimized.

Copy link
Member

fnothaft commented Jun 25, 2015

What's a bit bizarre?

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