zerovm / zrt

ZeroVM Run-Time environment
zerovm.org
66 stars 18 forks source link

alloc reporting added, pth added into libports #136

Closed YaroslavLitvinov closed 10 years ago

YaroslavLitvinov commented 10 years ago

need to rebuild glibc

rampage644 commented 10 years ago

Could you split those into smaller ones? That will help me, personally, to review it.

YaroslavLitvinov commented 10 years ago

Hmm, probably the problem is in pth codebase? It's can't be splited because it's a single project.

rampage644 commented 10 years ago

Ok, regarding second commit. It could be split (according to your commit message) to:

  1. Added pth
  2. Added pth test
  3. Added forgotten files

First one could also be split into smaller ones. Maybe i'm not so smart to review such long commits, but small ones are definitely manageable by me.

YaroslavLitvinov commented 10 years ago

I always trying to add feature and related test in single commit to get an atomic commit. The same related to first commit, and if do split it to several commits it then can't be an atomic because separately will be broken.

YaroslavLitvinov commented 10 years ago

problems related to timeouts mentioned in glibc repo https://github.com/zerovm/glibc/pull/17, was fixed with https://github.com/YaroslavLitvinov/zrt/commit/571c7509ab78b5f382cb94c38bfb2bc9e0d4f29a commit

YaroslavLitvinov commented 10 years ago

And for the future, it would be better to do smaller but not atomic commits (that separately doesn't work) or do it bigger but working?

mgeisler commented 10 years ago

@YaroslavLitvinov I'm strongly in favor of several small commit, but they must all work individually. I don't know the code here at all, so all I can understand is the commit messages. The second commit says

pth/pthread and test added, also added forgotten files for alloc report testing

The also is a strong hint that the commit is doing two things, not just one. The forgotten files should probably be committed as part of the first commit. So this means that the first commit is broken (if I understand the second commit message correctly) and that the second commit does too much.

Rewriting them would thus be great. I'll try to come up with a guide for doing such rewrites!

mgeisler commented 10 years ago

Please see https://github.com/zerovm/zerovm-docs/pull/17 for my take on how good commits should look like.

mgeisler commented 10 years ago

And please see https://github.com/mgeisler/zerovm-docs/commit/6633574c428b7bd80815d0b4d97ec9fb26286187 for a recipe for splitting commits.

YaroslavLitvinov commented 10 years ago

Thank you Martin for the answer, as I understood we don't need atomic commits at all. I've performed some changes in according to contributing rules.

rampage644 commented 10 years ago

Good job! Now reviewing much much easier for me. Thanks, Yarolsav!

rampage644 commented 10 years ago

Will merge after testing