Re: Libgit2 on the Summer of Code

From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Date: May 27, 2010, 17:46 UTC
Message-ID: <20100527174636.GB9909@LK-Perkele-V2.elisa-laajakaista.fi>
In-Reply-To: <AANLkTil-M3kQGbRZIacItlwr37Lf3lso7FspftLr43_F@mail.gmail.com>

On Thu, May 27, 2010 at 06:25:32PM +0200, Vicent Marti wrote:

Show 13 quoted lines

> 
> 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