Two fixes for the built-in git add -i
cover Two fixes for the built-in git add -i
These issues were noticed in
https://github.com/git-for-windows/git/issues/2466, fulfilling my hope that Git for Windows' exposure of the built-in git add -i/git add -p as an experimental option would attract testers.
Johannes Schindelin (2):
built-in add -i: do not try to patch/diff an empty list of files built-in add -i: accept open-ended ranges again
add-interactive.c | 9 ++++++--- t/t3701-add-interactive.sh | 9 +++++++++ 2 files changed, 15 insertions(+), 3 deletions(-)
base-commit: 2e697ced9d647d6998d70f010d582ba8019fe3af
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-528%2Fdscho%2Fbuiltin-add-i-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-528/dscho/builtin-add-i-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/528
Signature · 1 line
gitgitgadget
PATCH 1/2 built-in add -i: do not try to `patch`/`diff` an empty list of files
From: Johannes Schindelin <[email protected]>
When the user does not select any files to patch or diff, there is no need to call run_add_p() on them.
Even worse: we _have_ to avoid calling parse_pathspec() with an empty list because that would trigger this error:
BUG: pathspec.c:557: PATHSPEC_PREFER_CWD requires arguments
So let's avoid doing any work on a list of files that is empty anyway.
This fixes https://github.com/git-for-windows/git/issues/2466.
--- add-interactive.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
add-interactive.c +2 -2
@@ -915,7 +915,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps, opts->prompt =N_ ("Patch update" ); count =list_and_choose (s, files, opts); -if (count >=0 ) { +if (count >0 ) {struct argv_array args =ARGV_ARRAY_INIT ; argv_array_pushl(&args, "git", "add--interactive", "--patch",
@@ -953,7 +953,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps, opts->flags =IMMEDIATE ; count =list_and_choose (s, files, opts); opts->flags =0 ; -if (count >=0 ) { +if (count >0 ) {struct argv_array args =ARGV_ARRAY_INIT ; argv_array_pushl(&args, "git", "diff", "-p", "--cached",
Signature · 1 line
gitgitgadget
PATCH 2/2 built-in add -i: accept open-ended ranges again
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
The interactive add command allows selecting multiple files for some of its sub-commands, via unique prefixes, indices or index ranges.
When re-implementing git add -i in C, we even added a code comment talking about ranges with a missing end index, such as 2-, but the code did not actually accept those, as pointed out in
https://github.com/git-for-windows/git/issues/2466#issuecomment-574142760.
Let's fix this, and add a test case to verify that this stays fixed forever.
--- add-interactive.c | 5 ++++- t/t3701-add-interactive.sh | 9 +++++++++ 2 files changed, 13 insertions(+), 1 deletion(-)
add-interactive.c +4 -1
@@ -328,7 +328,10 @@ static ssize_t list_and_choose(struct add_i_state *s,if (endp == p + sep) to = from +1 ;else if (*endp =='-' ) { - to =strtoul (++endp, &endp,10 ); +if (isdigit (*(++endp))) + to =strtoul (endp, &endp,10 ); +else + to = items->items.nr;/* extra characters after the range? */ if (endp != p + sep) from =-1 ;
t/t3701-add-interactive.sh +9 -0
@@ -57,6 +57,15 @@ test_expect_success 'revert works (initial)' ' ! grep . output ' +test_expect_success 'add untracked (multiple)' ' + test_when_finished "git reset && rm [1-9]" && + touch $(test_seq 9) && + test_write_lines a "2-5 8-" | git add -i -- [1-9] && + test_write_lines 2 3 4 5 8 9 >expected && + git ls-files [1-9] >output && + test_cmp expected output +' + test_expect_success 'setup (commit)' ' echo baseline >file && git add file &&
Signature · 1 line
gitgitgadget
Re: [PATCH 1/2] built-in add -i: do not try to `patch`/`diff` an empty list of files
In reply to Johannes Schindelin via GitGitGadget"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 10 lines · writes:
From: Johannes Schindelin <[email protected]>
When the user does not select any files to
patchordiff, there is no need to callrun_add_p()on them.Even worse: we _have_ to avoid calling
parse_pathspec()with an empty list because that would trigger this error:BUG: pathspec.c:557: PATHSPEC_PREFER_CWD requires arguments
Quoted 8 lines
So let's avoid doing any work on a list of files that is empty anyway.
This fixes https://github.com/git-for-windows/git/issues/2466.
Signed-off-by Johannes Schindelin <[email protected]>--- add-interactive.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Makes sense. No tests?
Quoted 23 lines
add-interactive.c +2
-2@@ -915,7 +915,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps, opts->prompt =N_ ("Patch update" ); count =list_and_choose (s, files, opts); -if (count >=0 ) { +if (count >0 ) {struct argv_array args =ARGV_ARRAY_INIT ; argv_array_pushl(&args, "git", "add--interactive", "--patch",@@ -953,7 +953,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps, opts->flags =IMMEDIATE ; count =list_and_choose (s, files, opts); opts->flags =0 ; -if (count >=0 ) { +if (count >0 ) {struct argv_array args =ARGV_ARRAY_INIT ; argv_array_pushl(&args, "git", "diff", "-p", "--cached",
Re: [PATCH 2/2] built-in add -i: accept open-ended ranges again
In reply to Johannes Schindelin via GitGitGadget"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 24 lines · writes:
From: Johannes Schindelin <[email protected]>
The interactive
addcommand allows selecting multiple files for some of its sub-commands, via unique prefixes, indices or index ranges.When re-implementing
git add -iin C, we even added a code comment talking about ranges with a missing end index, such as2-, but the code did not actually accept those, as pointed out in
https://github.com/git-for-windows/git/issues/2466#issuecomment-574142760.Let's fix this, and add a test case to verify that this stays fixed forever.
Signed-off-by Johannes Schindelin <[email protected]>--- add-interactive.c | 5 ++++- t/t3701-add-interactive.sh | 9 +++++++++ 2 files changed, 13 insertions(+), 1 deletion(-)add-interactive.c +0
-0
add-interactive.c static ssize_t list_and_choose(struct add_i_state *s, · line 334
Quoted 8 lines
add-interactive.c +4
-1@@ -328,7 +328,10 @@ static ssize_t list_and_choose(struct add_i_state *s,if (endp == p + sep) to = from +1 ;else if (*endp =='-' ) { - to =strtoul (++endp, &endp,10 ); +if (isdigit (*(++endp))) + to =strtoul (endp, &endp,10 ); +else + to = items->items.nr;
Good. We do not allow "everything up to N" with "-N", so covering "N and everything after" with "N-" is sufficient.
Quoted 20 lines
t/t3701-add-interactive.sh +9
-0@@ -57,6 +57,15 @@ test_expect_success 'revert works (initial)' ' ! grep . output ' +test_expect_success 'add untracked (multiple)' ' + test_when_finished "git reset && rm [1-9]" && + touch $(test_seq 9) && + test_write_lines a "2-5 8-" | git add -i -- [1-9] && + test_write_lines 2 3 4 5 8 9 >expected && + git ls-files [1-9] >output && + test_cmp expected output +' + test_expect_success 'setup (commit)' ' echo baseline >file && git add file &&
Re: [PATCH 2/2] built-in add -i: accept open-ended ranges again
Hi Junio,
Quoted 22 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 19 lines · writes:
From: Johannes Schindelin <[email protected]>
The interactive
addcommand allows selecting multiple files for some of its sub-commands, via unique prefixes, indices or index ranges.When re-implementing
git add -iin C, we even added a code comment talking about ranges with a missing end index, such as2-, but the code did not actually accept those, as pointed out in
https://github.com/git-for-windows/git/issues/2466#issuecomment-574142760.Let's fix this, and add a test case to verify that this stays fixed forever.
Signed-off-by Johannes Schindelin <[email protected]>--- add-interactive.c | 5 ++++- t/t3701-add-interactive.sh | 9 +++++++++ 2 files changed, 13 insertions(+), 1 deletion(-)
Quoted 16 lines
add-interactive.c static ssize_t list_and_choose(struct add_i_state *s, · line 334
Quoted 13 lines
add-interactive.c +4
-1@@ -328,7 +328,10 @@ static ssize_t list_and_choose(struct add_i_state *s,if (endp == p + sep) to = from +1 ;else if (*endp =='-' ) { - to =strtoul (++endp, &endp,10 ); +if (isdigit (*(++endp))) + to =strtoul (endp, &endp,10 ); +else + to = items->items.nr;Good. We do not allow "everything up to N" with "-N", so covering "N and everything after" with "N-" is sufficient.
Even worse, -N means "toggle N off". But that can't be fixed easily as it has been part of the UI for ages.
Thanks,
Dscho
Quoted 22 lines
Quoted 21 lines
t/t3701-add-interactive.sh +9
-0@@ -57,6 +57,15 @@ test_expect_success 'revert works (initial)' ' ! grep . output ' +test_expect_success 'add untracked (multiple)' ' + test_when_finished "git reset && rm [1-9]" && + touch $(test_seq 9) && + test_write_lines a "2-5 8-" | git add -i -- [1-9] && + test_write_lines 2 3 4 5 8 9 >expected && + git ls-files [1-9] >output && + test_cmp expected output +' + test_expect_success 'setup (commit)' ' echo baseline >file && git add file &&
Re: [PATCH 1/2] built-in add -i: do not try to `patch`/`diff` an empty list of files
In reply to Junio C HamanoHi Junio,
Quoted 15 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 12 lines · writes:
From: Johannes Schindelin <[email protected]>
When the user does not select any files to
patchordiff, there is no need to callrun_add_p()on them.Even worse: we _have_ to avoid calling
parse_pathspec()with an empty list because that would trigger this error:BUG: pathspec.c:557: PATHSPEC_PREFER_CWD requires argumentsSo let's avoid doing any work on a list of files that is empty anyway.
Quoted 8 lines
Quoted 6 lines
This fixes https://github.com/git-for-windows/git/issues/2466.
Signed-off-by Johannes Schindelin <[email protected]>--- add-interactive.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)Makes sense. No tests?
Do we really need tests here? ;-)
Honestly, I would deem the likelihood of this to get broken again very low, and the effort for writing a regression test relatively high...
If you insist, I will add some, of course, but not today.
Ciao,
Dscho
Quoted 25 lines
Quoted 24 lines
add-interactive.c +2
-2@@ -915,7 +915,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps, opts->prompt =N_ ("Patch update" ); count =list_and_choose (s, files, opts); -if (count >=0 ) { +if (count >0 ) {struct argv_array args =ARGV_ARRAY_INIT ; argv_array_pushl(&args, "git", "add--interactive", "--patch",@@ -953,7 +953,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps, opts->flags =IMMEDIATE ; count =list_and_choose (s, files, opts); opts->flags =0 ; -if (count >=0 ) { +if (count >0 ) {struct argv_array args =ARGV_ARRAY_INIT ; argv_array_pushl(&args, "git", "diff", "-p", "--cached",
Re: [PATCH 2/2] built-in add -i: accept open-ended ranges again
In reply to Johannes SchindelinQuoted 5 lines · Johannes Schindelin
Quoted 2 lines
Good. We do not allow "everything up to N" with "-N", so covering "N and everything after" with "N-" is sufficient.
Even worse,
-Nmeans "toggle N off". But that can't be fixed easily as it has been part of the UI for ages.
There is no loss. "N-" is useful because you do not necessarily know what the maximum is, but "everything up to N" will always be "1-N" and you do not know what the minimum is.
So, there is not even worse there at all ;-)