Hi,

On Thu, Aug 16, 2018 at 1:25 AM, Thomas Gummerer [off-list ref] wrote:

On 08/08, Paul-Sebastian Ungureanu wrote:

quoted

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!

quoted

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