You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Remi Bergsma <RB...@schubergphilis.com> on 2015/09/11 16:26:04 UTC
No more direct commits to master please!
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.
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
Re: No more direct commits to master please!
Posted by Wido den Hollander <wi...@widodh.nl>.
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 <bh...@citrix.com>>
> AuthorDate: Thu Sep 10 04:14:30 2015 -0700
> Commit: Wido den Hollander <wi...@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
> mferreira@schubergphilis.com<ma...@schubergphilis.com>
>
>
>
>
> On 11 Sep 2015, at 16:39, Wido den Hollander <wi...@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
>
>
Re: No more direct commits to master please!
Posted by Miguel Ferreira <MF...@schubergphilis.com>.
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 <bh...@citrix.com>>
AuthorDate: Thu Sep 10 04:14:30 2015 -0700
Commit: Wido den Hollander <wi...@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.
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?
The bottom line here is that the mentioned commit (as well as others too) was not merged properly. It was directly pushed to master.
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.
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.
In case of doubts, find someone to double check.
I will always be more than happy to help out with such things.
Cheers,
\ Miguel Ferreira
mferreira@schubergphilis.com<ma...@schubergphilis.com>
On 11 Sep 2015, at 16:39, Wido den Hollander <wi...@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
Re: No more direct commits to master please!
Posted by Miguel Ferreira <MF...@schubergphilis.com>.
Hi Wido,
In order to figure out what went wrong, could you please point me to the merge commit (the one saying “Merge pull request …”) for commit b66dcda49f370e6fc91ebff889a694f17826ca44 ?
Cheers,
\ Miguel Ferreira
mferreira@schubergphilis.com<ma...@schubergphilis.com>
On 11 Sep 2015, at 16:39, Wido den Hollander <wi...@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
Re: No more direct commits to master please!
Posted by Remi Bergsma <RB...@schubergphilis.com>.
Hi Wido,
Thanks, good to know. Will try to figure out what went wrong here.
Regards, Remi
> On 11 Sep 2015, at 16:40, Wido den Hollander <wi...@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
>>
Re: No more direct commits to master please!
Posted by Wido den Hollander <wi...@widodh.nl>.
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
>