{"thread":{"id":"56789","subject":"git apply --indent-to-add deletes other files from the index","startedAt":"2021-10-26T15:18:51Z","lastAt":"2021-11-06T11:47:15Z","messageCount":12,"participants":["Ryan Hodges (rhodges)","Johannes Altmanninger","Ryan Hodges","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"439650","messageId":"0DB10E05-094D-4382-AD1F-657878B06A80@cisco.com","threadId":"56789","inReplyTo":null,"subject":"git apply --indent-to-add deletes other files from the index","fromName":"Ryan Hodges (rhodges)","fromEmail":"rhodges@cisco.com","sentAt":"2021-10-26T15:11:36Z","receivedAt":"2021-10-26T15:18:51Z","isPatch":false,"sender":{"key":"rhodges@cisco.com","avatar":null},"body":"Hi all,\n \nI’ve got a quick question about ‘git apply –intent-to-add’.  If I’ve got a patch that just adds one file to the tree:\n \n[sjc-ads-2565:t.git]$ git diff\ndiff --git a/c.c b/c.c\nnew file mode 100644\nindex 0000000..9daeafb\n--- /dev/null\n+++ b/c.c\n@@ -0,0 +1 @@\n+test\n \nand I apply that patch with –intent-to-add:\n \n[sjc-ads-2565:t.git]$ git apply --intent-to-add c.diff\n \nThe newly added file is tracked but other files in the tree get marked as deleted:\n \n[sjc-ads-2565:t.git]$ git status\nOn branch master\nChanges to be committed:\n  (use “git restore –staged <file>…” to unstage)\n                deleted:    a.c\n                deleted:    b.c\n \nChanges not staged for commit:\n  (use “git add <file>…” to update what will be committed)\n  (use “git restore <file>…” to discard changes in working directory)\n                new file:   c.c\n \nIt looks like Git created a new index with only the newly added file in the patch.  However, I’d like Git to just add one entry to the index corresponding to the newly added file in the patch.  Is this a bug or am I completely misinterpreting the goal of ‘intent-to-add’.  I just started looking at the source but a quick message from the experts would be much appreciated. \n \nI’m currently testing with Git version 2.33.\n \nRegards,\nRyan\n "},{"id":"440136","messageId":"20211030203916.zopggbajumvb4z3f@gmail.com","threadId":"56789","inReplyTo":"0DB10E05-094D-4382-AD1F-657878B06A80@cisco.com","subject":"Re: git apply --intent-to-add deletes other files from the index","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-10-30T20:39:16Z","receivedAt":"2021-10-30T20:39:24Z","isPatch":false,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Tue, Oct 26, 2021 at 03:11:36PM +0000, Ryan Hodges (rhodges) wrote:\n> Hi all,\n>  \n> I’ve got a quick question about ‘git apply –intent-to-add’.  If I’ve got a patch that just adds one file to the tree:\n>  \n> [sjc-ads-2565:t.git]$ git diff\n> diff --git a/c.c b/c.c\n> new file mode 100644\n> index 0000000..9daeafb\n> --- /dev/null\n> +++ b/c.c\n> @@ -0,0 +1 @@\n> +test\n>  \n> and I apply that patch with –intent-to-add:\n>  \n> [sjc-ads-2565:t.git]$ git apply --intent-to-add c.diff\n>  \n> The newly added file is tracked but other files in the tree get marked as deleted:\n>  \n> [sjc-ads-2565:t.git]$ git status\n> On branch master\n> Changes to be committed:\n>   (use “git restore –staged <file>…” to unstage)\n>                 deleted:    a.c\n\nYep, looks like a bug to me.\ngit apply should never change the status of files that are not mentioned in\nthe input patch.\n\n>                 deleted:    b.c\n>  \n> Changes not staged for commit:\n>   (use “git add <file>…” to update what will be committed)\n>   (use “git restore <file>…” to discard changes in working directory)\n>                 new file:   c.c\n>  \n> It looks like Git created a new index with only the newly added file in the patch.\n\nSeems so.\n\n> However, I’d like Git to just add one entry to the index corresponding\n> to the newly added file in the patch.  Is this a bug or am I completely\n> misinterpreting the goal of ‘intent-to-add’.\n\nYeah, I think your \"git apply --intent-to-add c.diff\" should behave exactly like\n\n\techo test > c.c && git add --intent-to-add c.c\n"},{"id":"440137","messageId":"20211030204155.2500624-1-aclopte@gmail.com","threadId":"56789","inReplyTo":"0DB10E05-094D-4382-AD1F-657878B06A80@cisco.com","subject":"[PATCH] apply: make --intent-to-add not stomp index","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-10-30T20:41:55Z","receivedAt":"2021-10-30T20:42:14Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"Commit cff5dc09ed (apply: add --intent-to-add, 2018-05-26) introduced\n\"apply -N\" plus a test to make sure it behaves exactly as \"add -N\"\nwhen given equivalent changes.  However, the test only checks working\ntree changes. Now \"apply -N\" forgot to read the index, so it left\nall tracked files as deleted, except for the ones it touched.\n\nFix this by reading the index file, like we do for \"apply --cached\".\nand test that we leave no content changes in the index.\n\nReported-by: Ryan Hodges <rphodges@cisco.com>\nSigned-off-by: Johannes Altmanninger <aclopte@gmail.com>\n---\n apply.c               | 2 +-\n t/t2203-add-intent.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 43a0aebf4e..4f740e373b 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4771,7 +4771,7 @@ static int apply_patch(struct apply_state *state,\n \t\t\t\t\t       LOCK_DIE_ON_ERROR);\n \t}\n \n-\tif (state->check_index && read_apply_cache(state) < 0) {\n+\tif ((state->check_index || state->ita_only) && read_apply_cache(state) < 0) {\n \t\terror(_(\"unable to read index file\"));\n \t\tres = -128;\n \t\tgoto end;\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex cf0175ad6e..035ce3a2b9 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -307,7 +307,7 @@ test_expect_success 'apply --intent-to-add' '\n \tgrep \"new file\" expected &&\n \tgit reset --hard &&\n \tgit apply --intent-to-add expected &&\n-\tgit diff >actual &&\n+\t(git diff && git diff --cached) >actual &&\n \ttest_cmp expected actual\n '\n \n-- \n2.33.1\n\n"},{"id":"440138","messageId":"20211030205147.2503327-1-aclopte@gmail.com","threadId":"56789","inReplyTo":"20211030204155.2500624-1-aclopte@gmail.com","subject":"[PATCH v2] apply: make --intent-to-add not stomp index","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-10-30T20:51:47Z","receivedAt":"2021-10-30T20:51:59Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"Commit cff5dc09ed (apply: add --intent-to-add, 2018-05-26) introduced\n\"apply -N\" plus a test to make sure it behaves exactly as \"add -N\"\nwhen given equivalent changes.  However, the test only checks working\ntree changes. Now \"apply -N\" forgot to read the index, so it left\nall tracked files as deleted, except for the ones it touched.\n\nFix this by reading the index file, like we do for \"apply --cached\".\nand test that we leave no content changes in the index.\n\nReported-by: Ryan Hodges <rhodges@cisco.com>\nSigned-off-by: Johannes Altmanninger <aclopte@gmail.com>\n---\n\nSorry I used the wrong Reported-by: address in v1\n\n apply.c               | 2 +-\n t/t2203-add-intent.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 43a0aebf4e..4f740e373b 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4771,7 +4771,7 @@ static int apply_patch(struct apply_state *state,\n \t\t\t\t\t       LOCK_DIE_ON_ERROR);\n \t}\n \n-\tif (state->check_index && read_apply_cache(state) < 0) {\n+\tif ((state->check_index || state->ita_only) && read_apply_cache(state) < 0) {\n \t\terror(_(\"unable to read index file\"));\n \t\tres = -128;\n \t\tgoto end;\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex cf0175ad6e..035ce3a2b9 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -307,7 +307,7 @@ test_expect_success 'apply --intent-to-add' '\n \tgrep \"new file\" expected &&\n \tgit reset --hard &&\n \tgit apply --intent-to-add expected &&\n-\tgit diff >actual &&\n+\t(git diff && git diff --cached) >actual &&\n \ttest_cmp expected actual\n '\n \n-- \n2.33.1\n\n"},{"id":"440165","messageId":"C09B3C5C-86D1-47B4-B4BC-9D0355596A1D@gmail.com","threadId":"56789","inReplyTo":"20211030203916.zopggbajumvb4z3f@gmail.com","subject":"Re: git apply --intent-to-add deletes other files from the index","fromName":"Ryan Hodges","fromEmail":"rphodges@gmail.com","sentAt":"2021-10-30T21:42:42Z","receivedAt":"2021-10-30T21:42:46Z","isPatch":false,"sender":{"key":"rphodges@gmail.com","avatar":null},"body":"Thank you. I was hoping to be the one that fixed this because it was a level of logic that matched my current knowledge level.  I appreciate you jumping in with a fix and also confirming this was unexpected behavior.  I was kind of surprised no one has reported this before.\n\nCheers,\nRyan\n\n\n\n\n\n\n> On Oct 30, 2021, at 1:39 PM, Johannes Altmanninger <aclopte@gmail.com> wrote:\n> \n> On Tue, Oct 26, 2021 at 03:11:36PM +0000, Ryan Hodges (rhodges) wrote:\n>> Hi all,\n>> \n>> I’ve got a quick question about ‘git apply –intent-to-add’.  If I’ve got a patch that just adds one file to the tree:\n>> \n>> [sjc-ads-2565:t.git]$ git diff\n>> diff --git a/c.c b/c.c\n>> new file mode 100644\n>> index 0000000..9daeafb\n>> --- /dev/null\n>> +++ b/c.c\n>> @@ -0,0 +1 @@\n>> +test\n>> \n>> and I apply that patch with –intent-to-add:\n>> \n>> [sjc-ads-2565:t.git]$ git apply --intent-to-add c.diff\n>> \n>> The newly added file is tracked but other files in the tree get marked as deleted:\n>> \n>> [sjc-ads-2565:t.git]$ git status\n>> On branch master\n>> Changes to be committed:\n>>  (use “git restore –staged <file>…” to unstage)\n>>                deleted:    a.c\n> \n> Yep, looks like a bug to me.\n> git apply should never change the status of files that are not mentioned in\n> the input patch.\n> \n>>                deleted:    b.c\n>> \n>> Changes not staged for commit:\n>>  (use “git add <file>…” to update what will be committed)\n>>  (use “git restore <file>…” to discard changes in working directory)\n>>                new file:   c.c\n>> \n>> It looks like Git created a new index with only the newly added file in the patch.\n> \n> Seems so.\n> \n>> However, I’d like Git to just add one entry to the index corresponding\n>> to the newly added file in the patch.  Is this a bug or am I completely\n>> misinterpreting the goal of ‘intent-to-add’.\n> \n> Yeah, I think your \"git apply --intent-to-add c.diff\" should behave exactly like\n> \n> \techo test > c.c && git add --intent-to-add c.c\n\n"},{"id":"440189","messageId":"20211031064300.z6lk4wpojwudgxjw@gmail.com","threadId":"56789","inReplyTo":"C09B3C5C-86D1-47B4-B4BC-9D0355596A1D@gmail.com","subject":"Re: git apply --intent-to-add deletes other files from the index","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-10-31T06:43:00Z","receivedAt":"2021-10-31T06:43:06Z","isPatch":false,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Sat, Oct 30, 2021 at 02:42:42PM -0700, Ryan Hodges wrote:\n> Thank you. I was hoping to be the one that fixed this because it was a level of logic that matched my current knowledge level.\n\nSorry I should have just confirmed the bug since you had already said to\nlook into it. (I usually try to send things when they are \"done\" from my\nside to minimize roundtrips.)\nI'm sure there are more low-hanging fruits but finding them is the hard part,\nsee also https://lore.kernel.org/git/xmqq7dl5z425.fsf@gitster.g/\n\n> I was kind of surprised no one has reported this before.\n\nI guess no one has used it since it was added in 2018.\n"},{"id":"440213","messageId":"xmqqr1c0cray.fsf@gitster.g","threadId":"56789","inReplyTo":"20211030205147.2503327-1-aclopte@gmail.com","subject":"Re: [PATCH v2] apply: make --intent-to-add not stomp index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-01T06:40:05Z","receivedAt":"2021-11-01T06:41:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Altmanninger <aclopte@gmail.com> writes:\n\n> Commit cff5dc09ed (apply: add --intent-to-add, 2018-05-26) introduced\n> \"apply -N\" plus a test to make sure it behaves exactly as \"add -N\"\n> when given equivalent changes.  However, the test only checks working\n> tree changes. Now \"apply -N\" forgot to read the index, so it left\n> all tracked files as deleted, except for the ones it touched.\n>\n> Fix this by reading the index file, like we do for \"apply --cached\".\n> and test that we leave no content changes in the index.\n>\n> Reported-by: Ryan Hodges <rhodges@cisco.com>\n> Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>\n> ---\n>\n> Sorry I used the wrong Reported-by: address in v1\n>\n>  apply.c               | 2 +-\n>  t/t2203-add-intent.sh | 2 +-\n>  2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/apply.c b/apply.c\n> index 43a0aebf4e..4f740e373b 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -4771,7 +4771,7 @@ static int apply_patch(struct apply_state *state,\n>  \t\t\t\t\t       LOCK_DIE_ON_ERROR);\n>  \t}\n>  \n> -\tif (state->check_index && read_apply_cache(state) < 0) {\n> +\tif ((state->check_index || state->ita_only) && read_apply_cache(state) < 0) {\n>  \t\terror(_(\"unable to read index file\"));\n>  \t\tres = -128;\n>  \t\tgoto end;\n\nThanks for an attempt, but I am not sure if it is wise to keep\nita_only independent from check_index like this patch does.\n\nThere are many safety/correctness related checks that check_index\nenables, and that is why not just the \"--index\" option, but \"--3way\"\nand \"--cached\" enable it internally.  As \"instead of adding the\ncontents to the index, mark the new path with i-t-a bit\" is also\nfutzing with the index, it should enable the same safety checks by\nenabling check_index _much_ earlier.  And if you did so, the above\nhunk will become a totally unnecessary change, because by the time\nthe control gets there, because you accepted ita_only earlier and\nenabled check_index, just like you did for \"--3way\" and \"--cached\".\n\nOne thing that check_index does is that it drops unsafe_paths bit,\nwhich means we would be protected from patch application that tries\nto step out of our narrow cone of the directory hierarchy, which is\na safety measure.  There are probably others I am forgetting.\n\nCan you study the code to decide if check_apply_state() is the right\nplace to do this instead?  I have this feeling that the following\nbit in the function\n\n\tif (state->ita_only && (state->check_index || is_not_gitdir))\n\t\tstate->ita_only = 0;\n\nis simply _wrong_ to silently drop the ita_only bit when not in a\nrepository, or other index-touching options are in effect.  Rather,\nI wonder if it should look more like the attached (the other parts\nof the implementation of ita_only may be depending on the buggy\nconstruct, which might result in other breakages if we did this\nalone, though).\n\nThanks.\n\n\n apply.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git c/apply.c w/apply.c\nindex 43a0aebf4e..887465347b 100644\n--- c/apply.c\n+++ w/apply.c\n@@ -146,15 +146,15 @@ int check_apply_state(struct apply_state *state, int force_apply)\n \t}\n \tif (!force_apply && (state->diffstat || state->numstat || state->summary || state->check || state->fake_ancestor))\n \t\tstate->apply = 0;\n+\tif (state->ita_only)\n+\t\tstate->check_index = 1;\n \tif (state->check_index && is_not_gitdir)\n \t\treturn error(_(\"--index outside a repository\"));\n \tif (state->cached) {\n \t\tif (is_not_gitdir)\n \t\t\treturn error(_(\"--cached outside a repository\"));\n \t\tstate->check_index = 1;\n \t}\n-\tif (state->ita_only && (state->check_index || is_not_gitdir))\n-\t\tstate->ita_only = 0;\n \tif (state->check_index)\n \t\tstate->unsafe_paths = 0;\n \n"},{"id":"440215","messageId":"xmqqfssgcq1b.fsf_-_@gitster.g","threadId":"56789","inReplyTo":"xmqqr1c0cray.fsf@gitster.g","subject":"Re* [PATCH v2] apply: make --intent-to-add not stomp index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-01T07:07:28Z","receivedAt":"2021-11-01T07:07:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Can you study the code to decide if check_apply_state() is the right\n> place to do this instead?  I have this feeling that the following\n> bit in the function\n>\n> \tif (state->ita_only && (state->check_index || is_not_gitdir))\n> \t\tstate->ita_only = 0;\n>\n> is simply _wrong_ to silently drop the ita_only bit when not in a\n> repository, or other index-touching options are in effect.  Rather,\n> I wonder if it should look more like the attached (the other parts\n> of the implementation of ita_only may be depending on the buggy\n> construct, which might result in other breakages if we did this\n> alone, though).\n\nAll the existing tests and your new test seem to pass with the \"-N\nshould imply --index\" fix.  It could merely be an indication that\nour test coverage is horrible, but I _think_ the intent of \"-N\" is\nto behave like \"--index\" does, but handle creation part slightly\ndifferently.\n\nOf course there is another possible interpretation for \"-N\", which\nis to behave unlike \"--index\" and touch _only_ the working tree\nfiles, but creations are recorded as if \"git add -N\" were run for\nnew paths after such a \"working tree only\" application was done.\n\nI cannot tell if that is what you wanted to implement; the new test\nin your patch seems to pass with the first interpretation.\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] apply: --intent-to-add should imply --index\n\nOtherwise we do not read the current index, and more importantly, we\ndo not check with the current index, losing all the safety.\n\nAnd the worst part of the story is that we still write the result\nout to the index, which loses all the files that are not mentioned\nin the incoming patch.\n\nReported-by: Ryan Hodges <rhodges@cisco.com>\nTest-by: Johannes Altmanninger <aclopte@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n apply.c               | 4 ++--\n t/t2203-add-intent.sh | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 43a0aebf4e..887465347b 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -146,6 +146,8 @@ int check_apply_state(struct apply_state *state, int force_apply)\n \t}\n \tif (!force_apply && (state->diffstat || state->numstat || state->summary || state->check || state->fake_ancestor))\n \t\tstate->apply = 0;\n+\tif (state->ita_only)\n+\t\tstate->check_index = 1;\n \tif (state->check_index && is_not_gitdir)\n \t\treturn error(_(\"--index outside a repository\"));\n \tif (state->cached) {\n@@ -153,8 +155,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n \t\t\treturn error(_(\"--cached outside a repository\"));\n \t\tstate->check_index = 1;\n \t}\n-\tif (state->ita_only && (state->check_index || is_not_gitdir))\n-\t\tstate->ita_only = 0;\n \tif (state->check_index)\n \t\tstate->unsafe_paths = 0;\n \ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex cf0175ad6e..035ce3a2b9 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -307,7 +307,7 @@ test_expect_success 'apply --intent-to-add' '\n \tgrep \"new file\" expected &&\n \tgit reset --hard &&\n \tgit apply --intent-to-add expected &&\n-\tgit diff >actual &&\n+\t(git diff && git diff --cached) >actual &&\n \ttest_cmp expected actual\n '\n \n-- \n2.34.0-rc0-136-gecf67dd964\n\n"},{"id":"440571","messageId":"20211106112432.otomxu6mwud4b7xi@gmail.com","threadId":"56789","inReplyTo":"xmqqr1c0cray.fsf@gitster.g","subject":"Re: [PATCH v2] apply: make --intent-to-add not stomp index","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-11-06T11:24:32Z","receivedAt":"2021-11-06T11:24:38Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Sun, Oct 31, 2021 at 11:40:05PM -0700, Junio C Hamano wrote:\n> Johannes Altmanninger <aclopte@gmail.com> writes:\n> \n> > Commit cff5dc09ed (apply: add --intent-to-add, 2018-05-26) introduced\n> > \"apply -N\" plus a test to make sure it behaves exactly as \"add -N\"\n> > when given equivalent changes.  However, the test only checks working\n> > tree changes. Now \"apply -N\" forgot to read the index, so it left\n> > all tracked files as deleted, except for the ones it touched.\n> >\n> > Fix this by reading the index file, like we do for \"apply --cached\".\n> > and test that we leave no content changes in the index.\n> >\n> > Reported-by: Ryan Hodges <rhodges@cisco.com>\n> > Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>\n> > ---\n> >\n> > Sorry I used the wrong Reported-by: address in v1\n> >\n> >  apply.c               | 2 +-\n> >  t/t2203-add-intent.sh | 2 +-\n> >  2 files changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/apply.c b/apply.c\n> > index 43a0aebf4e..4f740e373b 100644\n> > --- a/apply.c\n> > +++ b/apply.c\n> > @@ -4771,7 +4771,7 @@ static int apply_patch(struct apply_state *state,\n> >  \t\t\t\t\t       LOCK_DIE_ON_ERROR);\n> >  \t}\n> >  \n> > -\tif (state->check_index && read_apply_cache(state) < 0) {\n> > +\tif ((state->check_index || state->ita_only) && read_apply_cache(state) < 0) {\n> >  \t\terror(_(\"unable to read index file\"));\n> >  \t\tres = -128;\n> >  \t\tgoto end;\n> \n> Thanks for an attempt, but I am not sure if it is wise to keep\n> ita_only independent from check_index like this patch does.\n\nI must confess, I didn't even consider alternative solutions.\n\n> \n> There are many safety/correctness related checks that check_index\n> enables, and that is why not just the \"--index\" option, but \"--3way\"\n> and \"--cached\" enable it internally.  As \"instead of adding the\n> contents to the index, mark the new path with i-t-a bit\" is also\n> futzing with the index, it should enable the same safety checks by\n> enabling check_index _much_ earlier.  And if you did so, the above\n> hunk will become a totally unnecessary change, because by the time\n> the control gets there, because you accepted ita_only earlier and\n> enabled check_index, just like you did for \"--3way\" and \"--cached\".\n> \n> One thing that check_index does is that it drops unsafe_paths bit,\n> which means we would be protected from patch application that tries\n> to step out of our narrow cone of the directory hierarchy, which is\n> a safety measure.  There are probably others I am forgetting.\n\nTo be clear, check_index *disables* the unsafe_paths check, but it enables\na stronger check: verify_index_match(), which makes sure that the touched\npaths exist in the index.\n"},{"id":"440572","messageId":"20211106112439.iw7asj4bq6uwcb3l@gmail.com","threadId":"56789","inReplyTo":"xmqqfssgcq1b.fsf_-_@gitster.g","subject":"Re: Re* [PATCH v2] apply: make --intent-to-add not stomp index","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-11-06T11:24:39Z","receivedAt":"2021-11-06T11:24:45Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Mon, Nov 01, 2021 at 12:07:28AM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Can you study the code to decide if check_apply_state() is the right\n> > place to do this instead?  I have this feeling that the following\n> > bit in the function\n> >\n> > \tif (state->ita_only && (state->check_index || is_not_gitdir))\n> > \t\tstate->ita_only = 0;\n> >\n> > is simply _wrong_ to silently drop the ita_only bit when not in a\n> > repository, or other index-touching options are in effect.  Rather,\n> > I wonder if it should look more like the attached (the other parts\n> > of the implementation of ita_only may be depending on the buggy\n> > construct, which might result in other breakages if we did this\n> > alone, though).\n> \n> All the existing tests and your new test seem to pass with the \"-N\n> should imply --index\" fix.  It could merely be an indication that\n> our test coverage is horrible, but I _think_ the intent of \"-N\" is\n> to behave like \"--index\" does, but handle creation part slightly\n> differently.\n> \n> Of course there is another possible interpretation for \"-N\", which\n> is to behave unlike \"--index\" and touch _only_ the working tree\n> files, but creations are recorded as if \"git add -N\" were run for\n> new paths after such a \"working tree only\" application was done.\n> \n> I cannot tell if that is what you wanted to implement; the new test\n> in your patch seems to pass with the first interpretation.\n\nI'm still not entirely sure, but the ita-implies-check_index seems simpler\noverall, which is a good sign.\nIt will prevent \"apply -N\" from modifying untracked files, which seems like\na good safety measure.\n\n> \n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\n> Subject: [PATCH] apply: --intent-to-add should imply --index\n> \n> Otherwise we do not read the current index, and more importantly, we\n> do not check with the current index, losing all the safety.\n\n(The i-t-a bit should only trigger for added files, so a correct implementation\nwould preserve the index for all other entries.)\n\n> \n> And the worst part of the story is that we still write the result\n> out to the index, which loses all the files that are not mentioned\n> in the incoming patch.\n> \n> Reported-by: Ryan Hodges <rhodges@cisco.com>\n> Test-by: Johannes Altmanninger <aclopte@gmail.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  apply.c               | 4 ++--\n>  t/t2203-add-intent.sh | 2 +-\n>  2 files changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/apply.c b/apply.c\n> index 43a0aebf4e..887465347b 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -146,6 +146,8 @@ int check_apply_state(struct apply_state *state, int force_apply)\n>  \t}\n>  \tif (!force_apply && (state->diffstat || state->numstat || state->summary || state->check || state->fake_ancestor))\n>  \t\tstate->apply = 0;\n> +\tif (state->ita_only)\n> +\t\tstate->check_index = 1;\n>  \tif (state->check_index && is_not_gitdir)\n>  \t\treturn error(_(\"--index outside a repository\"));\n>  \tif (state->cached) {\n> @@ -153,8 +155,6 @@ int check_apply_state(struct apply_state *state, int force_apply)\n>  \t\t\treturn error(_(\"--cached outside a repository\"));\n>  \t\tstate->check_index = 1;\n>  \t}\n> -\tif (state->ita_only && (state->check_index || is_not_gitdir))\n> -\t\tstate->ita_only = 0;\n\nAs you suspected earlier, adding \"ita_only implies check_index\" alone will\nbreak the test case below, because other places assume\n\"ita_only implies none of --cached/--index/--threeway was given\"\n\ntest_expect_success 'apply --index --intent-to-add ignores --intent-to-add, so it does not set i-t-a bit of touched file' '\n\techo >file &&\n\tgit add file &&\n\tgit apply --index --intent-to-add <<-EOF &&\n\tdiff --git a/file b/file\n\tdeleted file mode 100644\n\tindex f00c965..7e91ed5 100644\n\t--- a/file\n\t+++ /dev/null\n\t@@ -1 +0,0 @@\n\t-\n\tEOF\n\tgit ls-files file >actual &&\n\ttest_must_be_empty actual\n'\n\nA fix would be to say\n\"ita_only implies check_index, except if one of its older siblings is present\"\n\n\tif (state->check_index)\n\t\tstate->ita_only = 0;\n\tif (state->ita_only)\n\t\tstate->check_index = 1;\n\nThis matches the documentation of git-apply, and puts ita_only in its place\nas early as possible.\n\n>  \tif (state->check_index)\n>  \t\tstate->unsafe_paths = 0;\n>  \n> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\n> index cf0175ad6e..035ce3a2b9 100755\n> --- a/t/t2203-add-intent.sh\n> +++ b/t/t2203-add-intent.sh\n> @@ -307,7 +307,7 @@ test_expect_success 'apply --intent-to-add' '\n>  \tgrep \"new file\" expected &&\n>  \tgit reset --hard &&\n>  \tgit apply --intent-to-add expected &&\n> -\tgit diff >actual &&\n> +\t(git diff && git diff --cached) >actual &&\n>  \ttest_cmp expected actual\n>  '\n>  \n> -- \n> 2.34.0-rc0-136-gecf67dd964\n> \n"},{"id":"440573","messageId":"20211106114202.3486969-1-aclopte@gmail.com","threadId":"56789","inReplyTo":"20211106112439.iw7asj4bq6uwcb3l@gmail.com","subject":"[PATCH v3] apply: --intent-to-add should imply --index","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-11-06T11:42:02Z","receivedAt":"2021-11-06T11:46:01Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"Otherwise we do not read the current index, and more importantly, we\ndo not check with the current index, losing all the safety.\n\nAnd the worst part of the story is that we still write the result\nout to the index, which loses all the files that are not mentioned\nin the incoming patch.\n\nMake --intent-to-add imply --index. This means that apply\n--intent-to-add will error without a repo, and refuse to modify\nuntracked files (except for added files). Add a test for the latter, and\nanother one to make sure that combinations like \"--cached -N\" keep working.\nas documented (-N is ignored, otherwise it would do weird things to the index).\n\nUse --intent-to-add instead of -N because we don't document -N in\ngit-apply.txt, which might be because it's much more obscure than \"add -N\".\n\nReported-by: Ryan Hodges <rhodges@cisco.com>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Johannes Altmanninger <aclopte@gmail.com>\n---\n\nNot sure about the log message, it feels a bit stitched together.\n\nMost importantly this adds a test to show the difference between v2 (where\nita did not imply check_index).\n\nI wrapped the doc changes to 80 columns, but not the entire paragraph,\nsince we are inconsistent about that.\n\n Documentation/git-apply.txt |  7 +++----\n apply.c                     |  9 +++++++--\n t/t2203-add-intent.sh       |  2 +-\n t/t4140-apply-ita.sh        | 29 +++++++++++++++++++++++++++++\n 4 files changed, 40 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt\nindex aa1ae56a25..18ddb4cf8a 100644\n--- a/Documentation/git-apply.txt\n+++ b/Documentation/git-apply.txt\n@@ -77,10 +77,9 @@ OPTIONS\n --intent-to-add::\n \tWhen applying the patch only to the working tree, mark new\n \tfiles to be added to the index later (see `--intent-to-add`\n-\toption in linkgit:git-add[1]). This option is ignored unless\n-\trunning in a Git repository and `--index` is not specified.\n-\tNote that `--index` could be implied by other options such\n-\tas `--cached` or `--3way`.\n+\toption in linkgit:git-add[1]). This option has is ignored if `--index`\n+\tis specified.  Note that `--index` could be implied by other options\n+\tsuch as `--cached` or `--3way`.\n \n -3::\n --3way::\ndiff --git a/apply.c b/apply.c\nindex 43a0aebf4e..b0239b7482 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -153,8 +153,13 @@ int check_apply_state(struct apply_state *state, int force_apply)\n \t\t\treturn error(_(\"--cached outside a repository\"));\n \t\tstate->check_index = 1;\n \t}\n-\tif (state->ita_only && (state->check_index || is_not_gitdir))\n+\tif (state->ita_only && state->check_index)\n \t\tstate->ita_only = 0;\n+\tif (state->ita_only) {\n+\t\tif (is_not_gitdir)\n+\t\t\treturn error(_(\"--intent-to-add outside a repository\"));\n+\t\tstate->check_index = 1;\n+\t}\n \tif (state->check_index)\n \t\tstate->unsafe_paths = 0;\n \n@@ -4760,7 +4765,7 @@ static int apply_patch(struct apply_state *state,\n \tif (state->whitespace_error && (state->ws_error_action == die_on_ws_error))\n \t\tstate->apply = 0;\n \n-\tstate->update_index = (state->check_index || state->ita_only) && state->apply;\n+\tstate->update_index = state->check_index && state->apply;\n \tif (state->update_index && !is_lock_file_locked(&state->lock_file)) {\n \t\tif (state->index_file)\n \t\t\thold_lock_file_for_update(&state->lock_file,\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex cf0175ad6e..035ce3a2b9 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -307,7 +307,7 @@ test_expect_success 'apply --intent-to-add' '\n \tgrep \"new file\" expected &&\n \tgit reset --hard &&\n \tgit apply --intent-to-add expected &&\n-\tgit diff >actual &&\n+\t(git diff && git diff --cached) >actual &&\n \ttest_cmp expected actual\n '\n \ndiff --git a/t/t4140-apply-ita.sh b/t/t4140-apply-ita.sh\nindex c614eaf04c..4db1ae4e7e 100755\n--- a/t/t4140-apply-ita.sh\n+++ b/t/t4140-apply-ita.sh\n@@ -53,4 +53,33 @@ test_expect_success 'apply deletion patch to ita path (--index)' '\n \tgit ls-files --stage --error-unmatch test-file\n '\n \n+test_expect_success 'apply --intent-to-add is not allowed to modify untracked file' '\n+\techo version1 >file &&\n+\t! git apply --intent-to-add <<-EOF\n+\tdiff --git a/file b/file\n+\tindex 1234567..89abcde 100644\n+\t--- b/file\n+\t+++ b/file\n+\t@@ -1 +1 @@\n+\t-version1\n+\t+version2\n+\tEOF\n+'\n+\n+test_expect_success 'apply --index --intent-to-add ignores --intent-to-add, so it does not set i-t-a bit of touched file' '\n+\techo >file &&\n+\tgit add file &&\n+\tgit apply --index --intent-to-add <<-EOF &&\n+\tdiff --git a/file b/file\n+\tdeleted file mode 100644\n+\tindex 1234567..89abcde 100644\n+\t--- a/file\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-\n+\tEOF\n+\tgit ls-files file >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \n2.33.1\n\n"},{"id":"440574","messageId":"20211106114710.v3ew6752svuvjtk6@gmail.com","threadId":"56789","inReplyTo":"20211106114202.3486969-1-aclopte@gmail.com","subject":"Re: [PATCH v3] apply: --intent-to-add should imply --index","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-11-06T11:47:10Z","receivedAt":"2021-11-06T11:47:15Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Sat, Nov 06, 2021 at 12:42:02PM +0100, Johannes Altmanninger wrote:\n> +test_expect_success 'apply --intent-to-add is not allowed to modify untracked file' '\n> +\techo version1 >file &&\n> +\t! git apply --intent-to-add <<-EOF\n\nI guess s/!/test_must_fail/\n"}]}