Re: [PATCH v4 0/7] Turn git add-i into built-in

From: Slavica Djukic <slavicadj.ip2018@gmail.com>
To: "Slavica Đukić via GitGitGadget" <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: phillip.wood@dunelm.org.uk
Subject: Re: [PATCH v4 0/7] Turn git add-i into built-in
Date: Fri, 25 Jan 2019 13:37:42 +0100	[thread overview]
Message-ID: <c7d3ce15-567a-20b5-1bde-2a7fdd88c4e5@gmail.com> (raw)
In-Reply-To: <pull.103.v4.git.gitgitgadget@gmail.com>

Hi Phillip,

Sorry for omitting you in Cc, again.
I accidentally wrote yours and Ævar's email prefixed with Cc:, rather 
then separating
them with comma, like Johannes said I should.

-Slavica

On 25-Jan-19 1:23 PM, Slavica Đukić via GitGitGadget wrote:
> This is the first version of a patch series to start porting
> git-add--interactive from Perl to C. Daniel Ferreira's patch series used as
> a head start:
> https://public-inbox.org/git/1494907234-28903-1-git-send-email-bnmvco@gmail.com/t/#u
>
> Changes since v3:
>
>   * add error check when calling add--helper's functions from
>     git-add--interactive.perl
>   * replace trailing whitespace in the test with variable $SP used in earlier
>     tests
>
> Cc: Phillip Wood phillip.wood@dunelm.org.uk [phillip.wood@dunelm.org.uk]
>
> Daniel Ferreira (4):
>    diff: export diffstat interface
>    add--helper: create builtin helper for interactive add
>    add-interactive.c: implement status command
>    add--interactive.perl: use add--helper --status for status_cmd
>
> Slavica Djukic (3):
>    add-interactive.c: implement show-help command
>    t3701-add-interactive: test add_i_show_help()
>    add--interactive.perl: use add--helper --show-help for help_cmd
>
>   .gitignore                 |   1 +
>   Makefile                   |   2 +
>   add-interactive.c          | 263 +++++++++++++++++++++++++++++++++++++
>   add-interactive.h          |  10 ++
>   builtin.h                  |   1 +
>   builtin/add--helper.c      |  43 ++++++
>   diff.c                     |  36 ++---
>   diff.h                     |  18 +++
>   git-add--interactive.perl  |  17 +--
>   git.c                      |   1 +
>   t/t3701-add-interactive.sh |  24 ++++
>   11 files changed, 381 insertions(+), 35 deletions(-)
>   create mode 100644 add-interactive.c
>   create mode 100644 add-interactive.h
>   create mode 100644 builtin/add--helper.c
>
>
> base-commit: b21ebb671bb7dea8d342225f0d66c41f4e54d5ca
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-103%2FslavicaDj%2Fadd-i-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-103/slavicaDj/add-i-v4
> Pull-Request: https://github.com/gitgitgadget/git/pull/103
>
> Range-diff vs v3:
>
>   1:  737767b6f4 = 1:  737767b6f4 diff: export diffstat interface
>   2:  91b1963125 = 2:  91b1963125 add--helper: create builtin helper for interactive add
>   3:  d247ef69fe = 3:  d247ef69fe add-interactive.c: implement status command
>   4:  4950c889aa ! 4:  fb3f9378ac add--interactive.perl: use add--helper --status for status_cmd
>       @@ -2,12 +2,21 @@
>        
>            add--interactive.perl: use add--helper --status for status_cmd
>        
>       -    Call the newly introduced add--helper builtin on
>       +    Call the newly introduced add--helper builtin in
>            status_cmd() instead of relying on add--interactive's Perl
>       -    functions to build print the numstat.
>       +    functions to print the numstat.
>       +
>       +    If an error occurs, it will be reported, but the Perl script will
>       +    not exit, since the add--helper is called within an eval block.
>       +
>       +    As the Perl script will go away soon, so will this scenario, where
>       +    the built-in helper is called  from the Perl script. Combined with
>       +    the fact that it would be hard to test, we'll pass on adding
>       +    a regression test for this.
>        
>            Signed-off-by: Daniel Ferreira <bnmvco@gmail.com>
>            Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
>       +    Signed-off-by: Ã†var ArnfjÃ¶rÃ° Bjarmason <avarab@gmail.com>
>        
>         diff --git a/git-add--interactive.perl b/git-add--interactive.perl
>         --- a/git-add--interactive.perl
>       @@ -19,7 +28,8 @@
>        -	list_and_choose({ LIST_ONLY => 1, HEADER => $status_head },
>        -			list_modified());
>        -	print "\n";
>       -+	system(qw(git add--helper --status));
>       ++	my @status_cmd = ("git", "add--helper", "--status");
>       ++	!system(@status_cmd) or die "@status_cmd exited with code $?";
>         }
>         
>         sub say_n_paths {
>   5:  581b108c9c = 5:  ab16afd1d5 add-interactive.c: implement show-help command
>   6:  aede733318 ! 6:  0a27304a84 t3701-add-interactive: test add_i_show_help()
>       @@ -7,6 +7,7 @@
>            Also, add it before changing git-add--interactive.perl's help_cmd
>            to demonstrate that there are no changes introduced by the
>            conversion to C.
>       +
>            Prefix git add -i call with GIT_PAGER_IN_USE=true TERM=vt100
>            to force colored output on Windows.
>        
>       @@ -21,7 +22,7 @@
>         
>        +test_expect_success 'show help from add--helper' '
>        +	git reset --hard &&
>       -+	cat >expect <<-\EOF &&
>       ++	cat >expect <<-EOF &&
>        +
>        +	<BOLD>*** Commands ***<RESET>
>        +	  1: <BOLD;BLUE>s<RESET>tatus	  2: <BOLD;BLUE>u<RESET>pdate	  3: <BOLD;BLUE>r<RESET>evert	  4: <BOLD;BLUE>a<RESET>dd untracked
>       @@ -35,7 +36,7 @@
>        +	<BOLD>*** Commands ***<RESET>
>        +	  1: <BOLD;BLUE>s<RESET>tatus	  2: <BOLD;BLUE>u<RESET>pdate	  3: <BOLD;BLUE>r<RESET>evert	  4: <BOLD;BLUE>a<RESET>dd untracked
>        +	  5: <BOLD;BLUE>p<RESET>atch	  6: <BOLD;BLUE>d<RESET>iff	  7: <BOLD;BLUE>q<RESET>uit	  8: <BOLD;BLUE>h<RESET>elp
>       -+	<BOLD;BLUE>What now<RESET>>
>       ++	<BOLD;BLUE>What now<RESET>>$SP
>        +	Bye.
>        +	EOF
>        +	test_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored &&
>   7:  b9a1a7e37a ! 7:  ca2a7c4375 add--interactive.perl: use add--helper --show-help for help_cmd
>       @@ -5,7 +5,15 @@
>            Change help_cmd sub in git-add--interactive.perl to use
>            show-help command from builtin add--helper.
>        
>       +    If an error occurs, it will be reported, but the Perl script will
>       +    not exit, since the add--helper is called within an eval block.
>       +
>       +    Just like the change where the Perl script calls the add--helper
>       +    to print the numstat, also here we forgo adding a regression test:
>       +    the Perl script is on its way out (and this patch is part of that journey).
>       +
>            Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
>       +    Signed-off-by: Ã†var ArnfjÃ¶rÃ° Bjarmason <avarab@gmail.com>
>        
>         diff --git a/git-add--interactive.perl b/git-add--interactive.perl
>         --- a/git-add--interactive.perl
>       @@ -24,7 +32,8 @@
>        -diff          - view diff between HEAD and index
>        -add untracked - add contents of untracked files to the staged set of changes
>        -EOF
>       -+	system(qw(git add--helper --show-help));
>       ++	my @help_cmd = ("git", "add--helper", "--show-help");
>       ++	!system(@help_cmd) or die "@help_cmd exited with code $?";
>         }
>         
>         sub process_args {
>