https://www.chiliproject.org/2010-12-29T10:56:54+01:00ChiliProjectChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=97322010-12-29T10:56:54+01:00Muntek Singhsikhnerd@gmail.com
<ul><li><strong>Start date</strong> set to <i>2010-12-29</i></li><li><strong>Estimated time</strong> deleted ()</li><li><strong>Priority</strong> changed from <i>Normal</i> to <i>Normal</i></li><li><strong>Project</strong> set to <i>ChiliProject</i></li><li><strong>Target version</strong> set to <i>1.1.0 — Bell</i></li><li><strong>Assignee</strong> set to <i>Eric Davis</i></li><li><strong>% Done</strong> changed from <i>0</i> to <i>0</i></li><li><strong>Subject</strong> set to <i>Code review of latest Redmine commits (November and December) </i></li><li><strong>Category</strong> deleted ()</li><li><strong>Tracker</strong> set to <i>Task</i></li><li><strong>Due date</strong> deleted ()</li><li><strong>Subproject of</strong> deleted ()</li><li><strong>Description</strong> set to <i>Code review of latest Redmine commits (November and December)
</i> <a href="/journals/9732/diff/description" class="lightbox-ajax">More</a></li><li><strong>Status</strong> changed from <i>Open</i> to <i>Closed</i></li></ul> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=582010-12-29T16:51:46+01:00Eric Davis
<ul></ul><p>I've been reading a few of the commit and there will be a few that need to be reverted or reworked. Commit 73167fb4f26b34c1da9c9bd9c12d175442a8e56d is the first questionable one that needs to be reviewed (2010-11-06 03:57).</p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=602010-12-29T16:55:16+01:00Eric Davis
<ul><li><strong>Target version</strong> deleted ()</li></ul> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=1102011-01-02T17:49:54+01:00Eric Davis
<ul></ul><p>I'm thinking we should create an issue for each commit to do a "code review". This might be useful later on too, that way there can be discussions around each commit and the reviewer can be recognized ("Closed/Reviewed by Eric").</p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=2812011-01-12T14:29:28+01:00Eric Davis
<ul><li><strong>Tracker</strong> changed from <i>Bug</i> to <i>Task</i></li></ul> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=3292011-01-13T15:37:33+01:00Eric Davis
<ul><li><strong>Subproject of</strong> deleted (<strike><i>#28</i></strike>)</li></ul> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=6932011-02-01T20:42:23+01:00Holger Justchiliproject@meine-er.de
<ul><li><strong>Target version</strong> set to <i>1.1.0 — Bell</i></li></ul> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=6982011-02-01T22:49:41+01:00Felix Schäferfelix+chili@oh14.de
<ul><li><strong>Status</strong> set to <i>Open</i></li></ul> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=8622011-02-06T01:58:44+01:00Eric Davis
<ul><li><strong>Assignee</strong> set to <i>Eric Davis</i></li></ul><p>Starting on this</p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=8672011-02-06T05:12:38+01:00Eric Davis
<ul></ul><p>Did some fast code reviews of everything up to 1.0.5 on <a href="https://www.chiliproject.org/projects/chiliproject/wiki/Edavis10" class="wiki-page">my page</a>. I've accepted everything so far but I found quite a few things we need to make a decision on (e.g. keep, revert, modify). I suspect the next set will have more things.</p>
<p>I'm still not sure how far we should go for our 1.1.0 release. Should we be mostly-compatible with Redmine 1.1.0?</p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=8682011-02-06T05:14:02+01:00Eric Davis
<ul></ul><p>P.S. Using this branch <a class="external" href="https://github.com/edavis10/chiliproject/tree/ticket/master/30-upstream-code-review">https://github.com/edavis10/chiliproject/tree/ticket/master/30-upstream-code-review</a></p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=8762011-02-06T12:26:12+01:00Felix Schäferfelix+chili@oh14.de
<ul></ul><p>Eric Davis wrote:</p>
<blockquote>
<p>I'm still not sure how far we should go for our 1.1.0 release. Should we be mostly-compatible with Redmine 1.1.0?</p>
</blockquote>
<p>I think that would be good at least for the DB to ease upgrading/changing back-and-fro, I don't have that much reserve regarding the external APIs though.</p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=9132011-02-09T00:17:14+01:00Eric Davis
<ul></ul><p>Okay, I'll keep going up to the Redmine 1.1.0 release and see what else I can pull out of 1.1.1.</p>
<p>For some of the larger changes, we might have to make some branches and cherry-pick. There are a lot of SCM changes that we might want.</p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=9142011-02-09T01:07:31+01:00Eric Davis
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>In Progress</i></li></ul><p>I've finished the code review and it looks like most of the commits will be fine to include in ChiliProject.</p>
<ul>
<li>There are quite a few we need to discuss</li>
<li>There are some that need bugfixes for</li>
<li>There are two commits I want to completely reject (User deletion) (Search on <a href="https://www.chiliproject.org/projects/chiliproject/wiki/Edavis10" class="wiki-page">Edavis10</a> for "rejected commit" to find them)</li>
</ul>
<p>I propose we:</p>
<ol>
<li>take all of the commits up to Redmine r4725 (ced782ecb21c62)</li>
<li>skip over the to commits to reject</li>
<li>cherry-pick the rest of the commits</li>
<li>open issues here to discuss and/or fix the commits I've flagged</li>
<li>after a discussion we can come back and revert or just change any code needed</li>
</ol>
<p>If we do it this way, I can do some merge magic and make our repository start right at r4725. This should make upgrades from Redmine 1.1.1 painless and also get some of the great stuff from Toshi MARUYAMA.</p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=9152011-02-09T01:14:23+01:00Robert Chady
<ul></ul><p>This seems like a valid approach. It would be nice to get user deletion working, but not the way it is in Redmine.</p>
<p>I am also glad to hear there will be what sounds like a formal code review process in place for CP.</p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=9162011-02-09T04:33:16+01:00Muntek Singhsikhnerd@gmail.com
<ul></ul><blockquote>
<p>I propose we:</p>
<ol>
<li>take all of the commits up to Redmine r4725 (ced782ecb21c62)</li>
<li>skip over the to commits to reject</li>
<li>cherry-pick the rest of the commits</li>
<li>open issues here to discuss and/or fix the commits I've flagged</li>
<li>after a discussion we can come back and revert or just change any code needed</li>
</ol>
<p>If we do it this way, I can do some merge magic and make our repository start right at r4725. This should make upgrades from Redmine 1.1.1 painless and also get some of the great stuff from Toshi MARUYAMA.</p>
</blockquote>
<p>Looks good, I glanced through the review as they came and didn't see any I didn't agree with you, nor did my programmers find any. This proposal is sound, and I look forward to seeing it.</p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=9182011-02-09T08:36:28+01:00Felix Schäferfelix+chili@oh14.de
<ul></ul><p>Eric Davis wrote:</p>
<blockquote>
<p>I propose we:</p>
<ol>
<li>take all of the commits up to Redmine r4725 (ced782ecb21c62)</li>
<li>skip over the to commits to reject</li>
<li>cherry-pick the rest of the commits</li>
<li>open issues here to discuss and/or fix the commits I've flagged</li>
<li>after a discussion we can come back and revert or just change any code needed</li>
</ol>
<p>If we do it this way, I can do some merge magic and make our repository start right at r4725. This should make upgrades from Redmine 1.1.1 painless and also get some of the great stuff from Toshi MARUYAMA.</p>
</blockquote>
<p>Works for me.</p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=9372011-02-09T20:14:35+01:00Eric Davis
<ul></ul><p>Thanks everyone, I'll redo my branch and get it ready to send to the main repo.</p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=9412011-02-10T01:22:39+01:00Eric Davis
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Closed</i></li></ul><p>Alright, the main repo how now been updated and we are ready to get started on actual development for 1.1.0 now.</p>
<a name="Gory-details-for-git-folks"></a>
<h2 >Gory details for git folks<a href="#Gory-details-for-git-folks" class="wiki-anchor">¶</a></h2>
<ol>
<li>First thing I did was to remove my feature branch and re-branch from the last master version (<a href="https://www.chiliproject.org/projects/chiliproject/repository/revisions/a52417eca9311aabaeaab847873aea52cb78ce52" class="changeset" title="Escapes attachment ids in TracMigrate (#6996). git-svn-id: svn+ssh://rubyforge.org/var/svn/redmi...">a52417eca9311aabaeaab847873aea52cb78ce52</a>)</li>
<li>Then I found the last liner commit we wanted to use, which was <a href="https://www.chiliproject.org/projects/chiliproject/repository/revisions/ced782ecb21c629e13306010ca34bbaa8e3bb473" class="changeset" title="Adds a User-Agent header to Redmine mailhandler requests (#7318). git-svn-id: svn+ssh://rubyforg...">ced782ecb21c62</a></li>
<li>I make a local branch off of that commit (<code>git checkout -b redmine-r4725 ced782ecb21c62</code>)</li>
<li>Then I did a <strong>fast forward merge</strong> (git's default) to make my feature branch look like it started from there. <code>git checkout ticket/master/30-upstream-code-review && git merge redmine-r4725</code></li>
<li>Then I deleted the <code>redmine-r4725</code> branch</li>
<li>Then I created a new branch so I could pull out the commits we want to keep but skip over the ones to reject <code>git checkout -b ticket/master/30-upstream-code-review-cherry-pick</code></li>
<li>Using git format-patch I created a patch for the single commit we wanted to keep that was after the rejected one (<code>git format-patch e17fadd07ac34f96a10a8b1e77b9ee06bc4149b2^..109fd2cdfc880a1b91bf6e658e4bc35c6bdcf85f</code>)</li>
<li>I applied this patch (remember I'm on the -cherry-pick branch now) <code>git am < 0001-Do-not-show-for-only-project-I-select-notification-o.patch</code></li>
<li>Using the same process, I created a single patch that was a combination of all of the remaining commits we wanted to include <code>git format-patch e17fadd07ac34f96a10a8b1e77b9ee06bc4149b2..109fd2cdfc880a1b91bf6e658e4bc35c6bdcf85f --stdout > full.patch</code></li>
<li>Then using git am I applied all of the commits in a single batch run (<code>git am full.patch</code>)</li>
<li>During this process several patches failed and I had to merge them by hand.</li>
<li>Once this was done I then merged the -cherry-pick branch back into the main feature branch, this time making sure the merge <strong>wasn't</strong> fast forwarded so we can track it later on (<code>git checkout ticket/master/30-upstream-code-review</code> <code>git merge --no-ff ticket/master/30-upstream-code-review-cherry-pick</code>)</li>
<li>Using git diff I was then able to compare the final results against Redmine's code. Our branch was identical except for the rejected commits and some whitespace changes in coderay (probably from svn/git). <code>git diff redmine-upstream/master</code></li>
<li>At this point I checked out ChiliProject's master branch (named <code>upstream</code>) and merged in my branch. <code>git checkout -b upstream-master upstream/master</code> and <code>git merge --no-ff ticket/master/30-upstream-code-review</code></li>
<li>Finally after a quick sanity check, I pushed this up to github into the master branch <code>git push upstream upstream-master:master</code> (I also configured this mapping to be automatic in my local config)</li>
</ol>
<p>I know this process looks confusing but we will probably never have to do it again. Future pulls from Redmine will be less work and we can cherry-pick as we go too.</p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=9422011-02-10T05:19:51+01:00Toshi MARUYAMA
<ul></ul><p>Mercurial Transplant extension is very smart.<br /><a class="external" href="http://mercurial.selenic.com/wiki/TransplantExtension">http://mercurial.selenic.com/wiki/TransplantExtension</a></p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=9492011-02-10T12:01:07+01:00Simon Hürlimannsimon.huerlimann@cyt.ch
<ul></ul><p>Mmh, why all the "magic"? I tried to reproduce by simply using the latest redmine version and just revert those commits you don't like. git diff to master seems to show that this resulted in same tree.</p>
<p>Beside being simpler, the reason for this is that git cherry doesn't return an ever growing list of commits. It also allows to do more simpler merges in the future...</p>
<p>just my 2c</p>
<p>Good work anyway!</p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=9502011-02-10T12:13:06+01:00Simon Hürlimannsimon.huerlimann@cyt.ch
<ul></ul><p>Another point: git revert XXX does clearly show the intent of reverting this patch. And the commit don't all get edavis10 as commiter;-)</p>
<p>Just to make sure I correctly understood what you did: Am I correct that what you did is more or less a 'git rebase -i XXX' where XXX is the redmine/master with dropping those 3 commits?</p> ChiliProject - Task #30: Code review of latest Redmine commits (November and December) https://www.chiliproject.org/issues/30?journal_id=9612011-02-10T23:31:40+01:00Eric Davis
<ul></ul><p>Simon Hürlimann wrote:</p>
<blockquote>
<p>Mmh, why all the "magic"? I tried to reproduce by simply using the latest redmine version and just revert those commits you don't like. git diff to master seems to show that this resulted in same tree.</p>
</blockquote>
<p>To have a clear history of where we branched at. Look around Feb 3rd on the <a href="https://github.com/chiliproject/chiliproject/network" class="external">network graph</a></p>
<blockquote>
<p>Beside being simpler, the reason for this is that git cherry doesn't return an ever growing list of commits. It also allows to do more simpler merges in the future...</p>
</blockquote>
<p>I'm not sure I understand, could we talk about this in the forums? I'm interested to see if there is a better way in the future.</p>
<blockquote>
<p>Another point: git revert XXX does clearly show the intent of reverting this patch. And the commit don't all get edavis10 as commiter;-)</p>
</blockquote>
<p>I didn't want to put one of our revert commits in the middle of the history and I didn't want to revert them at the end.</p>
<blockquote>
<p>Just to make sure I correctly understood what you did: Am I correct that what you did is more or less a 'git rebase -i XXX' where XXX is the redmine/master with dropping those 3 commits?</p>
</blockquote>
<p>Yes, it is almost like that. If I recall <code>git rebase -i</code> would have to rewrite the commits when changing the history and I think it would make me the author and committer. I also think it would reset the commit/author dates too.</p>