Re: [GSoC][PATCH v7 00/26] Convert "git stash" to C builtin

[Paul Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>]

Hi,

On Thu, Aug 16, 2018 at 1:25 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> On 08/08, Paul-Sebastian Ungureanu wrote:
>> Hello,
>>
>> Here is the whole `git stash` C version. Some of the previous
>> patches were already reviewed (up to and including "stash: convert
>> store to builtin"), but there are some which were not
>> (starting with "stash: convert create to builtin").
>
> Thanks for this new iteration, and sorry I took a while to find some
> time to review this.  I had another read through the patches up until
> patch 15, and left some comments, before running out of time again.  I
> hope to find some time in the next few days to go through the rest of
> the series as well.

Thank you a lot for taking time to review my patches. It really means a lot.

> One more comment in terms of the structure of the series.  The
> patches doing the actual conversion from shell to C seem to be
> interleaved with cleanup patches and patches that make the C version
> use more internal APIs.  I'd suggest putting all the cleanup patches
> (e.g. "stash: change `git stash show` usage text and documentation")
> to the front of the series, as that's more likely to be
> uncontroversial, and could maybe even be merged by itself.

Good point.

> Then I'd put all the conversion from shell to C patches, and only once
> everything is converted I'd put the patches to use more of the
> internal APIs rather than using run_command everywhere.  A possible
> alternative would be to squash the patches to replace the run_command
> calls with patches that use the internal API directly, to save the
> reviewers some time by reading through less churn.  Though I'm kind of
> on the fence with that, as a faithful conversion using 'run_command'
> may be easier to review as a first step.

Makes sense. Actually, as you said, the patches that replace `run_command()`
or `pipe_command()` were not squashed because I thought it might be more
easier for reviewers. The `stash: replace all "git apply" child
processes with API
calls` patch is a exception to the rule because I was highly in doubts
if the patch
would actually be good.

> Hope this helps!

It helps a lot. Thank you!

>> In order to see the difference between the shell version and
>> the C version, I ran `time` on:
>>
>> * git test suite (t3903-stash.sh, t3904-stash-patch.sh,
>> t3905-stash-include-untracked.sh and t3906-stash-submodule.sh)
>>
>>         t3903-stash.sh:
>>         ** SHELL: 12,69s user 9,95s system 109% cpu 20,730 total
>>         **     C:  2,67s user 2,84s system 105% cpu  5,206 total
>>
>>         t3904-stash-patch.sh:
>>         ** SHELL: 1,43s user 0,94s system 106% cpu 2,242 total
>>         **     C: 1,01s user 0,58s system 104% cpu 1,530 total
>>
>>         t3905-stash-include-untracked.sh
>>         ** SHELL: 2,22s user 1,73s system 110% cpu 3,569 total
>>         **     C: 0,59s user 0,57s system 106% cpu 1,085 total
>>
>>         t3906-stash-submodule.sh
>>         ** SHELL: 2,89s user 2,99s system 106% cpu 5,527 total
>>         **     C: 2,21s user 2,61s system 105% cpu 4,568 total
>>
>>         TOTAL:
>>         ** SHELL: 19.23s user 15.61s system
>>         **     C:  6.48s user  6.60s system
>
> Awesome!

I hope that it will get even better.

Best regards,
Paul