git apply --indent-to-add deletes other files from the index

git apply --indent-to-add deletes other files from the index

[Ryan Hodges (rhodges)]

Hi all,
 
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:
 
[sjc-ads-2565:t.git]$ git diff
diff --git a/c.c b/c.c
new file mode 100644
index 0000000..9daeafb
--- /dev/null
+++ b/c.c
@@ -0,0 +1 @@
+test
 
and I apply that patch with –intent-to-add:
 
[sjc-ads-2565:t.git]$ git apply --intent-to-add c.diff
 
The newly added file is tracked but other files in the tree get marked as deleted:
 
[sjc-ads-2565:t.git]$ git status
On branch master
Changes to be committed:
  (use “git restore –staged <file>…” to unstage)
                deleted:    a.c
                deleted:    b.c
 
Changes not staged for commit:
  (use “git add <file>…” to update what will be committed)
  (use “git restore <file>…” to discard changes in working directory)
                new file:   c.c
 
It 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. 
 
I’m currently testing with Git version 2.33.
 
Regards,
Ryan
 

Re: git apply --intent-to-add deletes other files from the index

[Johannes Altmanninger]

On Tue, Oct 26, 2021 at 03:11:36PM +0000, Ryan Hodges (rhodges) wrote:
> Hi all,
>  
> 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:
>  
> [sjc-ads-2565:t.git]$ git diff
> diff --git a/c.c b/c.c
> new file mode 100644
> index 0000000..9daeafb
> --- /dev/null
> +++ b/c.c
> @@ -0,0 +1 @@
> +test
>  
> and I apply that patch with –intent-to-add:
>  
> [sjc-ads-2565:t.git]$ git apply --intent-to-add c.diff
>  
> The newly added file is tracked but other files in the tree get marked as deleted:
>  
> [sjc-ads-2565:t.git]$ git status
> On branch master
> Changes to be committed:
>   (use “git restore –staged <file>…” to unstage)
>                 deleted:    a.c

Yep, looks like a bug to me.
git apply should never change the status of files that are not mentioned in
the input patch.

>                 deleted:    b.c
>  
> Changes not staged for commit:
>   (use “git add <file>…” to update what will be committed)
>   (use “git restore <file>…” to discard changes in working directory)
>                 new file:   c.c
>  
> It looks like Git created a new index with only the newly added file in the patch.

Seems so.

> 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’.

Yeah, I think your "git apply --intent-to-add c.diff" should behave exactly like

	echo test > c.c && git add --intent-to-add c.c

[PATCH] apply: make --intent-to-add not stomp index

[Johannes Altmanninger]

Commit cff5dc09ed (apply: add --intent-to-add, 2018-05-26) introduced
"apply -N" plus a test to make sure it behaves exactly as "add -N"
when given equivalent changes.  However, the test only checks working
tree changes. Now "apply -N" forgot to read the index, so it left
all tracked files as deleted, except for the ones it touched.

Fix this by reading the index file, like we do for "apply --cached".
and test that we leave no content changes in the index.

Reported-by: Ryan Hodges <rphodges@cisco.com>
Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
---
 apply.c               | 2 +-
 t/t2203-add-intent.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index 43a0aebf4e..4f740e373b 100644
--- a/apply.c
+++ b/apply.c
@@ -4771,7 +4771,7 @@ static int apply_patch(struct apply_state *state,
 					       LOCK_DIE_ON_ERROR);
 	}
 
-	if (state->check_index && read_apply_cache(state) < 0) {
+	if ((state->check_index || state->ita_only) && read_apply_cache(state) < 0) {
 		error(_("unable to read index file"));
 		res = -128;
 		goto end;
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index cf0175ad6e..035ce3a2b9 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -307,7 +307,7 @@ test_expect_success 'apply --intent-to-add' '
 	grep "new file" expected &&
 	git reset --hard &&
 	git apply --intent-to-add expected &&
-	git diff >actual &&
+	(git diff && git diff --cached) >actual &&
 	test_cmp expected actual
 '
 
-- 
2.33.1

[PATCH v2] apply: make --intent-to-add not stomp index

[Johannes Altmanninger]

Commit cff5dc09ed (apply: add --intent-to-add, 2018-05-26) introduced
"apply -N" plus a test to make sure it behaves exactly as "add -N"
when given equivalent changes.  However, the test only checks working
tree changes. Now "apply -N" forgot to read the index, so it left
all tracked files as deleted, except for the ones it touched.

Fix this by reading the index file, like we do for "apply --cached".
and test that we leave no content changes in the index.

Reported-by: Ryan Hodges <rhodges@cisco.com>
Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
---

Sorry I used the wrong Reported-by: address in v1

 apply.c               | 2 +-
 t/t2203-add-intent.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index 43a0aebf4e..4f740e373b 100644
--- a/apply.c
+++ b/apply.c
@@ -4771,7 +4771,7 @@ static int apply_patch(struct apply_state *state,
 					       LOCK_DIE_ON_ERROR);
 	}
 
-	if (state->check_index && read_apply_cache(state) < 0) {
+	if ((state->check_index || state->ita_only) && read_apply_cache(state) < 0) {
 		error(_("unable to read index file"));
 		res = -128;
 		goto end;
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index cf0175ad6e..035ce3a2b9 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -307,7 +307,7 @@ test_expect_success 'apply --intent-to-add' '
 	grep "new file" expected &&
 	git reset --hard &&
 	git apply --intent-to-add expected &&
-	git diff >actual &&
+	(git diff && git diff --cached) >actual &&
 	test_cmp expected actual
 '
 
-- 
2.33.1

Re: git apply --intent-to-add deletes other files from the index

[Ryan Hodges]

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.

Cheers,
Ryan






> On Oct 30, 2021, at 1:39 PM, Johannes Altmanninger <aclopte@gmail.com> wrote:
> 
> On Tue, Oct 26, 2021 at 03:11:36PM +0000, Ryan Hodges (rhodges) wrote:
>> Hi all,
>> 
>> 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:
>> 
>> [sjc-ads-2565:t.git]$ git diff
>> diff --git a/c.c b/c.c
>> new file mode 100644
>> index 0000000..9daeafb
>> --- /dev/null
>> +++ b/c.c
>> @@ -0,0 +1 @@
>> +test
>> 
>> and I apply that patch with –intent-to-add:
>> 
>> [sjc-ads-2565:t.git]$ git apply --intent-to-add c.diff
>> 
>> The newly added file is tracked but other files in the tree get marked as deleted:
>> 
>> [sjc-ads-2565:t.git]$ git status
>> On branch master
>> Changes to be committed:
>>  (use “git restore –staged <file>…” to unstage)
>>                deleted:    a.c
> 
> Yep, looks like a bug to me.
> git apply should never change the status of files that are not mentioned in
> the input patch.
> 
>>                deleted:    b.c
>> 
>> Changes not staged for commit:
>>  (use “git add <file>…” to update what will be committed)
>>  (use “git restore <file>…” to discard changes in working directory)
>>                new file:   c.c
>> 
>> It looks like Git created a new index with only the newly added file in the patch.
> 
> Seems so.
> 
>> 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’.
> 
> Yeah, I think your "git apply --intent-to-add c.diff" should behave exactly like
> 
> 	echo test > c.c && git add --intent-to-add c.c

Re: git apply --intent-to-add deletes other files from the index

[Johannes Altmanninger]

On Sat, Oct 30, 2021 at 02:42:42PM -0700, Ryan Hodges wrote:
> 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.

Sorry I should have just confirmed the bug since you had already said to
look into it. (I usually try to send things when they are "done" from my
side to minimize roundtrips.)
I'm sure there are more low-hanging fruits but finding them is the hard part,
see also https://lore.kernel.org/git/xmqq7dl5z425.fsf@gitster.g/

> I was kind of surprised no one has reported this before.

I guess no one has used it since it was added in 2018.

Re: [PATCH v2] apply: make --intent-to-add not stomp index

[Junio C Hamano]

Johannes Altmanninger <aclopte@gmail.com> writes:

> Commit cff5dc09ed (apply: add --intent-to-add, 2018-05-26) introduced
> "apply -N" plus a test to make sure it behaves exactly as "add -N"
> when given equivalent changes.  However, the test only checks working
> tree changes. Now "apply -N" forgot to read the index, so it left
> all tracked files as deleted, except for the ones it touched.
>
> Fix this by reading the index file, like we do for "apply --cached".
> and test that we leave no content changes in the index.
>
> Reported-by: Ryan Hodges <rhodges@cisco.com>
> Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
> ---
>
> Sorry I used the wrong Reported-by: address in v1
>
>  apply.c               | 2 +-
>  t/t2203-add-intent.sh | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 43a0aebf4e..4f740e373b 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4771,7 +4771,7 @@ static int apply_patch(struct apply_state *state,
>  					       LOCK_DIE_ON_ERROR);
>  	}
>  
> -	if (state->check_index && read_apply_cache(state) < 0) {
> +	if ((state->check_index || state->ita_only) && read_apply_cache(state) < 0) {
>  		error(_("unable to read index file"));
>  		res = -128;
>  		goto end;

Thanks for an attempt, but I am not sure if it is wise to keep
ita_only independent from check_index like this patch does.

There are many safety/correctness related checks that check_index
enables, and that is why not just the "--index" option, but "--3way"
and "--cached" enable it internally.  As "instead of adding the
contents to the index, mark the new path with i-t-a bit" is also
futzing with the index, it should enable the same safety checks by
enabling check_index _much_ earlier.  And if you did so, the above
hunk will become a totally unnecessary change, because by the time
the control gets there, because you accepted ita_only earlier and
enabled check_index, just like you did for "--3way" and "--cached".

One thing that check_index does is that it drops unsafe_paths bit,
which means we would be protected from patch application that tries
to step out of our narrow cone of the directory hierarchy, which is
a safety measure.  There are probably others I am forgetting.

Can you study the code to decide if check_apply_state() is the right
place to do this instead?  I have this feeling that the following
bit in the function

	if (state->ita_only && (state->check_index || is_not_gitdir))
		state->ita_only = 0;

is simply _wrong_ to silently drop the ita_only bit when not in a
repository, or other index-touching options are in effect.  Rather,
I wonder if it should look more like the attached (the other parts
of the implementation of ita_only may be depending on the buggy
construct, which might result in other breakages if we did this
alone, though).

Thanks.


 apply.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git c/apply.c w/apply.c
index 43a0aebf4e..887465347b 100644
--- c/apply.c
+++ w/apply.c
@@ -146,15 +146,15 @@ int check_apply_state(struct apply_state *state, int force_apply)
 	}
 	if (!force_apply && (state->diffstat || state->numstat || state->summary || state->check || state->fake_ancestor))
 		state->apply = 0;
+	if (state->ita_only)
+		state->check_index = 1;
 	if (state->check_index && is_not_gitdir)
 		return error(_("--index outside a repository"));
 	if (state->cached) {
 		if (is_not_gitdir)
 			return error(_("--cached outside a repository"));
 		state->check_index = 1;
 	}
-	if (state->ita_only && (state->check_index || is_not_gitdir))
-		state->ita_only = 0;
 	if (state->check_index)
 		state->unsafe_paths = 0;
 

Re* [PATCH v2] apply: make --intent-to-add not stomp index

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Can you study the code to decide if check_apply_state() is the right
> place to do this instead?  I have this feeling that the following
> bit in the function
>
> 	if (state->ita_only && (state->check_index || is_not_gitdir))
> 		state->ita_only = 0;
>
> is simply _wrong_ to silently drop the ita_only bit when not in a
> repository, or other index-touching options are in effect.  Rather,
> I wonder if it should look more like the attached (the other parts
> of the implementation of ita_only may be depending on the buggy
> construct, which might result in other breakages if we did this
> alone, though).

All the existing tests and your new test seem to pass with the "-N
should imply --index" fix.  It could merely be an indication that
our test coverage is horrible, but I _think_ the intent of "-N" is
to behave like "--index" does, but handle creation part slightly
differently.

Of course there is another possible interpretation for "-N", which
is to behave unlike "--index" and touch _only_ the working tree
files, but creations are recorded as if "git add -N" were run for
new paths after such a "working tree only" application was done.

I cannot tell if that is what you wanted to implement; the new test
in your patch seems to pass with the first interpretation.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] apply: --intent-to-add should imply --index

Otherwise we do not read the current index, and more importantly, we
do not check with the current index, losing all the safety.

And the worst part of the story is that we still write the result
out to the index, which loses all the files that are not mentioned
in the incoming patch.

Reported-by: Ryan Hodges <rhodges@cisco.com>
Test-by: Johannes Altmanninger <aclopte@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 apply.c               | 4 ++--
 t/t2203-add-intent.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/apply.c b/apply.c
index 43a0aebf4e..887465347b 100644
--- a/apply.c
+++ b/apply.c
@@ -146,6 +146,8 @@ int check_apply_state(struct apply_state *state, int force_apply)
 	}
 	if (!force_apply && (state->diffstat || state->numstat || state->summary || state->check || state->fake_ancestor))
 		state->apply = 0;
+	if (state->ita_only)
+		state->check_index = 1;
 	if (state->check_index && is_not_gitdir)
 		return error(_("--index outside a repository"));
 	if (state->cached) {
@@ -153,8 +155,6 @@ int check_apply_state(struct apply_state *state, int force_apply)
 			return error(_("--cached outside a repository"));
 		state->check_index = 1;
 	}
-	if (state->ita_only && (state->check_index || is_not_gitdir))
-		state->ita_only = 0;
 	if (state->check_index)
 		state->unsafe_paths = 0;
 
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index cf0175ad6e..035ce3a2b9 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -307,7 +307,7 @@ test_expect_success 'apply --intent-to-add' '
 	grep "new file" expected &&
 	git reset --hard &&
 	git apply --intent-to-add expected &&
-	git diff >actual &&
+	(git diff && git diff --cached) >actual &&
 	test_cmp expected actual
 '
 
-- 
2.34.0-rc0-136-gecf67dd964

Re: [PATCH v2] apply: make --intent-to-add not stomp index

[Johannes Altmanninger]

On Sun, Oct 31, 2021 at 11:40:05PM -0700, Junio C Hamano wrote:
> Johannes Altmanninger <aclopte@gmail.com> writes:
> 
> > Commit cff5dc09ed (apply: add --intent-to-add, 2018-05-26) introduced
> > "apply -N" plus a test to make sure it behaves exactly as "add -N"
> > when given equivalent changes.  However, the test only checks working
> > tree changes. Now "apply -N" forgot to read the index, so it left
> > all tracked files as deleted, except for the ones it touched.
> >
> > Fix this by reading the index file, like we do for "apply --cached".
> > and test that we leave no content changes in the index.
> >
> > Reported-by: Ryan Hodges <rhodges@cisco.com>
> > Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
> > ---
> >
> > Sorry I used the wrong Reported-by: address in v1
> >
> >  apply.c               | 2 +-
> >  t/t2203-add-intent.sh | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/apply.c b/apply.c
> > index 43a0aebf4e..4f740e373b 100644
> > --- a/apply.c
> > +++ b/apply.c
> > @@ -4771,7 +4771,7 @@ static int apply_patch(struct apply_state *state,
> >  					       LOCK_DIE_ON_ERROR);
> >  	}
> >  
> > -	if (state->check_index && read_apply_cache(state) < 0) {
> > +	if ((state->check_index || state->ita_only) && read_apply_cache(state) < 0) {
> >  		error(_("unable to read index file"));
> >  		res = -128;
> >  		goto end;
> 
> Thanks for an attempt, but I am not sure if it is wise to keep
> ita_only independent from check_index like this patch does.

I must confess, I didn't even consider alternative solutions.

> 
> There are many safety/correctness related checks that check_index
> enables, and that is why not just the "--index" option, but "--3way"
> and "--cached" enable it internally.  As "instead of adding the
> contents to the index, mark the new path with i-t-a bit" is also
> futzing with the index, it should enable the same safety checks by
> enabling check_index _much_ earlier.  And if you did so, the above
> hunk will become a totally unnecessary change, because by the time
> the control gets there, because you accepted ita_only earlier and
> enabled check_index, just like you did for "--3way" and "--cached".
> 
> One thing that check_index does is that it drops unsafe_paths bit,
> which means we would be protected from patch application that tries
> to step out of our narrow cone of the directory hierarchy, which is
> a safety measure.  There are probably others I am forgetting.

To be clear, check_index *disables* the unsafe_paths check, but it enables
a stronger check: verify_index_match(), which makes sure that the touched
paths exist in the index.

Re: Re* [PATCH v2] apply: make --intent-to-add not stomp index

[Johannes Altmanninger]

On Mon, Nov 01, 2021 at 12:07:28AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Can you study the code to decide if check_apply_state() is the right
> > place to do this instead?  I have this feeling that the following
> > bit in the function
> >
> > 	if (state->ita_only && (state->check_index || is_not_gitdir))
> > 		state->ita_only = 0;
> >
> > is simply _wrong_ to silently drop the ita_only bit when not in a
> > repository, or other index-touching options are in effect.  Rather,
> > I wonder if it should look more like the attached (the other parts
> > of the implementation of ita_only may be depending on the buggy
> > construct, which might result in other breakages if we did this
> > alone, though).
> 
> All the existing tests and your new test seem to pass with the "-N
> should imply --index" fix.  It could merely be an indication that
> our test coverage is horrible, but I _think_ the intent of "-N" is
> to behave like "--index" does, but handle creation part slightly
> differently.
> 
> Of course there is another possible interpretation for "-N", which
> is to behave unlike "--index" and touch _only_ the working tree
> files, but creations are recorded as if "git add -N" were run for
> new paths after such a "working tree only" application was done.
> 
> I cannot tell if that is what you wanted to implement; the new test
> in your patch seems to pass with the first interpretation.

I'm still not entirely sure, but the ita-implies-check_index seems simpler
overall, which is a good sign.
It will prevent "apply -N" from modifying untracked files, which seems like
a good safety measure.

> 
> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
> Subject: [PATCH] apply: --intent-to-add should imply --index
> 
> Otherwise we do not read the current index, and more importantly, we
> do not check with the current index, losing all the safety.

(The i-t-a bit should only trigger for added files, so a correct implementation
would preserve the index for all other entries.)

> 
> And the worst part of the story is that we still write the result
> out to the index, which loses all the files that are not mentioned
> in the incoming patch.
> 
> Reported-by: Ryan Hodges <rhodges@cisco.com>
> Test-by: Johannes Altmanninger <aclopte@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  apply.c               | 4 ++--
>  t/t2203-add-intent.sh | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/apply.c b/apply.c
> index 43a0aebf4e..887465347b 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -146,6 +146,8 @@ int check_apply_state(struct apply_state *state, int force_apply)
>  	}
>  	if (!force_apply && (state->diffstat || state->numstat || state->summary || state->check || state->fake_ancestor))
>  		state->apply = 0;
> +	if (state->ita_only)
> +		state->check_index = 1;
>  	if (state->check_index && is_not_gitdir)
>  		return error(_("--index outside a repository"));
>  	if (state->cached) {
> @@ -153,8 +155,6 @@ int check_apply_state(struct apply_state *state, int force_apply)
>  			return error(_("--cached outside a repository"));
>  		state->check_index = 1;
>  	}
> -	if (state->ita_only && (state->check_index || is_not_gitdir))
> -		state->ita_only = 0;

As you suspected earlier, adding "ita_only implies check_index" alone will
break the test case below, because other places assume
"ita_only implies none of --cached/--index/--threeway was given"

test_expect_success 'apply --index --intent-to-add ignores --intent-to-add, so it does not set i-t-a bit of touched file' '
	echo >file &&
	git add file &&
	git apply --index --intent-to-add <<-EOF &&
	diff --git a/file b/file
	deleted file mode 100644
	index f00c965..7e91ed5 100644
	--- a/file
	+++ /dev/null
	@@ -1 +0,0 @@
	-
	EOF
	git ls-files file >actual &&
	test_must_be_empty actual
'

A fix would be to say
"ita_only implies check_index, except if one of its older siblings is present"

	if (state->check_index)
		state->ita_only = 0;
	if (state->ita_only)
		state->check_index = 1;

This matches the documentation of git-apply, and puts ita_only in its place
as early as possible.

>  	if (state->check_index)
>  		state->unsafe_paths = 0;
>  
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index cf0175ad6e..035ce3a2b9 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -307,7 +307,7 @@ test_expect_success 'apply --intent-to-add' '
>  	grep "new file" expected &&
>  	git reset --hard &&
>  	git apply --intent-to-add expected &&
> -	git diff >actual &&
> +	(git diff && git diff --cached) >actual &&
>  	test_cmp expected actual
>  '
>  
> -- 
> 2.34.0-rc0-136-gecf67dd964
> 

[PATCH v3] apply: --intent-to-add should imply --index

[Johannes Altmanninger]

Otherwise we do not read the current index, and more importantly, we
do not check with the current index, losing all the safety.

And the worst part of the story is that we still write the result
out to the index, which loses all the files that are not mentioned
in the incoming patch.

Make --intent-to-add imply --index. This means that apply
--intent-to-add will error without a repo, and refuse to modify
untracked files (except for added files). Add a test for the latter, and
another one to make sure that combinations like "--cached -N" keep working.
as documented (-N is ignored, otherwise it would do weird things to the index).

Use --intent-to-add instead of -N because we don't document -N in
git-apply.txt, which might be because it's much more obscure than "add -N".

Reported-by: Ryan Hodges <rhodges@cisco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
---

Not sure about the log message, it feels a bit stitched together.

Most importantly this adds a test to show the difference between v2 (where
ita did not imply check_index).

I wrapped the doc changes to 80 columns, but not the entire paragraph,
since we are inconsistent about that.

 Documentation/git-apply.txt |  7 +++----
 apply.c                     |  9 +++++++--
 t/t2203-add-intent.sh       |  2 +-
 t/t4140-apply-ita.sh        | 29 +++++++++++++++++++++++++++++
 4 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index aa1ae56a25..18ddb4cf8a 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -77,10 +77,9 @@ OPTIONS
 --intent-to-add::
 	When applying the patch only to the working tree, mark new
 	files to be added to the index later (see `--intent-to-add`
-	option in linkgit:git-add[1]). This option is ignored unless
-	running in a Git repository and `--index` is not specified.
-	Note that `--index` could be implied by other options such
-	as `--cached` or `--3way`.
+	option in linkgit:git-add[1]). This option has is ignored if `--index`
+	is specified.  Note that `--index` could be implied by other options
+	such as `--cached` or `--3way`.
 
 -3::
 --3way::
diff --git a/apply.c b/apply.c
index 43a0aebf4e..b0239b7482 100644
--- a/apply.c
+++ b/apply.c
@@ -153,8 +153,13 @@ int check_apply_state(struct apply_state *state, int force_apply)
 			return error(_("--cached outside a repository"));
 		state->check_index = 1;
 	}
-	if (state->ita_only && (state->check_index || is_not_gitdir))
+	if (state->ita_only && state->check_index)
 		state->ita_only = 0;
+	if (state->ita_only) {
+		if (is_not_gitdir)
+			return error(_("--intent-to-add outside a repository"));
+		state->check_index = 1;
+	}
 	if (state->check_index)
 		state->unsafe_paths = 0;
 
@@ -4760,7 +4765,7 @@ static int apply_patch(struct apply_state *state,
 	if (state->whitespace_error && (state->ws_error_action == die_on_ws_error))
 		state->apply = 0;
 
-	state->update_index = (state->check_index || state->ita_only) && state->apply;
+	state->update_index = state->check_index && state->apply;
 	if (state->update_index && !is_lock_file_locked(&state->lock_file)) {
 		if (state->index_file)
 			hold_lock_file_for_update(&state->lock_file,
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index cf0175ad6e..035ce3a2b9 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -307,7 +307,7 @@ test_expect_success 'apply --intent-to-add' '
 	grep "new file" expected &&
 	git reset --hard &&
 	git apply --intent-to-add expected &&
-	git diff >actual &&
+	(git diff && git diff --cached) >actual &&
 	test_cmp expected actual
 '
 
diff --git a/t/t4140-apply-ita.sh b/t/t4140-apply-ita.sh
index c614eaf04c..4db1ae4e7e 100755
--- a/t/t4140-apply-ita.sh
+++ b/t/t4140-apply-ita.sh
@@ -53,4 +53,33 @@ test_expect_success 'apply deletion patch to ita path (--index)' '
 	git ls-files --stage --error-unmatch test-file
 '
 
+test_expect_success 'apply --intent-to-add is not allowed to modify untracked file' '
+	echo version1 >file &&
+	! git apply --intent-to-add <<-EOF
+	diff --git a/file b/file
+	index 1234567..89abcde 100644
+	--- b/file
+	+++ b/file
+	@@ -1 +1 @@
+	-version1
+	+version2
+	EOF
+'
+
+test_expect_success 'apply --index --intent-to-add ignores --intent-to-add, so it does not set i-t-a bit of touched file' '
+	echo >file &&
+	git add file &&
+	git apply --index --intent-to-add <<-EOF &&
+	diff --git a/file b/file
+	deleted file mode 100644
+	index 1234567..89abcde 100644
+	--- a/file
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-
+	EOF
+	git ls-files file >actual &&
+	test_must_be_empty actual
+'
+
 test_done
-- 
2.33.1

Re: [PATCH v3] apply: --intent-to-add should imply --index

[Johannes Altmanninger]

On Sat, Nov 06, 2021 at 12:42:02PM +0100, Johannes Altmanninger wrote:
> +test_expect_success 'apply --intent-to-add is not allowed to modify untracked file' '
> +	echo version1 >file &&
> +	! git apply --intent-to-add <<-EOF

I guess s/!/test_must_fail/

RE: [PATCH v3] apply: --intent-to-add should imply --index

[Jason Cho]

I'm following up on the bug reported by Ryan Hodges on October 26, 2021,
regarding the `git apply --intent-to-add` command incorrectly marking all 
other tracked files as deleted from the index.

Johannes Altmanninger submitted patch v3 titled "apply: --intent-to-add 
should imply --index" to fix this issue. 

Is this fix merged? If so, which Git version includes this fix.

Re: [PATCH v3] apply: --intent-to-add should imply --index

[Kristoffer Haugsbakk]

On Thu, May 1, 2025, at 11:03, Jason Cho wrote:
> I'm following up on the bug reported by Ryan Hodges on October 26, 2021,
> regarding the `git apply --intent-to-add` command incorrectly marking all
> other tracked files as deleted from the index.
>
> Johannes Altmanninger submitted patch v3 titled "apply: --intent-to-add
> should imply --index" to fix this issue.
>
> Is this fix merged? If so, which Git version includes this fix.

I can’t find any commits by Johannes Altmanninger that addresses this.
I also can’t find any commits that start with `apply: --intent-to-add`.

Re: [PATCH v3] apply: --intent-to-add should imply --index

[Junio C Hamano]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

>> Johannes Altmanninger submitted patch v3 titled "apply: --intent-to-add
>> should imply --index" to fix this issue.
>>
>> Is this fix merged? If so, which Git version includes this fix.
>
> I can’t find any commits by Johannes Altmanninger that addresses this.
> I also can’t find any commits that start with `apply: --intent-to-add`.

The documentation says this:

    --intent-to-add::
            When applying the patch only to the working tree, mark new
            files to be added to the index later (see `--intent-to-add`
            option in linkgit:git-add[1]). This option is ignored unless
            running in a Git repository and `--index` is not specified.
            Note that `--index` could be implied by other options such
            as `--cached` or `--3way`.

It is clear that whoever wrote it understands that for this option
to be effective, the patch needs to affect the index, and one way to
do so is for the user to pass `--index`.  But at the same time, that
is not the only option that makes the command touch the index (e.g.,
`--cached` does, too), and it would make it behave incorrectly if a
patch automatically pretends that `--index` was given when this
option was given.

I can't find the patch either, but given the above documentation, is
it even still relevant?

Thanks.

Re: [PATCH v3] apply: --intent-to-add should imply --index

[Ryan Hodges]

> On May 1, 2025, at 9:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> "Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:
> 
>>> Johannes Altmanninger submitted patch v3 titled "apply: --intent-to-add
>>> should imply --index" to fix this issue.
>>> 
>>> Is this fix merged? If so, which Git version includes this fix.
>> 
>> I can’t find any commits by Johannes Altmanninger that addresses this.
>> I also can’t find any commits that start with `apply: --intent-to-add`.
> 
> The documentation says this:
> 
>    --intent-to-add::
>            When applying the patch only to the working tree, mark new
>            files to be added to the index later (see `--intent-to-add`
>            option in linkgit:git-add[1]). This option is ignored unless
>            running in a Git repository and `--index` is not specified.
>            Note that `--index` could be implied by other options such
>            as `--cached` or `--3way`.
> 
> It is clear that whoever wrote it understands that for this option
> to be effective, the patch needs to affect the index, and one way to
> do so is for the user to pass `--index`.  But at the same time, that
> is not the only option that makes the command touch the index (e.g.,
> `--cached` does, too), and it would make it behave incorrectly if a
> patch automatically pretends that `--index` was given when this
> option was given.

The person that wrote that understood that there’s a conflict between
—index and —intent-to-add:

—index means add new files to the index and stage them for commit
—intent-to-add means add an entry for the file to the index but don’t stage it; ie it will be added later

The author of this commit decided that if —intent-to-add and —index are both specified, then —index takes precedence.


> 
> I can't find the patch either, but given the above documentation, is

The reason you can’t find the patch is because you took issue with the fix provided by Johannes and asked for more analysis.  If you want I can dig up the details of that thread.

> it even still relevant?
> 

This issue is still very relevant.  Firstly, let me point out the major issue with the current functionality:

Given a repo with one file, “b.c” and the following diff that adds file “a.c":

	ryan@Ryans-Macbook-Pro git-repo % cat diff
	diff --git a/a.c b/a.c
	new file mode 100644
	index 0000000..e69de29

Here’s what happens when you run git apply —intent-to-add:

	ryan@Ryans-Macbook-Pro git-repo % git apply --intent-to-add diff

	ryan@Ryans-Macbook-Pro git-repo % git status
	On branch master
	Changes to be committed:
   	(use "git restore --staged <file>..." to unstage)
        	 deleted:    b.c

Bam! b.c gets marked for deletion.  The whole index gets replaced with just the files that are marked by —intent-to-add. That’s very bad. 


Lastly, I’d like to point out my use case for —intent-to-add.  In many VCSs if you run ‘apply’, ‘diff’, and ‘commit’ in sequence and in a clean workspace with no changes, the diff that gets fed to ‘apply’ should look the same as the diff that you get from running ‘diff’, which should be the same as the diff created by the commit command.  Because of the index, that doesn’t happen with Git. When applying a patch in git, new files get added to the worktree but they are not registered in the index.  Thus git diff never sees them. Additionally, those files will be missed by commit even if you use -a. The —intent-to-add switch fixes this workflow and allows files to remain out of the index until the user is ready for them.

Thanks



> Thanks.

Re: [PATCH v3] apply: --intent-to-add should imply --index

[Raymond E. Pasco]

Intents to add are a tricky part of the system; I fixed them up
some time ago for `add -p` which uses apply.c machinery, but not for
`apply -N`, which seems to have never worked since its introduction
in Git 2.19.

To recap how this all works, apply has three modes: with no flag, it
applies a diff to the physical files in the worktree; with --index it
applies a diff to both the physical files in the worktree and to the
index, and with --cached it applies to the index but *not* the physical
files in the worktree.

--intent-to-add / -N is intended to apply only to the first of these
modes; this makes sense, because an intent to add is meant to behave
like a diff not added to the index. However, the intent to add lives in
the index; Git just behaves as though it were a worktree change not in
the index.

The behavior `apply -N` actually exhibits is that it clobbers the index
with a new index containing *only* the contents of the diff, nothing
else; my guess is that it was only tested against repositories with
entirely empty trees. If the tree is not empty, then of course an index
with only the intent to add and nothing else shows up as every file in
the tree being deleted.

The patch discussed here (the headers for the thread seem broken,
but the message id is <20211106114202.3486969-1-aclopte@gmail.com>) does
seem like a mostly complete fix for the issue. However, the message is
entirely wrong and confused about how any of this works, which is likely
why the patch fell through the cracks. (Of course --intent-to-add can't
imply --index, they are mutually exclusive options.)

However, the code appears entirely correct. The combination of --cached
with -N doesn't work, despite the message claiming it does, but it can't
possibly work because it includes the file in the index, so it can't
include it as an intent to add in the index. So this just merits a note
that --intent-to-add is mutually exclusive with both --index and
--cached.

If the original author (Johannes Altmanninger) isn't around or doesn't
want to, I can clean this patch up for resubmission.

Re: [PATCH v3] apply: --intent-to-add should imply --index

[Raymond E. Pasco]

On 25/05/02 11:51PM, Raymond E. Pasco wrote:
> However, the code appears entirely correct. The combination of --cached
> with -N doesn't work, despite the message claiming it does, but it can't
> possibly work because it includes the file in the index, so it can't
> include it as an intent to add in the index. So this just merits a note
> that --intent-to-add is mutually exclusive with both --index and
> --cached.
> 
> If the original author (Johannes Altmanninger) isn't around or doesn't
> want to, I can clean this patch up for resubmission.

In fact, I've just discovered an additional issue that remains even
post-patch (it tries to apply the whole diff, even parts that are
not new file additions, with ita_only on), so it needs slightly more
work than implied above to be complete.