* [PATCH 0/2] Two fixes for the built-in git add -i @ 2020-01-16 8:33 Johannes Schindelin via GitGitGadget 2020-01-16 8:33 ` [PATCH 1/2] built-in add -i: do not try to `patch`/`diff` an empty list of files Johannes Schindelin via GitGitGadget 2020-01-16 8:33 ` [PATCH 2/2] built-in add -i: accept open-ended ranges again Johannes Schindelin via GitGitGadget 0 siblings, 2 replies; 8+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2020-01-16 8:33 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin 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 -- gitgitgadget ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] built-in add -i: do not try to `patch`/`diff` an empty list of files 2020-01-16 8:33 [PATCH 0/2] Two fixes for the built-in git add -i Johannes Schindelin via GitGitGadget @ 2020-01-16 8:33 ` Johannes Schindelin via GitGitGadget 2020-01-16 22:13 ` Junio C Hamano 2020-01-16 8:33 ` [PATCH 2/2] built-in add -i: accept open-ended ranges again Johannes Schindelin via GitGitGadget 1 sibling, 1 reply; 8+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2020-01-16 8:33 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> 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. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- add-interactive.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index f395d54c08..14d4688c26 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -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", -- gitgitgadget ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] built-in add -i: do not try to `patch`/`diff` an empty list of files 2020-01-16 8:33 ` [PATCH 1/2] built-in add -i: do not try to `patch`/`diff` an empty list of files Johannes Schindelin via GitGitGadget @ 2020-01-16 22:13 ` Junio C Hamano 2020-01-17 10:03 ` Johannes Schindelin 0 siblings, 1 reply; 8+ messages in thread From: Junio C Hamano @ 2020-01-16 22:13 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > 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. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > add-interactive.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Makes sense. No tests? > > diff --git a/add-interactive.c b/add-interactive.c > index f395d54c08..14d4688c26 100644 > --- a/add-interactive.c > +++ b/add-interactive.c > @@ -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", ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] built-in add -i: do not try to `patch`/`diff` an empty list of files 2020-01-16 22:13 ` Junio C Hamano @ 2020-01-17 10:03 ` Johannes Schindelin 0 siblings, 0 replies; 8+ messages in thread From: Johannes Schindelin @ 2020-01-17 10:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git Hi Junio, On Thu, 16 Jan 2020, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > 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. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > 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 > > > > > diff --git a/add-interactive.c b/add-interactive.c > > index f395d54c08..14d4688c26 100644 > > --- a/add-interactive.c > > +++ b/add-interactive.c > > @@ -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", > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] built-in add -i: accept open-ended ranges again 2020-01-16 8:33 [PATCH 0/2] Two fixes for the built-in git add -i Johannes Schindelin via GitGitGadget 2020-01-16 8:33 ` [PATCH 1/2] built-in add -i: do not try to `patch`/`diff` an empty list of files Johannes Schindelin via GitGitGadget @ 2020-01-16 8:33 ` Johannes Schindelin via GitGitGadget 2020-01-16 22:16 ` Junio C Hamano 1 sibling, 1 reply; 8+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2020-01-16 8:33 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> 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. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- add-interactive.c | 5 ++++- t/t3701-add-interactive.sh | 9 +++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/add-interactive.c b/add-interactive.c index 14d4688c26..396066e724 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -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; diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index d4f9386621..b02fe73631 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -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 && -- gitgitgadget ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] built-in add -i: accept open-ended ranges again 2020-01-16 8:33 ` [PATCH 2/2] built-in add -i: accept open-ended ranges again Johannes Schindelin via GitGitGadget @ 2020-01-16 22:16 ` Junio C Hamano 2020-01-17 10:01 ` Johannes Schindelin 0 siblings, 1 reply; 8+ messages in thread From: Junio C Hamano @ 2020-01-16 22:16 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > 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. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > add-interactive.c | 5 ++++- > t/t3701-add-interactive.sh | 9 +++++++++ > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/add-interactive.c b/add-interactive.c > index 14d4688c26..396066e724 100644 > --- a/add-interactive.c > +++ b/add-interactive.c > @@ -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. > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index d4f9386621..b02fe73631 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -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 && ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] built-in add -i: accept open-ended ranges again 2020-01-16 22:16 ` Junio C Hamano @ 2020-01-17 10:01 ` Johannes Schindelin 2020-01-17 17:35 ` Junio C Hamano 0 siblings, 1 reply; 8+ messages in thread From: Johannes Schindelin @ 2020-01-17 10:01 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git Hi Junio, On Thu, 16 Jan 2020, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > 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. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > add-interactive.c | 5 ++++- > > t/t3701-add-interactive.sh | 9 +++++++++ > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/add-interactive.c b/add-interactive.c > > index 14d4688c26..396066e724 100644 > > --- a/add-interactive.c > > +++ b/add-interactive.c > > @@ -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 > > > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > > index d4f9386621..b02fe73631 100755 > > --- a/t/t3701-add-interactive.sh > > +++ b/t/t3701-add-interactive.sh > > @@ -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 && > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] built-in add -i: accept open-ended ranges again 2020-01-17 10:01 ` Johannes Schindelin @ 2020-01-17 17:35 ` Junio C Hamano 0 siblings, 0 replies; 8+ messages in thread From: Junio C Hamano @ 2020-01-17 17:35 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> 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. 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 ;-) ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-01-17 17:35 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-16 8:33 [PATCH 0/2] Two fixes for the built-in git add -i Johannes Schindelin via GitGitGadget 2020-01-16 8:33 ` [PATCH 1/2] built-in add -i: do not try to `patch`/`diff` an empty list of files Johannes Schindelin via GitGitGadget 2020-01-16 22:13 ` Junio C Hamano 2020-01-17 10:03 ` Johannes Schindelin 2020-01-16 8:33 ` [PATCH 2/2] built-in add -i: accept open-ended ranges again Johannes Schindelin via GitGitGadget 2020-01-16 22:16 ` Junio C Hamano 2020-01-17 10:01 ` Johannes Schindelin 2020-01-17 17:35 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).