Re: [RFC][GSoC] Project proposal: convert interactive rebase to C

[Christian Couder <christian.couder@gmail.com>]

Hi,

On Thu, Mar 22, 2018 at 11:03 PM, Alban Gruin <alban.gruin@gmail.com> wrote:
> Hi,
>
> here is my second draft of my proposal. As last time, any feedback is
> welcome :)
>
> I did not write my phone number and address here for obvious reasons,
> but they will be in the “about me” section of the final proposal.
>
> Apart from that, do you think there is something to add?

Please take a look at the comments that have been made on other's
proposal. Many proposals could be improved in the same way and it is a
bit annoying for us to repeat the same things many times.

[...]

> The goal of this project is to rewrite git-rebase--interactive in C
> as it has been discussed on the git mailing list[1], for multiple
> reasons :

In general when the project or some issues related to the project have
already been worked on or discussed on the mailing list, it is a good
thing to summarize those discussions and link to them in your
proposal. It shows that you want to take the time to gather existing
information, to understand that information and to take it into
account in your proposal, and it can also make your proposal easier to
read and understand.

More specifically your proposal has some links which is nice, but I
think it would be better if it summarized a bit more what the links
contain.

[...]

> Weeks 1 & 2 -- May 14, 2018 - May 27, 2018
> /From May 14 to 18, I have exams at the university, so I won’t be able
> to work full time./
>
> I would search for edge cases not covered by current tests and write
> some if needed.
>
> Week 3 -- May 28, 2018 - June 3, 2018
> At the same time, I would refactor --preserve-merges in its own
> shell script (as described in Dscho’s email[1]), if it has
> not been deprecated or moved in the meantime.

Here for example it is better if we could get a better idea about how
you plan to do it without having to read Dscho's email.

> This operation is not
> really tricky by itself, as --preserve-merges is about only 50 lines
> of code into git_rebase__interactive().
>
> Weeks 4 to 7 -- May 4, 2018 - July 1, 2018
> Then, I would start to incrementally rewrite
> git-rebase--interactive.sh functions in C, and move them
> git-rebase--helper.c (as in commits 0cce4a2756[2] (rebase -i
> -x: add exec commands via the rebase--helper) and b903674b35[3]
> (bisect--helper: `is_expected_rev` & `check_expected_revs` shell
> function in C)).

I know what you mean but I would still appreciate if you could summarize it.

> There is a lot of functions into git-rebase--interactive.sh to
> rewrite. Most of them are small, and some of them are even wrappers
> for a single command (eg. is_merge_commit()), so they shouldn’t be
> really problematic.
>
> A couple of them are quite long (eg. pick_one()), and will probably
> be even longer once rewritten in C due to the low-level nature of the
> language. They also tend to depend a lot on other smaller functions.
>
> The plan here would be to start rewriting the smaller functions when
> applicable (ie. they’re not a simple command wrapper) before
> working on the biggest of them.
>
> Week 8 -- July 2, 2018 - July 8, 2018
> When all majors functions from git-rebase--interactive.sh have been
> rewritten in C, I would retire the script in favor of a builtin.
>
> Weeks 9 & 10 -- July 9, 2018 - July 22, 2018
> I plan to spend theses two weeks to improve the code coverage where
> needed.
>
> Weeks 11 & 12 -- July 23, 2018 - August 5, 2018
> In the last two weeks, I would polish the code where needed, in order
> to improve its performance or to make it more readable.

We like to have big improvements be split into batches of patches,
also called patch series, and polishing of the first batches happening
as soon as possible so that they can be ready to be merged soon. The
patch series that are sent to the mailing list often need a number of
round of reviews and improvements which can take a long time, so it is
better if this process starts as soon as possible.

Thanks.