Re: [RFC] [GSoC] Project proposal: convert scripts to builtins

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 20, 2018, 15:28 UTC
Message-ID: <nycvar.QRO.7.76.6.1803201616290.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
In-Reply-To: <20180320093421.26817-1-predatoramigo@gmail.com>

Hi Pratik,

thank you so much for posting this inline, to make it easier to review.

I will quote only on specific parts below; Please just assume that I like the other parts and have nothing to add.

On Tue, 20 Mar 2018, Pratik Karki wrote:

Show 8 quoted lines

> 
> Timeline and Development Cycle
> ------------------------------
> 
> -   Apr 23: Accepted student proposals announced.
> -   Apr 23 onwards: Researching of all the test suites. Discussion of
>     possible test improvements in for `git-stash` and `git-rebase`.
> -   May 1: Rewriting skeleton begins.

I would have liked more detail here. Like, maybe even a rudimentary initial version identifying, say, a part of `git stash` and/or `git rebase` that could be put into a builtin (stash--helper and rebase--helper, respectively).

It is my experience from several GSoCs working on this huge overarching project to convert the scripts (which are good prototypes, but lack in stringency in addition to performance) to C that even the individual scripts are too much to stem for a single GSoC.

> -   May 13: Making `builtin/stash.c` ready for review process. (This
>     goes on for some time.)

There have been two past efforts to turn stash into a builtin:

	https://github.com/git-for-windows/git/pull/508

and

	https://public-inbox.org/git/20171110231314.30711-1-joel@teichroeb.net/

It would be good to read up on those and incorporate the learnings into the proposal.

> -   May 26: Making `builtin/rebase.c` ready for review process. (This
>     goes on for some time.)

The `git-rebase.sh` script is itself not terribly interesting, as it hands off to `git-rebase--am.sh`, `git-rebase--interactive.sh` and `git-rebase--merge.sh`, respectively.

Converting `git-rebase` into a builtin without first converting all of those scripts would make little sense.

It would probably be better to choose one of those latter scripts and move their functionality into a builtin, in an incremental fashion.

By doing it incrementally, you can also avoid...

> -   June 10: Make second versions with more improvements and more
>     batteries ready for next review cycle.

... leaving two weeks between checkpoints. Also, doing it incrementally lets you avoid sitting on your hands while waiting for the first patches to be reviewed.

> -   June 20: Writing new tests and using more code-coverage tools to
>     squash bugs present.

Typically it helps a lot to have those tests *during* the conversion. That's how I found most of the bugs when converting difftool, for example.

Show 5 quoted lines

> -   June 25 - Jul 20: Start optimizing `builtin/stash.c` and
>     `builtin/rebase.c`. Benchmarking and profiling is done. They are
>     exclusively compared to their original shell scripts, to check
>     whether they are more performant or not and the results are
>     published in the mailing list for further discussion.

Could you add details how you would perform benchmarking and profiling?

Show 7 quoted lines

> -   Jul 20 - Aug 5: More optimizing and polishing of `builtin/stash.c`
>     and `builtin/rebase.c` and running of new tests series written and
>     send them for code review.
> -   Aug 14: Submit final patches.
> -   Aug 22: Results announced.
> -   Apr 24 - Aug 14: Documentation is written. "What I'm working on" is
>     written and posted in my blog regarding GSoC with Git.

The timeline is a bit ambitious. I would like to caution you that these are all big tasks, and maybe you want to cut down on the deliverables, and add more detail what exactly you want to deliver (such as: what part of stash/rebase do you find under-tested in our test suite and would therefore want to augment, what parts of stash/rebase do you think you would handle first, and how?).

Ciao, Johannes