Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
    GuhuangLS
    @GuhuangLS
    @yoshiat Thanks . I would like to consult prattmic.
    Zach Koopmans
    @madscout12
    Hey Folks in the chat, community meeting in 20 minutes. See you there.
    Yong He
    @zhuangel
    To track the mincore syscall cost CPU util, we have created an issue at google/gvisor#1234
    Adin Scannell
    @amscanne
    I had a further thought about that issue, I will post on the GitHub bug
    (Excellent big number ;)
    Err bug number
    Zach Koopmans
    @madscout12
    @zhuangel Do you mind sharing the link to the presentation from yesterday? OK to make it public? I'd like to post it on the meeting minutes.
    Yong He
    @zhuangel
    Thanks @amscanne for reply the issue.
    @madscout12 I can't share the original presentation, I need to remove some pics from the presentation, and then complete our company information disclosure process.
    Zach Koopmans
    @madscout12
    @zhuangel Thanks for going throught that extra effort to share, if you do. Everyone on our end was super impressed with the work you've done. Thanks again!
    tanjianfeng
    @tanjianfeng
    Hi guys, it seems that we can have some refinement on "gVisor bot" when it does code merge. Sometimes a patch is merged with extra merge commit, like "4a620c43 Merge pull request #981 from tanjianfeng:fix-898"; and sometimes a patch is merged with the extra commit squashed, like "0416c247 Merge pull request #1176 from xiaobo55x:runsc_boot". We suppose neither of the above two cases are wanted; instead, better to merge the original commit with its orginal commit log. Any throughts?
    Adin Scannell
    @amscanne
    Looking at this now..
    Adin Scannell
    @amscanne
    So the second parent for the merge commit is f697d1a33e4e7cefb4164ec977c38ccc2a228099, which appears in the git history log where it's logical parent was in the original chain
    e.g. See November 23rd here:
    For #1176, the original commit was 05871a1cdc73e98df58f56841be23a4eac27225c, which also appears in the history log where it's logical parent was
    e.g. See November 12th here:
    If you go to the merge commit you can find the original parent commit by clicking the right parent, e.g. google/gvisor@0416c24
    Adin Scannell
    @amscanne
    So I think that both cases above are actually working the same way, and the original commit is preserved, it just appears in the "git log" chain where it was originally derived, and the merge appears where it was merged
    I think what you're asking for is automatic rebasing, so it doesn't appear as a merge commit and will be reparented when merged
    Adin Scannell
    @amscanne
    I can see if that's possible, but .. multi-change pull requests would be squashed into a single change instead of merge (which is actually a bigger alternation)
    Adin Scannell
    @amscanne
    But I think this just may be a case of the commit log being a bit confusing in the merge case,... please let me know if I'm not understanding the #981 vs #1176 cases
    tanjianfeng
    @tanjianfeng
    If you see commit 0416c247eca, this commit does not contain its original commit log.
    tanjianfeng
    @tanjianfeng

    I can see if that's possible, but .. multi-change pull requests would be squashed into a single change instead of merge (which is actually a bigger alternation)

    That's not what I mean. We'd better not squash multiple commits into one for merge. What I'm saying is: better not introduce the extra merge commit; like commit 4a620c436da, there's no single line of code.

    Adin Scannell
    @amscanne
    0416c247eca has 05871a1cdc7 as it’s second parent — isn’t this the original commit log that you’re looking for? Or is it something else?
    If you do a straight “git log” (or look in the GitHub UI), 05871a1cdc7 will appear where it was originally done, and the merge commit will appear where it was merged
    tanjianfeng
    @tanjianfeng

    0416c247eca has 05871a1cdc7 as it’s second parent

    But this pull request only has one commit: google/gvisor#1176

    Adin Scannell
    @amscanne
    Yes that’s 05871a1, which is in the git history.. the merge commit is not destroying any information, it’s preserving 1) the merge order and 2) the original author order
    No commits are being squashed
    I guess I’m trying to understand because the feedback is slightly at odds: “We'd better not squash multiple commits into one for merge.” => this doesn’t happen, “better not introduce the extra merge commit” => nothing gets squashed because the merge commit is there
    It might be feasible to get rid of the merge commit and do an automated rebase, but it wouldn’t only be possible if multi-commit changes are squashed (because the CI system can’t atomically commit multiple changes without the merge)
    Adin Scannell
    @amscanne
    It might also just be a case of merges being confusing: do things make more sense if you just do “git log —no-merges” ?
    tanjianfeng
    @tanjianfeng
    "It might be feasible to get rid of the merge commit", this is what I'm suggesting; though, I don't understand the next part: "because the CI system can’t atomically commit multiple changes without the merge".

    It might also just be a case of merges being confusing: do things make more sense if you just do “git log —no-merges” ?

    Yep, I can do that.

    Another question, if we do "git show 0416c247eca", it contains some code (not in original PR), where it comes from?
    Adin Scannell
    @amscanne
    Every commit on master goes through a full CI process, which includes a bunch of side effects when merged, e.g. the go branch is generated, a release is generated
    (Sorry explaining the CI bit first)
    tanjianfeng
    @tanjianfeng
    I see, it's auto-generated.
    Adin Scannell
    @amscanne
    We don’t really have the ability to commit part of a pull request, and commits must be atomic for these reasons.. so we can’t really do a multi-commit rebase (it would need to be squashed)
    Re: git show
    tanjianfeng
    @tanjianfeng
    I suppose it's committer's responsibility to rebase to latest code before merge, isn't it?
    Adin Scannell
    @amscanne
    The diff of a merge commit can be confusing
    I recommend just following the second parent link or ignoring merge commits
    I will see if we can get rid of the merge commit and do an automatic rebase for the single commit case
    tanjianfeng
    @tanjianfeng

    I recommend just following the second parent link or ignoring merge commits

    OK, I'll try to understand this. Thanks!

    I will see if we can get rid of the merge commit and do an automatic rebase for the single commit case

    Great, that'll save some confusion. Thanks!

    Adin Scannell
    @amscanne
    I agree this would be clearer, but I just want to reinforce that the original commits are in the history .. the presence of the merges are just causing a bit of confusion
    tanjianfeng
    @tanjianfeng
    Yep, I'll try to understand it and get used to it. Thanks!