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

skypeforlinux: caution against updates to unstable versions #32684

Closed
wants to merge 1 commit into from

Conversation

orivej
Copy link
Contributor

@orivej orivej commented Dec 14, 2017

Motivation for this change

#32604 (comment)

@orivej
Copy link
Contributor Author

orivej commented Dec 14, 2017

Upstream publishes x.y.0.z in the stable dist and x.y.76.z in the unstable:
https://repo.skype.com/deb/dists/stable/main/binary-amd64/Packages
https://repo.skype.com/deb/dists/unstable/main/binary-amd64/Packages

@rycee
Copy link
Member

rycee commented Dec 15, 2017

Rebased to master in 9045920. Thanks a lot for the clarification!

@rycee rycee closed this Dec 15, 2017
@rycee rycee mentioned this pull request Dec 15, 2017
8 tasks
@orivej
Copy link
Contributor Author

orivej commented Dec 15, 2017

Thank you!

Rebased to master in 9045920

Why could not you merge the PR?

@rycee
Copy link
Member

rycee commented Dec 15, 2017

I typically avoid doing merges for small PRs to help keep the history somewhat clean.

@orivej
Copy link
Contributor Author

orivej commented Dec 15, 2017

Please prefer to (1) merge the PR on GitHub, or else (2) rebase the whole PR onto the target branch before merging it on GitHub, or else (3) include the PR number in the commit message, because otherwise it is difficult to find the PR that introduced an interesting commit when you need additional context or want to discuss the change, GitHub UI does not display it as merged, the branch is not deleted when you delete all your merged branches. (git-whence makes it simple to find the PR of any merged commit.) Your effort to keep the history linear is dwarfed by merges of unrebased PRs and does not benefit any workflow that I know of.

@bjornfor
Copy link
Contributor

A commit (message) should provide context on its own. If needing access to the PR to understand why a commit is made, something is wrong, IMHO.

@bjornfor
Copy link
Contributor

Disclaimer: I consider git to hold the project history, much more than GitHub.

@orivej
Copy link
Contributor Author

orivej commented Dec 15, 2017

Even if commits contained the complete context at the time they were made, they would tell nothing about what was discussed and became known after they were made.

If needing access to the PR to understand why a commit is made, something is wrong

Having access to the PR is useful for other reasons. Apart from being the best place for review, it tells what options were considered and what were not, who is interested or knowledgeable in the subject, etc.

@orivej orivej deleted the skypeforlinux branch December 15, 2017 17:05
@bjornfor
Copy link
Contributor

Even if commits contained the complete context at the time they were made, they would tell nothing about what was discussed and became known after they were made.

For discussion on the final commit (afterwards), you can comment directly on the commit object in github: https://github.com/NixOS/nixpkgs/commit/COMMIT_ID.

If you want to lookup the PR to see the pre-commit discussion, type the commit id in the PR search field. (Assuming the committer posted the final commit id into the PR.)

If needing access to the PR to understand why a commit is made, something is wrong

Having access to the PR is useful for other reasons. Apart from being the best place for review, it tells what options were considered and what were not, who is interested or knowledgeable in the subject, etc.

For big PRs, yes. For small/trivial PRs, I personally don't think the merge commit adds much value. (One merge commit for one (real) commit is noise to me.)

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

Successfully merging this pull request may close these issues.

None yet

4 participants