On Thu, May 27, 2010 at 06:25:32PM +0200, Vicent Marti wrote:
>
> All the milestones for the first coding period are mostly finished
> (yay for that) and you can find the code and follow my progress on my
> public github repo [2]. There is some documentation in place, and
> there are some tests in place. In the following days I'll aim for
> about 90% code coverage on the tests, to finish the documentation, and
> then prepare the patch series for review on this list.
>
> Meanwhile, you are very much welcome to start flaming away before the
> patch series are ready. We have roughly one month and a half to fix up
> my code to the project's standards; hopefully it won't be *that* bad
> and I will be able to implement some extra features before the
> evaluation.
I start the flaming... :->
* I noticed that you seem to format if/while/for like this:
if (foo)
{
something1;
something2;
}
else
{
something3;
something4;
}
While the existing percedent seems to be:
if (foo) {
something1;
something2;
} else {
something3;
something4;
}
* Also, you appear to be using 4 spaces for indent, whereas the existing
percedent appears to be indent by tab. Some functions appear to use mixed
indents (sometimes even in one block construct).
* There seems to be some trailing whitespace. I don't know the policies of
libgit2 on that, but I suppose it is not supposed to be there.
* git_commit_list_push_back() fails silently if memory allocation fails. Is
it supposed to? Same for git_commit_list_push_front().
* Where algorithm in git_revpool_table__hash() is from? Since it appears to
hash binary object IDs, wouldn't just simple sum/xor over words be sufficient
(all SHA-1 output bits are very nearly independent). Or do you need to be
compatible with some other implementation (doesn't appear so, because hash
is computed differently depending on endianess)?
* gitrp_push() just returns if git_commit_parse_existing() fails. But causes
of that failing seemingly can include ODB read errors, which are fairly
serious...
* Is gitrp__enroot() supposed to just ignore failures of
git_commit_parse_existing()? is 'commit->parents.head' valid even in this
case?
* There are numerious cases where function that suspiciously lacks error
code is called (if error code is added, it presumably needs to be bubbled
back to caller).
-Ilari
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html