{"thread":{"id":"52646","subject":"[PATCH 0/2] Two fixes for the built-in git add -i","startedAt":"2020-01-16T08:33:12Z","lastAt":"2020-01-17T17:35:17Z","messageCount":8,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"389882","messageId":"pull.528.git.1579163587.gitgitgadget@gmail.com","threadId":"52646","inReplyTo":null,"subject":"[PATCH 0/2] Two fixes for the built-in git add -i","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-16T08:33:05Z","receivedAt":"2020-01-16T08:33:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"These issues were noticed in \nhttps://github.com/git-for-windows/git/issues/2466, fulfilling my hope that\nGit for Windows' exposure of the built-in git add -i/git add -p as an\nexperimental option would attract testers.\n\nJohannes Schindelin (2):\n built-in add -i: do not try to `patch`/`diff` an empty list of files\n built-in add -i: accept open-ended ranges again\n\n add-interactive.c | 9 ++++++---\n t/t3701-add-interactive.sh | 9 +++++++++\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\n\nbase-commit: 2e697ced9d647d6998d70f010d582ba8019fe3af\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-528%2Fdscho%2Fbuiltin-add-i-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-528/dscho/builtin-add-i-fixes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/528\n-- \ngitgitgadget\n"},{"id":"389883","messageId":"353c748838d341bb325149234657c27215a9fab3.1579163587.git.gitgitgadget@gmail.com","threadId":"52646","inReplyTo":"pull.528.git.1579163587.gitgitgadget@gmail.com","subject":"[PATCH 1/2] built-in add -i: do not try to `patch`/`diff` an empty list of files","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-16T08:33:06Z","receivedAt":"2020-01-16T08:33:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen the user does not select any files to `patch` or `diff`, there is\nno need to call `run_add_p()` on them.\n\nEven worse: we _have_ to avoid calling `parse_pathspec()` with an empty\nlist because that would trigger this error:\n\n\tBUG: pathspec.c:557: PATHSPEC_PREFER_CWD requires arguments\n\nSo let's avoid doing any work on a list of files that is empty anyway.\n\nThis fixes https://github.com/git-for-windows/git/issues/2466.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex f395d54c08..14d4688c26 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -915,7 +915,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps,\n \n \topts->prompt = N_(\"Patch update\");\n \tcount = list_and_choose(s, files, opts);\n-\tif (count >= 0) {\n+\tif (count > 0) {\n \t\tstruct argv_array args = ARGV_ARRAY_INIT;\n \n \t\targv_array_pushl(&args, \"git\", \"add--interactive\", \"--patch\",\n@@ -953,7 +953,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,\n \topts->flags = IMMEDIATE;\n \tcount = list_and_choose(s, files, opts);\n \topts->flags = 0;\n-\tif (count >= 0) {\n+\tif (count > 0) {\n \t\tstruct argv_array args = ARGV_ARRAY_INIT;\n \n \t\targv_array_pushl(&args, \"git\", \"diff\", \"-p\", \"--cached\",\n-- \ngitgitgadget\n\n"},{"id":"389884","messageId":"7e4bf4bbbcd8c81a19d690aee379042e47246947.1579163587.git.gitgitgadget@gmail.com","threadId":"52646","inReplyTo":"pull.528.git.1579163587.gitgitgadget@gmail.com","subject":"[PATCH 2/2] built-in add -i: accept open-ended ranges again","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-16T08:33:07Z","receivedAt":"2020-01-16T08:33:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe interactive `add` command allows selecting multiple files for some\nof its sub-commands, via unique prefixes, indices or index ranges.\n\nWhen re-implementing `git add -i` in C, we even added a code comment\ntalking about ranges with a missing end index, such as `2-`, but the\ncode did not actually accept those, as pointed out in\nhttps://github.com/git-for-windows/git/issues/2466#issuecomment-574142760.\n\nLet's fix this, and add a test case to verify that this stays fixed\nforever.\n\nSigned-off-by: Johannes Schindelin \n---\n add-interactive.c | 5 ++++-\n t/t3701-add-interactive.sh | 9 +++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 14d4688c26..396066e724 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -328,7 +328,10 @@ static ssize_t list_and_choose(struct add_i_state *s,\n \t\t\t\tif (endp == p + sep)\n \t\t\t\t\tto = from + 1;\n \t\t\t\telse if (*endp == '-') {\n-\t\t\t\t\tto = strtoul(++endp, &endp, 10);\n+\t\t\t\t\tif (isdigit(*(++endp)))\n+\t\t\t\t\t\tto = strtoul(endp, &endp, 10);\n+\t\t\t\t\telse\n+\t\t\t\t\t\tto = items->items.nr;\n \t\t\t\t\t/* extra characters after the range? */\n \t\t\t\t\tif (endp != p + sep)\n \t\t\t\t\t\tfrom = -1;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex d4f9386621..b02fe73631 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -57,6 +57,15 @@ test_expect_success 'revert works (initial)' '\n \t! grep . output\n '\n \n+test_expect_success 'add untracked (multiple)' '\n+\ttest_when_finished \"git reset && rm [1-9]\" &&\n+\ttouch $(test_seq 9) &&\n+\ttest_write_lines a \"2-5 8-\" | git add -i -- [1-9] &&\n+\ttest_write_lines 2 3 4 5 8 9 >expected &&\n+\tgit ls-files [1-9] >output &&\n+\ttest_cmp expected output\n+'\n+\n test_expect_success 'setup (commit)' '\n \techo baseline >file &&\n \tgit add file &&\n-- \ngitgitgadget\n"},{"id":"389944","messageId":"xmqqzhenf3t0.fsf@gitster-ct.c.googlers.com","threadId":"52646","inReplyTo":"353c748838d341bb325149234657c27215a9fab3.1579163587.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] built-in add -i: do not try to `patch`/`diff` an empty list of files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-16T22:13:15Z","receivedAt":"2020-01-16T22:13:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> When the user does not select any files to `patch` or `diff`, there is\n> no need to call `run_add_p()` on them.\n>\n> Even worse: we _have_ to avoid calling `parse_pathspec()` with an empty\n> list because that would trigger this error:\n>\n> \tBUG: pathspec.c:557: PATHSPEC_PREFER_CWD requires arguments\n>\n> So let's avoid doing any work on a list of files that is empty anyway.\n>\n> This fixes https://github.com/git-for-windows/git/issues/2466.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> add-interactive.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n\nMakes sense. No tests?\n\n>\n> diff --git a/add-interactive.c b/add-interactive.c\n> index f395d54c08..14d4688c26 100644\n> --- a/add-interactive.c\n> +++ b/add-interactive.c\n> @@ -915,7 +915,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps,\n> \n> \topts->prompt = N_(\"Patch update\");\n> \tcount = list_and_choose(s, files, opts);\n> -\tif (count >= 0) {\n> +\tif (count > 0) {\n> \t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> \n> \t\targv_array_pushl(&args, \"git\", \"add--interactive\", \"--patch\",\n> @@ -953,7 +953,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,\n> \topts->flags = IMMEDIATE;\n> \tcount = list_and_choose(s, files, opts);\n> \topts->flags = 0;\n> -\tif (count >= 0) {\n> +\tif (count > 0) {\n> \t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> \n> \t\targv_array_pushl(&args, \"git\", \"diff\", \"-p\", \"--cached\",\n"},{"id":"389945","messageId":"xmqqv9pbf3mu.fsf@gitster-ct.c.googlers.com","threadId":"52646","inReplyTo":"7e4bf4bbbcd8c81a19d690aee379042e47246947.1579163587.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] built-in add -i: accept open-ended ranges again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-16T22:16:57Z","receivedAt":"2020-01-16T22:17:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> The interactive `add` command allows selecting multiple files for some\n> of its sub-commands, via unique prefixes, indices or index ranges.\n>\n> When re-implementing `git add -i` in C, we even added a code comment\n> talking about ranges with a missing end index, such as `2-`, but the\n> code did not actually accept those, as pointed out in\n> https://github.com/git-for-windows/git/issues/2466#issuecomment-574142760.\n>\n> Let's fix this, and add a test case to verify that this stays fixed\n> forever.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> add-interactive.c | 5 ++++-\n> t/t3701-add-interactive.sh | 9 +++++++++\n> 2 files changed, 13 insertions(+), 1 deletion(-)\n>\n> diff --git a/add-interactive.c b/add-interactive.c\n> index 14d4688c26..396066e724 100644\n> --- a/add-interactive.c\n> +++ b/add-interactive.c\n> @@ -328,7 +328,10 @@ static ssize_t list_and_choose(struct add_i_state *s,\n> \t\t\t\tif (endp == p + sep)\n> \t\t\t\t\tto = from + 1;\n> \t\t\t\telse if (*endp == '-') {\n> -\t\t\t\t\tto = strtoul(++endp, &endp, 10);\n> +\t\t\t\t\tif (isdigit(*(++endp)))\n> +\t\t\t\t\t\tto = strtoul(endp, &endp, 10);\n> +\t\t\t\t\telse\n> +\t\t\t\t\t\tto = items->items.nr;\n\nGood. We do not allow \"everything up to N\" with \"-N\", so covering\n\"N and everything after\" with \"N-\" is sufficient.\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index d4f9386621..b02fe73631 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -57,6 +57,15 @@ test_expect_success 'revert works (initial)' '\n> \t! grep . output\n> '\n> \n> +test_expect_success 'add untracked (multiple)' '\n> +\ttest_when_finished \"git reset && rm [1-9]\" &&\n> +\ttouch $(test_seq 9) &&\n> +\ttest_write_lines a \"2-5 8-\" | git add -i -- [1-9] &&\n> +\ttest_write_lines 2 3 4 5 8 9 >expected &&\n> +\tgit ls-files [1-9] >output &&\n> +\ttest_cmp expected output\n> +'\n> +\n> test_expect_success 'setup (commit)' '\n> \techo baseline >file &&\n> \tgit add file &&\n"},{"id":"389964","messageId":"nycvar.QRO.7.76.6.2001171100030.46@tvgsbejvaqbjf.bet","threadId":"52646","inReplyTo":"xmqqv9pbf3mu.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] built-in add -i: accept open-ended ranges again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-17T10:01:00Z","receivedAt":"2020-01-17T10:01:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 16 Jan 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > The interactive `add` command allows selecting multiple files for some\n> > of its sub-commands, via unique prefixes, indices or index ranges.\n> >\n> > When re-implementing `git add -i` in C, we even added a code comment\n> > talking about ranges with a missing end index, such as `2-`, but the\n> > code did not actually accept those, as pointed out in\n> > https://github.com/git-for-windows/git/issues/2466#issuecomment-574142760.\n> >\n> > Let's fix this, and add a test case to verify that this stays fixed\n> > forever.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > add-interactive.c | 5 ++++-\n> > t/t3701-add-interactive.sh | 9 +++++++++\n> > 2 files changed, 13 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/add-interactive.c b/add-interactive.c\n> > index 14d4688c26..396066e724 100644\n> > --- a/add-interactive.c\n> > +++ b/add-interactive.c\n> > @@ -328,7 +328,10 @@ static ssize_t list_and_choose(struct add_i_state *s,\n> > \t\t\t\tif (endp == p + sep)\n> > \t\t\t\t\tto = from + 1;\n> > \t\t\t\telse if (*endp == '-') {\n> > -\t\t\t\t\tto = strtoul(++endp, &endp, 10);\n> > +\t\t\t\t\tif (isdigit(*(++endp)))\n> > +\t\t\t\t\t\tto = strtoul(endp, &endp, 10);\n> > +\t\t\t\t\telse\n> > +\t\t\t\t\t\tto = items->items.nr;\n>\n> Good. We do not allow \"everything up to N\" with \"-N\", so covering\n> \"N and everything after\" with \"N-\" is sufficient.\n\nEven worse, `-N` means \"toggle N off\". But that can't be fixed easily as\nit has been part of the UI for ages.\n\nThanks,\nDscho\n\n>\n> > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> > index d4f9386621..b02fe73631 100755\n> > --- a/t/t3701-add-interactive.sh\n> > +++ b/t/t3701-add-interactive.sh\n> > @@ -57,6 +57,15 @@ test_expect_success 'revert works (initial)' '\n> > \t! grep . output\n> > '\n> >\n> > +test_expect_success 'add untracked (multiple)' '\n> > +\ttest_when_finished \"git reset && rm [1-9]\" &&\n> > +\ttouch $(test_seq 9) &&\n> > +\ttest_write_lines a \"2-5 8-\" | git add -i -- [1-9] &&\n> > +\ttest_write_lines 2 3 4 5 8 9 >expected &&\n> > +\tgit ls-files [1-9] >output &&\n> > +\ttest_cmp expected output\n> > +'\n> > +\n> > test_expect_success 'setup (commit)' '\n> > \techo baseline >file &&\n> > \tgit add file &&\n>\n"},{"id":"389965","messageId":"nycvar.QRO.7.76.6.2001171101150.46@tvgsbejvaqbjf.bet","threadId":"52646","inReplyTo":"xmqqzhenf3t0.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] built-in add -i: do not try to `patch`/`diff` an empty list of files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-17T10:03:02Z","receivedAt":"2020-01-17T10:03:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 16 Jan 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > When the user does not select any files to `patch` or `diff`, there is\n> > no need to call `run_add_p()` on them.\n> >\n> > Even worse: we _have_ to avoid calling `parse_pathspec()` with an empty\n> > list because that would trigger this error:\n> >\n> > \tBUG: pathspec.c:557: PATHSPEC_PREFER_CWD requires arguments\n> >\n> > So let's avoid doing any work on a list of files that is empty anyway.\n> >\n> > This fixes https://github.com/git-for-windows/git/issues/2466.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > add-interactive.c | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> Makes sense. No tests?\n\nDo we really need tests here? ;-)\n\nHonestly, I would deem the likelihood of this to get broken again very\nlow, and the effort for writing a regression test relatively high...\n\nIf you insist, I will add some, of course, but not today.\n\nCiao,\nDscho\n\n>\n> >\n> > diff --git a/add-interactive.c b/add-interactive.c\n> > index f395d54c08..14d4688c26 100644\n> > --- a/add-interactive.c\n> > +++ b/add-interactive.c\n> > @@ -915,7 +915,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps,\n> >\n> > \topts->prompt = N_(\"Patch update\");\n> > \tcount = list_and_choose(s, files, opts);\n> > -\tif (count >= 0) {\n> > +\tif (count > 0) {\n> > \t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> >\n> > \t\targv_array_pushl(&args, \"git\", \"add--interactive\", \"--patch\",\n> > @@ -953,7 +953,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,\n> > \topts->flags = IMMEDIATE;\n> > \tcount = list_and_choose(s, files, opts);\n> > \topts->flags = 0;\n> > -\tif (count >= 0) {\n> > +\tif (count > 0) {\n> > \t\tstruct argv_array args = ARGV_ARRAY_INIT;\n> >\n> > \t\targv_array_pushl(&args, \"git\", \"diff\", \"-p\", \"--cached\",\n>\n"},{"id":"390000","messageId":"xmqqv9padm0f.fsf@gitster-ct.c.googlers.com","threadId":"52646","inReplyTo":"nycvar.QRO.7.76.6.2001171100030.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] built-in add -i: accept open-ended ranges again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-17T17:35:12Z","receivedAt":"2020-01-17T17:35:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Good. We do not allow \"everything up to N\" with \"-N\", so covering\n>> \"N and everything after\" with \"N-\" is sufficient.\n>\n> Even worse, `-N` means \"toggle N off\". But that can't be fixed easily as\n> it has been part of the UI for ages.\n\nThere is no loss. \"N-\" is useful because you do not necessarily\nknow what the maximum is, but \"everything up to N\" will always be\n\"1-N\" and you do not know what the minimum is.\n\nSo, there is not even worse there at all ;-)\n\n"}]}