On 09/12/2015 02:29 PM, Miguel Ferreira wrote: > Hi Wido, > > I’ve been investigating what could have gone wrong with commit > "CLOUDSTACK-8799 fixed for vpc networks.” > (b66dcda49f370e6fc91ebff889a694f17826ca44). > This commit was directly pushed to master, that is, it was not merged as part > of a PR. > PR 784 contais the same commit (with different hash), and that PR is still > open and un-merged. > > [cid:70CF5375-376F-4C4C-A1D2-19C08A24D6E0@hq.forgot.her.name] > > In the picture above you can clearly see what happened. > Master is the left most line, and the commit is in there as a result of a > direct push, not a merge. A merge would mean a second line bringing that > commit in. > > Looking at the log for master we can understand when the commit was created, > and when it was last modified (i.e. committed to master). > >> git log --pretty=fuller b66dcda49f370e6fc91ebff889a694f17826ca44 > > commit b66dcda49f370e6fc91ebff889a694f17826ca44 > Author: Bharat Kumar > <bharat.ku...@citrix.com<mailto:bharat.ku...@citrix.com>> > AuthorDate: Thu Sep 10 04:14:30 2015 -0700 > Commit: Wido den Hollander <w...@widodh.nl<mailto:w...@widodh.nl>> > CommitDate: Fri Sep 11 14:57:32 2015 +0200 > > CLOUDSTACK-8799 fixed for vpc networks. > > The log shows that the commit was created by Bharat Kumar on Sep 10 04:14 > (-7000) and committed to master by you Sep 11 14:57 (+2000). > > > From your bash history you showed us that you’ve used the git-pr script like > this: > 2030 ./tools/git/git-pr https://github.com/apache/cloudstack/pull/784 > 2043 history |grep git-pr > > That should have checked out the PR branch, collected the PR info from the > github API, merge the PR branch into the current branch you were on the repo, > and nothing else. > The push to the remote repository has to be done manually, after inspecting > the git tree and making sure it’s all good. > I can’t be sure you’ve pushed right after because you’ve filter all other > commands out with the pipe to grep.
I didn't check them all manually. After I saw that the first PR merged properly I ran it multiple times to merge the PRs in. > But even if you did, that does not assure the git tree was as it should have > been. > It would depend on many things. For instance, were you on a “clean” master > branch (i.e. a master branch that is exactly like remote master) before > calling the script? > Yes, I always do a pull prior to doing anything on the master branch. > The bottom line here is that the mentioned commit (as well as others too) was > not merged properly. It was directly pushed to master. > Indeed, I can see that now. That was never the intention. > There can be many reasons for why this happened to you and to others as well. > Before investigating this commit I’ve spent some time investigating the > commits of PR 772, which was even more strange than this. > This has happened too many times already for us to consider this an isolated > case. > Anything we can do in the git-pr script to double-triple-check this? > Git is probably the most advanced version control system out there, and it > requires a steep learning curve to go from a SVN-like usage to a GIT-like > usage. > Always double check your git tree against the remote git tree before pushing > anything upstream. Lesson learned :) > In case of doubts, find someone to double check. There was no reason for me to doubt it. The first PR went through properly and as there were no git errors I assumed that all was well. > I will always be more than happy to help out with such things. > Thanks! > Cheers, > \ Miguel Ferreira > mferre...@schubergphilis.com<mailto:mferre...@schubergphilis.com> > > > > > On 11 Sep 2015, at 16:39, Wido den Hollander > <w...@widodh.nl<mailto:w...@widodh.nl>> wrote: > > > > On 11-09-15 16:26, Remi Bergsma wrote: > Hi all, > > What happened to master? I see a lot of direct commits. I thought we agreed > to make a PR, then _merge_ it to master with the script in ./tools/git/git-pr. > > > Errr, I used that script? > > This is what my Bash history shows me: > > wido@wido-desktop:~/repos/cloudstack$ history |grep git-pr > 1752 ./tools/git/git-pr https://github.com/apache/cloudstack/pull/757 > 2013 ./tools/git/git-pr https://github.com/apache/cloudstack/pull/783 > 2025 ./tools/git/git-pr https://github.com/apache/cloudstack/pull/794 > 2027 ./tools/git/git-pr https://github.com/apache/cloudstack/pull/807 > 2029 ./tools/git/git-pr https://github.com/apache/cloudstack/pull/795 > 2030 ./tools/git/git-pr https://github.com/apache/cloudstack/pull/784 > 2043 history |grep git-pr > wido@wido-desktop:~/repos/cloudstack$ > > They all had 2 LGTM, so I merged them. > > Any idea what went wrong here? > > We’re getting ready for a 4.6 RC and this makes stuff extra hard to track. > Most direct commits seem to come from PRs, but I don’t think all of them did. > > Talking about this (there are more): > https://github.com/apache/cloudstack/commit/b66dcda49f370e6fc91ebff889a694f17826ca44 > https://github.com/apache/cloudstack/commit/1c6378ec0056e8c75990a4a0c15e99b2df162a75 > https://github.com/apache/cloudstack/commit/1a02773b556a0efa277cf18cd099fc62a4e27706 > https://github.com/apache/cloudstack/commit/ba59a43333b6f31e48e4b6e43e16068e4cacdc45 > https://github.com/apache/cloudstack/commit/f661ac0a2a783447b6eaab590d58091ec542aec2 > > Please don’t make me revert the direct commits. > > If you need help with getting PRs merged, ping me or Rajani as we’re happy to > help. > > Thanks, > Remi > >
