<div dir="ltr">Hello James,<div><br></div><div>I am really sorry but I just realised that I introduced code from v-hasu's master branch rather than gsoc2014. Will try to make the fixes asap. Thanks for your suggestions, though. I will take care of them from the beginning this time. Will open a PR once I am done with everything.</div><div><br></div><div>Regards,</div><div>Ayush </div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 6, 2016 at 3:09 PM, James Aylett <span dir="ltr"><<a href="mailto:james-xapian@tartarus.org" target="_blank">james-xapian@tartarus.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Mon, Jun 06, 2016 at 01:13:14PM +0530, Ayush Tomar wrote:<br>
<br>
> I have completed introducing some code from v-hasu's branch into mine,<br>
> mainly for Features, FeatureVector and FeatureManager classes. I have<br>
> pushed the changes to <a href="https://github.com/ayshtmr/xapian/tree/letor-update" rel="noreferrer" target="_blank">https://github.com/ayshtmr/xapian/tree/letor-update</a>. I<br>
> am now proceeding to write unit tests for feature modules.<br>
<br>
</span>Great!<br>
<span class=""><br>
> There are a few things that I wanted to clarify:<br>
><br>
> 1. I have introduced a lot of code in a single commit (<br>
> <a href="https://github.com/ayshtmr/xapian/commit/2abaf6b8d64b1624699c3f490b6dc441e1c6cdf3" rel="noreferrer" target="_blank">https://github.com/ayshtmr/xapian/commit/2abaf6b8d64b1624699c3f490b6dc441e1c6cdf3</a><br>
> )<br>
> Is it fine or should I try to split things up?<br>
<br>
</span>A good rule of thumb is to aim for a maximum of 800 line<br>
changes. (This is based on observations of how long it takes to review<br>
code, and how long people can be effective at reviewing code.)<br>
Obviously for complex things, you want to aim somewhat lower.<br>
<br>
I think in this case we'll want to split the commit up in order to<br>
review it, and then squash it all down again when we merge. (That<br>
happens sometimes.)<br>
<br>
For instance, there's a rename of getlabel -> get_label which you<br>
could pull out into one commit (which is then a fairly straightforward<br>
commit to check, since providing the end result of the PR compiles it<br>
will have caught all the cases). Probably the easiest way of doing<br>
that is to do something like the following:<br>
<br>
* interactive rebase and mark that commit for edit<br>
* when you're dropped into the shell, use git reset --soft HEAD^ to<br>
get the commit into the stage<br>
* use git reset -p to remove everything but the getlabel rename lines<br>
from the index<br>
* git commit (the rename changes)<br>
* git add everything else back in and git commit that (or, more<br>
likely, continue to split into further commits)<br>
* git rebase --continue when you're done<br>
<br>
I think if you look carefully at that commit you'll also discover that<br>
the letor_features.cc changes contain a lot that's unwanted whitespace<br>
changes and should be dropped from the commit. (-b to git show and git<br>
diff can be helpful here, as it ignores whitespace changes. If it<br>
isn't there but shows up as an option when you're doing git add -p<br>
then it's a sign something's up, probably with your editor<br>
configuration.)<br>
<span class=""><br>
> 2. There is a lot of commented code in my branch right now that<br>
> needs to be removed. Is it wise to take care of it with each commit<br>
> I push or should it be done later, say, when some stability is<br>
> reached (post mid-term?)<br>
<br>
</span>I'd do it sooner rather than later, because it'll make the commits<br>
more manageable and also the files smaller, which is easier for you to<br>
deal with.<br>
<br>
What I'd do is to create fixup commits that remove the commented code,<br>
tied to whichever commits you want to get that removal into. Then you<br>
can squash them before we merge. (That's considerably easier for you,<br>
because you can focus first on getting the commit splits right, and<br>
then tidy up each one.)<br>
<span class="HOEnZb"><font color="#888888"><br>
J<br>
<br>
--<br>
James Aylett, occasional trouble-maker<br>
<a href="http://xapian.org" rel="noreferrer" target="_blank">xapian.org</a><br>
<br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div><div><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div>Ayush Tomar<br></div><div>Senior Undergraduate</div><div>Dept. of Computer Engg.<font color="#000000"> </font><br></div><div>Delhi Technological University </div><div><i>(formerly Delhi College of Engineering)</i></div></div></div></div></div></div></div></div></div></div></div></div></div></div>
</div>