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

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Aug 15, 2018, 22:25 UTC
Message-ID: <20180815222503.GR2734@hank.intra.tgummerer.com>
In-Reply-To: <cover.1533753605.git.ungureanupaulsebastian@gmail.com>

On 08/08, Paul-Sebastian Ungureanu wrote:

Show 6 quoted lines

> 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.

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.

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.

Hope this helps!

Show 25 quoted lines

> 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!

Show 69 quoted lines

> * a git repository with 4000 files: 1000 not changed,
> 1000 staged files, 1000 unstaged files, 1000 untracked.
> In this case I ran some of the most used commands:
> 
>         git stash push:
> 
>         ** SHELL: 0,12s user 0,21s system 101% cpu 0,329 total
>         **     C: 0,06s user 0,13s system 105% cpu 0,185 total
> 
>         git stash push -u:
> 
>         ** SHELL: 0,18s user 0,27s system  108% cpu 0,401 total
>         **     C: 0,09s user 0,19s system  103% cpu 0,267 total
> 
>         git stash pop:
> 
>         ** SHELL: 0,16s user 0,26s system 103% cpu 0,399 total
>         **     C: 0,13s user 0,19s system 102% cpu 0,308 total
> 
> Best regards,
> Paul Ungureanu
> 
> 
> Joel Teichroeb (5):
>   stash: improve option parsing test coverage
>   stash: convert apply to builtin
>   stash: convert drop and clear to builtin
>   stash: convert branch to builtin
>   stash: convert pop to builtin
> 
> Paul-Sebastian Ungureanu (21):
>   sha1-name.c: added 'get_oidf', which acts like 'get_oid'
>   stash: update test cases conform to coding guidelines
>   stash: renamed test cases to be more descriptive
>   stash: implement the "list" command in the builtin
>   stash: convert show to builtin
>   stash: change `git stash show` usage text and documentation
>   stash: refactor `show_stash()` to use the diff API
>   stash: update `git stash show` documentation
>   stash: convert store to builtin
>   stash: convert create to builtin
>   stash: replace spawning a "read-tree" process
>   stash: avoid spawning a "diff-index" process
>   stash: convert push to builtin
>   stash: make push to be quiet
>   stash: add tests for `git stash push -q`
>   stash: replace spawning `git ls-files` child process
>   stash: convert save to builtin
>   stash: convert `stash--helper.c` into `stash.c`
>   stash: optimize `get_untracked_files()` and `check_changes()`
>   stash: replace all `write-tree` child processes with API calls
>   stash: replace all "git apply" child processes with API calls
> 
>  Documentation/git-stash.txt |    7 +-
>  Makefile                    |    2 +-
>  builtin.h                   |    1 +
>  builtin/stash.c             | 1562 +++++++++++++++++++++++++++++++++++
>  cache.h                     |    1 +
>  git-stash.sh                |  752 -----------------
>  git.c                       |    1 +
>  sha1-name.c                 |   19 +
>  t/t3903-stash.sh            |  190 +++--
>  9 files changed, 1714 insertions(+), 821 deletions(-)
>  create mode 100644 builtin/stash.c
>  delete mode 100755 git-stash.sh
> 
> -- 
> 2.18.0.573.g56500d98f
>