Skip to content

Conversation

@minad
Copy link
Member

@minad minad commented Apr 4, 2019

Wconversion and Wsign-conversion is still not activated by default,
since there are many issues in demo.c.

Taken from #194

Copy link
Collaborator

@nijtmans nijtmans left a comment

Choose a reason for hiding this comment

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

Already discussed!

Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

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

LGTM, please rebase, then we can merge

Wconversion and Wsign-conversion is still not activated by default,
since there are many issues in demo.c.
@minad
Copy link
Member Author

minad commented Apr 5, 2019

@sjaeckel I rebased both the alloc-sizes PR and this one. Could we please stop this practice otherwise it is always going back and forth? The history won't look much different if non-rebased PRs are merged (if they merge cleanly).

@sjaeckel sjaeckel merged commit 32b3351 into develop Apr 5, 2019
@sjaeckel sjaeckel deleted the cast-cleanup branch April 5, 2019 08:54
@sjaeckel
Copy link
Member

sjaeckel commented Apr 5, 2019

Could we please stop this practice otherwise it is always going back and forth? The history won't look much different if non-rebased PRs are merged (if they merge cleanly).

GitHub doesn't even allow you by default to merge an outdated branch so what?

Screenshot_2019-04-05_11-31-31

Either you merge back from develop to make GH happy or you rebase... and we rebase.

Discussion finished?

@minad
Copy link
Member Author

minad commented Apr 5, 2019

I still disagree, but you are hard to convince. The rebasing all the time does not scale and git is made such that it allows merging from different branches. The history must not be linearized. In this case, I think github allows the merge but it warns you since it doesn't want to be responsible if something goes wrong ;)
If the merge is not possible due to merge conflicts, it completely disallows it as you have seen before.
Alternatively you could do it manually with your git client.

@minad
Copy link
Member Author

minad commented Apr 5, 2019

Hmm and there is also this update branch button which merges stuff back from develop and then the merge will work?

@sjaeckel
Copy link
Member

sjaeckel commented Apr 5, 2019

The rebasing all the time does not scale and git is made such that it allows merging from different branches. The history must not be linearized. In this case, I think github allows the merge but it warns you since it doesn't want to be responsible if something goes wrong ;)

I'm fully aware of that. Your only argument in there is "the approach doesn't scale" but TBH I don't see the need to "scale" in a circle of 4 people where the bottleneck is IMO clearly code-review capacity.

Hmm and there is also this update branch button which merges stuff back from develop and then the merge will work?

after travis has finished building

@minad
Copy link
Member Author

minad commented Apr 5, 2019

Yes it is not the bottleneck, but it is just unnecessary work on the side of the PR author. For you it does not make a big difference except that it takes longer to go back and forth between you and the PR author. I think, the Travis argument is not relevant since Travis also has to finish after the manual rebase by the PR author.

I've yet to see a good argument why you want the history to be linear. Only because it looks a bit nicer in the git history? It is also not safer (in the sense that nothing breaks) if the merge back from develop to the PR branch is done before and Travis has checked the updated branch. In the case where merge conflicts occur I perfectly understand that the rebase is necessary and should be done by the PR author, but in the case without conflicts, the issue is solved by git.

@sjaeckel
Copy link
Member

sjaeckel commented Apr 5, 2019

it is just unnecessary work on the side of the PR author

that's why I sometimes do the rebase myself, but as you sign your commits I didn't do that for your PR's yet.

I've yet to see a good argument why you want the history to be linear. Only because it looks a bit nicer in the git history?

Because the graph is harder to read if it's not linear, I couldn't even say that it looks nicer, it's just easier to follow.

@minad
Copy link
Member Author

minad commented Apr 5, 2019

Ok :) I don't want to argue for too long. And I somehow buy the signature argument. What about requiring signatures from all the contributors ;)

@sjaeckel
Copy link
Member

sjaeckel commented Apr 5, 2019

Also the back-merge from develop pollutes the history

What about requiring signatures from all the contributors ;)

IMO signed commits are a doubtful feature that brings exactly nothing besides a false sense of security.

@minad
Copy link
Member Author

minad commented Apr 5, 2019

IMO signed commits are a doubtful feature that brings exactly nothing besides a false sense of security.

What are you saying here? This is just wrong. It pushes the trust to GPG and if you trust my key it is much more trustworthy than the SHA alone.

@sjaeckel
Copy link
Member

sjaeckel commented Apr 5, 2019

I think it pushes trust especially to your development environment, where you as a person are probably someone to trust, but IMO the general public is absolutely untrustworthy when it comes to their development environments.
But probably you're right and in the end it brings an net improvement... let me think about it a bit longer.
Are you entering the passphrase to your GPG key for each commit or at least each few minutes/hours after gpg-agent expired?

@minad
Copy link
Member Author

minad commented Apr 5, 2019

I thought a bit and read about it. I've read general consensus is that releases/tags should be signed. The reasons are obvious I guess. Now the question is why this should not be done in a more fine grained way (every commit). At first there is the issue with the SHA forging, which is not really realistic however impossible if GPG signing is used and you are not in the possession of a quantum computer ;)

If not every commit is signed, there is plausible deniability for the committer. But as you said, since the signing is automatized on the development machine, the trust is pushed to the development environment. I am unsure if you can ask for much more than that. Ideally the releases would be transferred to an airgapped machine, reviewed and signed there. But you would still have to trust the review environment, who says that it doesn't contain malicious code, injecting some code behind your back (like trusting trust)? And at this point we can just give up since we probably cannot review a that environment.

Concerning my development environment - I am using gpg-agent and it expires from time to time. Unfortunately this means there is some window where an attacker could do something. However I am on a qubes machine, which is pretty safe I hope. Still there are ways someone could inject malicious code without me observing immediately.

But then - I am not a high profile target and the measures just raise the bar. If I would be, I could take even more precautions. I would probably go completely stateless with some hardware token and never leave my notebook physically unattended.

I think only because something does not give you 100% of security it can still be useful. This applies to many things. Crypto, sandboxing, exploit mitigations etc etc. In the end I would like provable security top to bottom - but we will never get that. We have to layer the things and introduce multiple trust layers. For example recently I toyed with https://github.com/solo5/solo5, which is a minimal hypervisor api, which introduces a pretty strong sandbox. Outside the sandbox is an OS like OpenBSD or Linux with huge attack surface, inside the sandbox things are still unsafe, but then you could layer a safe language like ocaml or rust on top, etc etc. It is interesting, you might want to take a look :)

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.

4 participants