gvisor/community

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:

https://github.com/google/gvisor/commits/master

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:

https://github.com/google/gvisor/commits/master?after=10bbcf97d25b824aa0565af114e3272d3e314d19+34

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

https://stackoverflow.com/questions/40986518/git-show-of-a-merge-commit

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!

Where communities thrive

People

Repo info

Activity