Re: Libgit2 on the Summer of Code

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