Re: [GSoC][PATCH v7 00/26] Convert "git stash" to C builtin
Hi, On Thu, Aug 16, 2018 at 1:25 AM, Thomas Gummerer 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
Re: [GSoC][PATCH v7 00/26] Convert "git stash" to C builtin
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. 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! > 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! > * 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 + >
[GSoC][PATCH v7 00/26] Convert "git stash" to C builtin
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"). 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 * 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
