{"thread":{"id":"58079","subject":"[PATCH] add --interactive: allow `update` to stage deleted files","startedAt":"2022-06-28T22:25:29Z","lastAt":"2022-06-29T10:33:33Z","messageCount":4,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Taylor Blau","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"458077","messageId":"pull.1273.git.1656454964378.gitgitgadget@gmail.com","threadId":"58079","inReplyTo":null,"subject":"[PATCH] add --interactive: allow `update` to stage deleted files","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-28T22:22:44Z","receivedAt":"2022-06-28T22:25:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThe scripted version of `git add -i` used `git update-index --add\n--remove`, but the built-in version implemented only the `--add` part.\n\nThis fixes https://github.com/msys2/MSYS2-packages/issues/3066\n\nReported-by: Christoph Reiter <reiter.christoph@gmail.com>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    add --interactive: fix bug in built-in variant\n    \n    This fixes a bug when using the built-in version of git add -i to update\n    a file that has been deleted (in order to stage its deletion), where it\n    fails with:\n    \n    fatal: unable to stat 'myfile': No such file or directory\n    \n    \n    Since the built-in version of git add -i has been made the default in\n    v2.37.0, from the users' point of view this is a regression, and this\n    patch fixes it. I therefore consider this v2.37.1 material.\n    \n    This addresses https://github.com/msys2/MSYS2-packages/issues/3066\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1273%2Fdscho%2Fadd-i-update-deleted-file-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1273/dscho/add-i-update-deleted-file-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1273\n\n add-interactive.c          | 12 ++++++++++--\n t/t3701-add-interactive.sh |  9 +++++++++\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 6047e8f6489..22fcd3412ca 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -697,8 +697,16 @@ static int run_update(struct add_i_state *s, const struct pathspec *ps,\n \n \tfor (i = 0; i < files->items.nr; i++) {\n \t\tconst char *name = files->items.items[i].string;\n-\t\tif (files->selected[i] &&\n-\t\t    add_file_to_index(s->r->index, name, 0) < 0) {\n+\t\tstruct stat st;\n+\n+\t\tif (!files->selected[i])\n+\t\t\tcontinue;\n+\t\tif (lstat(name, &st) && is_missing_file_error(errno)) {\n+\t\t\tif (remove_file_from_index(s->r->index, name) < 0) {\n+\t\t\t\tres = error(_(\"could not stage '%s'\"), name);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t} else if (add_file_to_index(s->r->index, name, 0) < 0) {\n \t\t\tres = error(_(\"could not stage '%s'\"), name);\n \t\t\tbreak;\n \t\t}\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex fc26cb8bae8..b354fb39de8 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -103,6 +103,15 @@ test_expect_success 'status works (commit)' '\n \tgrep \"+1/-0 *+2/-0 file\" output\n '\n \n+test_expect_success 'update can stage deletions' '\n+\t>to-delete &&\n+\tgit add to-delete &&\n+\trm to-delete &&\n+\ttest_write_lines u t \"\" | git add -i &&\n+\tgit ls-files to-delete >output &&\n+\ttest_must_be_empty output\n+'\n+\n test_expect_success 'setup expected' '\n \tcat >expected <<-\\EOF\n \tindex 180b47c..b6f2c08 100644\n\nbase-commit: e4a4b31577c7419497ac30cebe30d755b97752c5\n-- \ngitgitgadget\n"},{"id":"458078","messageId":"xmqqr138h0wn.fsf@gitster.g","threadId":"58079","inReplyTo":"pull.1273.git.1656454964378.gitgitgadget@gmail.com","subject":"Re: [PATCH] add --interactive: allow `update` to stage deleted files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-28T22:35:20Z","receivedAt":"2022-06-28T22:35:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> diff --git a/add-interactive.c b/add-interactive.c\n> index 6047e8f6489..22fcd3412ca 100644\n> --- a/add-interactive.c\n> +++ b/add-interactive.c\n> @@ -697,8 +697,16 @@ static int run_update(struct add_i_state *s, const struct pathspec *ps,\n>  \n>  \tfor (i = 0; i < files->items.nr; i++) {\n>  \t\tconst char *name = files->items.items[i].string;\n> -\t\tif (files->selected[i] &&\n> -\t\t    add_file_to_index(s->r->index, name, 0) < 0) {\n> +\t\tstruct stat st;\n> +\n> +\t\tif (!files->selected[i])\n> +\t\t\tcontinue;\n> +\t\tif (lstat(name, &st) && is_missing_file_error(errno)) {\n> +\t\t\tif (remove_file_from_index(s->r->index, name) < 0) {\n> +\t\t\t\tres = error(_(\"could not stage '%s'\"), name);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t} else if (add_file_to_index(s->r->index, name, 0) < 0) {\n>  \t\t\tres = error(_(\"could not stage '%s'\"), name);\n>  \t\t\tbreak;\n>  \t\t}\n\nThe fix looks quite straight-forward.  It is somewhat disappointing\nthat it was caught after this loooong time, but as they say, better\nlate than never.\n\nThanks for a quick report and a fix.\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index fc26cb8bae8..b354fb39de8 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -103,6 +103,15 @@ test_expect_success 'status works (commit)' '\n>  \tgrep \"+1/-0 *+2/-0 file\" output\n>  '\n>  \n> +test_expect_success 'update can stage deletions' '\n> +\t>to-delete &&\n> +\tgit add to-delete &&\n> +\trm to-delete &&\n> +\ttest_write_lines u t \"\" | git add -i &&\n> +\tgit ls-files to-delete >output &&\n> +\ttest_must_be_empty output\n> +'\n> +\n>  test_expect_success 'setup expected' '\n>  \tcat >expected <<-\\EOF\n>  \tindex 180b47c..b6f2c08 100644\n>\n> base-commit: e4a4b31577c7419497ac30cebe30d755b97752c5\n"},{"id":"458079","messageId":"YruJ06odaXmTOyjM@nand.local","threadId":"58079","inReplyTo":"xmqqr138h0wn.fsf@gitster.g","subject":"Re: [PATCH] add --interactive: allow `update` to stage deleted files","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-28T23:08:03Z","receivedAt":"2022-06-28T23:08:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 28, 2022 at 03:35:20PM -0700, Junio C Hamano wrote:\n> The fix looks quite straight-forward.  It is somewhat disappointing\n> that it was caught after this loooong time, but as they say, better\n> late than never.\n\nI had the same thought, but I think that it points to something I've\nsuspected privately for a while which is that we don't actually get many\nusers to opt-in to features early when they are hidden behind a\nconfiguration.\n\nI had hoped that things like feature.experimental would cause more users\nto try features that weren't quite ready for prime-time more often, and\nthat we'd get more and better feedback as a result.\n\nBut I think that this proves that is generally not so. I think we\nshould probably err on the side of enabling new features by default so\nlong as there is sufficiently low risk, rather than hide them behind\nconfig. Or at least, not hide them behind a config variable for so long\n(though I am guilty of this myself with the pack.writeReverseIndex,\nwhich I have been meaning to flip the default on for a little while\nnow).\n\nIn either case...\n\n> Thanks for a quick report and a fix.\n\nYes, indeed. This fix looks good to me.\n\n\nThanks,\nTaylor\n"},{"id":"458086","messageId":"220629.867d4zg3nu.gmgdl@evledraar.gmail.com","threadId":"58079","inReplyTo":"YruJ06odaXmTOyjM@nand.local","subject":"Re: [PATCH] add --interactive: allow `update` to stage deleted files","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-29T10:22:07Z","receivedAt":"2022-06-29T10:33:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 28 2022, Taylor Blau wrote:\n\n> On Tue, Jun 28, 2022 at 03:35:20PM -0700, Junio C Hamano wrote:\n>> The fix looks quite straight-forward.  It is somewhat disappointing\n>> that it was caught after this loooong time, but as they say, better\n>> late than never.\n>\n> I had the same thought, but I think that it points to something I've\n> suspected privately for a while which is that we don't actually get many\n> users to opt-in to features early when they are hidden behind a\n> configuration.\n>\n> I had hoped that things like feature.experimental would cause more users\n> to try features that weren't quite ready for prime-time more often, and\n> that we'd get more and better feedback as a result.\n>\n> But I think that this proves that is generally not so. I think we\n> should probably err on the side of enabling new features by default so\n> long as there is sufficiently low risk, rather than hide them behind\n> config. Or at least, not hide them behind a config variable for so long\n> (though I am guilty of this myself with the pack.writeReverseIndex,\n> which I have been meaning to flip the default on for a little while\n> now).\n\nI think we could get quite far by piggy-backing on the advice we emit\nwhen you haven't set user.{email,name}, or otherwise find some minimally\nannoying way to ask users to opt-in.\n\nBut while an opt-out being the default isn't quite the same as just\nchanging the code (as you can still toggle it off without downgrading),\nthat also has the disadvantage that we'd be more conservative about\nadding such features, which is partially why we have feature.* in the\nfirst place.\n\nThe X-Y problem of how we get early code to users who'd be able to tell\nus that it breaks has been discussed several times, and all the\nsolutions are trade-offs.\n\nOne thing I thought of now but I don't think has been brought up before:\nI wonder if doing \"more stable\" (or \"LTS\"?) releases would be a good\ncompromise between \"early bird\" and long-term stability, where we'd just\nmake every Nth release (say those divisible by 5) have\nfeature.experimental=false.\n\nBut any such scheme also quickly runs into the problem that we're just\nnot adding a lot of these, and for e.g. the built-in conversion we might\ndo the whole feature cycle in 3-4 releases, so such \"stable\" releases\nwould force us to keep the code around longer.\n\n"}]}