Week 8: First review received


After sending the patch series to Git mailing list, I have received some useful reviews. First, one interesting recommendation was to split the series into smaller groups in order to be easily reviewed. Let me remind that patch series was composed of 29 patches.

In this respect, I am preparing a new patch series version with 11 patches that corresponds to preparatory and clean-up commits and all bisect.c libification work.

Another suggestion was using enums to represent error codes in bisect.c functions. But finally nothing has been done about this because there was no consensus about using them by the reviewers.

Other proposed changes were: message commits amending because some redundant explanations were found, and also add some code comments about the meaning of error codes in bisect.c functions. And with others minor fixes that were all about this first round review.

I have prepared a new branch with the feedback of the community, and soon, after been accepted by my mentor, this first part of 11 patches will be sent. And again, I will receive the reviewers comments, and we will do this in cycles, until it is accepted.

When this first part is integrated in next branch, it will be time to send another part of previous patch series.


Meanwhile, my future patch series, which I will send after previous patch series is accepted, is growing with new commits.

One of the commits I had done last week was about a fix in an if condition when you run git bisect log command. It has been extracted from this future work branch to be integrated with previous patch series. Great!

The things done as of today are on a branch and are listed below:

One of the issues/commits that was more difficult to me was fixing git bisect run calls. Now, git bisect shell command uses git rev-parse --sq-quote to pass arguments to git bisect--helper command. But with the porting to C we have to directly call git bisect run <cmd>. After studying what was the error and what was happening, I implemented an internal solution that has a similar function as git rev-parse to solve the issue.

This branch has to be reviewed by my mentor so maybe next week some commits or implementations will change.

Thank you for reading!


Reviewed - first round
Integrated on master branch

See also