[PATCH 0/5] fix apply --intent-to-add

[PATCH 0/5] fix apply --intent-to-add

[Raymond E. Pasco]

The --intent-to-add (-N) flag to apply has not worked properly since its
introduction in Git 2.19; in particular, it creates an empty index
except for any new files in the patch, making it largely useless except
on blank repositories.

This patch series fixes it to work as expected and updates the tests
and documentation for this flag.

Earlier discussion of this issue can be found in the threads associated
with message ids <20211106114202.3486969-1-aclopte@gmail.com> and
<20250511003955.242889-1-ray@ameretat.dev>.

Raymond E. Pasco (5):
  apply: error on --intent-to-add outside gitdir
  apply: read in the index in --intent-to-add mode
  apply: only write intents to add for new files
  t4140: test apply --intent-to-add interactions
  apply docs: clarify wording for --intent-to-add

 Documentation/git-apply.adoc |  8 ++++----
 apply.c                      | 12 ++++++++----
 t/t4140-apply-ita.sh         | 31 ++++++++++++++++++++++++++++++-
 3 files changed, 42 insertions(+), 9 deletions(-)

-- 
2.50.0.195.g74e6fc65d0

[PATCH 1/5] apply: error on --intent-to-add outside gitdir

[Raymond E. Pasco]

It makes no sense to register an intent to add outside a repository. We
should error out here.

Based-on-patch-by: Johannes Altmanninger <aclopte@gmail.com>
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 apply.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index 8bbe6ed224..e7856ae6b3 100644
--- a/apply.c
+++ b/apply.c
@@ -174,8 +174,12 @@ int check_apply_state(struct apply_state *state, int force_apply)
 			return error(_("'%s' outside a repository"), "--cached");
 		state->check_index = 1;
 	}
-	if (state->ita_only && (state->check_index || is_not_gitdir))
-		state->ita_only = 0;
+	if (state->ita_only) {
+		if (is_not_gitdir)
+			return error(_("'%s' outside a repository"), "--intent-to-add");
+		if (state->check_index)
+			state->ita_only = 0;
+	}
 	if (state->check_index)
 		state->unsafe_paths = 0;
 
-- 
2.50.0.195.g74e6fc65d0

Re: [PATCH 1/5] apply: error on --intent-to-add outside gitdir

[Junio C Hamano]

"Raymond E. Pasco" <ray@ameretat.dev> writes:

> It makes no sense to register an intent to add outside a repository. We
> should error out here.

I am not so sure, especially if you still keep the original "if we
are using 'git apply' as a better GNU patch, ignore -N" logic.

Not that I am suggesting to also error out when -N is given without
--index or --cached, that is.

I think the most problematic is the claim "it makes no sense" is not
linked to the conclusion "we should error out".  The behaviour of
the original code, and the behaviour of the code with this patch in
"a better GNU patch" mode is based on "it makes no sense in such a
context, hence we just ignore (instead of erroring out)", which
makes perfect sense as well.

> Based-on-patch-by: Johannes Altmanninger <aclopte@gmail.com>
> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
> ---
>  apply.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 8bbe6ed224..e7856ae6b3 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -174,8 +174,12 @@ int check_apply_state(struct apply_state *state, int force_apply)
>  			return error(_("'%s' outside a repository"), "--cached");
>  		state->check_index = 1;
>  	}
> -	if (state->ita_only && (state->check_index || is_not_gitdir))
> -		state->ita_only = 0;
> +	if (state->ita_only) {
> +		if (is_not_gitdir)
> +			return error(_("'%s' outside a repository"), "--intent-to-add");
> +		if (state->check_index)
> +			state->ita_only = 0;
> +	}
>  	if (state->check_index)
>  		state->unsafe_paths = 0;

Re: [PATCH 1/5] apply: error on --intent-to-add outside gitdir

[Raymond E. Pasco]

On 25/06/30 11:34AM, Junio C Hamano wrote:
> "Raymond E. Pasco" <ray@ameretat.dev> writes:
> 
> > It makes no sense to register an intent to add outside a repository. We
> > should error out here.
> 
> I am not so sure, especially if you still keep the original "if we
> are using 'git apply' as a better GNU patch, ignore -N" logic.
> 
> Not that I am suggesting to also error out when -N is given without
> --index or --cached, that is.
> 
> I think the most problematic is the claim "it makes no sense" is not
> linked to the conclusion "we should error out".  The behaviour of
> the original code, and the behaviour of the code with this patch in
> "a better GNU patch" mode is based on "it makes no sense in such a
> context, hence we just ignore (instead of erroring out)", which
> makes perfect sense as well.

Yeah, perhaps that's right. My thought was that "the user said -N,
they must think they're in a repository". On the other hand, however,
it's a lot weaker than saying --index or --cached, which mean the
user definitely wants to manipulate a git repository; the user is
still /mainly/ trying to manipulate files if they use -N.

[PATCH 2/5] apply: read in the index in --intent-to-add mode

[Raymond E. Pasco]

There are three main modes of operation for apply: applying only to the
worktree, applying to the worktree and index (--index), and applying
only to the index (--cached).

The --intent-to-add flag modifies the first of these modes, applying
only to the worktree, in a way which touches the index, because
intents to add are special index entries. However, it has not ever
worked correctly in any but the most trivial (empty repository)
cases, because the index was never read in (in apply, this is done
in read_apply_cache()) before writing to it.

If we merely gate read_apply_cache() behind update_index, then it will
not be read when state->apply is false, even if it must be checked.
Therefore, we instead read the index if it will be either checked or
updated, because reading the index is a prerequisite to either.

Reported-by: Ryan Hodges <rhodges@cisco.com>
Original-patch-by: Johannes Altmanninger <aclopte@gmail.com>
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 apply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index e7856ae6b3..1757d34618 100644
--- a/apply.c
+++ b/apply.c
@@ -4837,7 +4837,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->update_index) && read_apply_cache(state) < 0) {
 		error(_("unable to read index file"));
 		res = -128;
 		goto end;
-- 
2.50.0.195.g74e6fc65d0

Re: [PATCH 2/5] apply: read in the index in --intent-to-add mode

[Junio C Hamano]

"Raymond E. Pasco" <ray@ameretat.dev> writes:

> There are three main modes of operation for apply: applying only to the
> worktree, applying to the worktree and index (--index), and applying
> only to the index (--cached).
>
> The --intent-to-add flag modifies the first of these modes, applying
> only to the worktree, in a way which touches the index, because
> intents to add are special index entries. However, it has not ever
> worked correctly in any but the most trivial (empty repository)
> cases, because the index was never read in (in apply, this is done
> in read_apply_cache()) before writing to it.

As the inventor of "add -N", I think what "apply -N" does may be
wrong (only judging from the above description; it's been a while
since I really read the code in apply.c).  It does not make any
sense to write a new index that has only the ITA entries.

> If we merely gate read_apply_cache() behind update_index, then it will
> not be read when state->apply is false, even if it must be checked.
> Therefore, we instead read the index if it will be either checked or
> updated, because reading the index is a prerequisite to either.

Makes sense.

> Reported-by: Ryan Hodges <rhodges@cisco.com>
> Original-patch-by: Johannes Altmanninger <aclopte@gmail.com>
> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
> ---
>  apply.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/apply.c b/apply.c
> index e7856ae6b3..1757d34618 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4837,7 +4837,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->update_index) && read_apply_cache(state) < 0) {
>  		error(_("unable to read index file"));
>  		res = -128;
>  		goto end;

Re: [PATCH 2/5] apply: read in the index in --intent-to-add mode

[Raymond E. Pasco]

On 25/06/30 11:47AM, Junio C Hamano wrote:
> "Raymond E. Pasco" <ray@ameretat.dev> writes:
> 
> > There are three main modes of operation for apply: applying only to the
> > worktree, applying to the worktree and index (--index), and applying
> > only to the index (--cached).
> >
> > The --intent-to-add flag modifies the first of these modes, applying
> > only to the worktree, in a way which touches the index, because
> > intents to add are special index entries. However, it has not ever
> > worked correctly in any but the most trivial (empty repository)
> > cases, because the index was never read in (in apply, this is done
> > in read_apply_cache()) before writing to it.
> 
> As the inventor of "add -N", I think what "apply -N" does may be
> wrong (only judging from the above description; it's been a while
> since I really read the code in apply.c).  It does not make any
> sense to write a new index that has only the ITA entries.

Yeah, that's the bug; it writes a new index with just ITA entries (iow,
the index thinks every existing file has been deleted); the fix is to
instead write ITA entries to the existing index, not make a new one. And
the root cause is not having read the index, so it's starting from an
empty tree.

[PATCH 3/5] apply: only write intents to add for new files

[Raymond E. Pasco]

In the "update only the worktree" mode, the index should not be touched
except to record intents to add when --intent-to-add is on. Because
having --intent-to-add on sets update_index, to indicate that we are
touching the index, we can't rely only on that flag to decide whether to
write an index entry.

Instead, we must test whether we are in a mode which updates the
index, or else are in worktree-only mode with --intent-to-add on and
the current file being an addition. We do not need to check
state->apply, because we only enter write_out_results() if state->apply
is already set.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 apply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index 1757d34618..5064a91fbf 100644
--- a/apply.c
+++ b/apply.c
@@ -4569,7 +4569,7 @@ static int create_file(struct apply_state *state, struct patch *patch)
 
 	if (patch->conflicted_threeway)
 		return add_conflicted_stages_file(state, patch);
-	else if (state->update_index)
+	else if (state->check_index || (state->ita_only && patch->is_new > 0))
 		return add_index_file(state, path, mode, buf, size);
 	return 0;
 }
-- 
2.50.0.195.g74e6fc65d0

Re: [PATCH 3/5] apply: only write intents to add for new files

[Junio C Hamano]

"Raymond E. Pasco" <ray@ameretat.dev> writes:

> In the "update only the worktree" mode, the index should not be touched
> except to record intents to add when --intent-to-add is on. Because
> having --intent-to-add on sets update_index, to indicate that we are
> touching the index, we can't rely only on that flag to decide whether to
> write an index entry.

Does that let us inspect state->ita_only alone and conclude that
state->update_index is set, though?  IOW ...

>  	if (patch->conflicted_threeway)
>  		return add_conflicted_stages_file(state, patch);
> -	else if (state->update_index)
> +	else if (state->check_index || (state->ita_only && patch->is_new > 0))

... I would have expected the new code to check not just ita_only but
check ita_only only when update_index is in effect.


>  		return add_index_file(state, path, mode, buf, size);
>  	return 0;
>  }

Re: [PATCH 3/5] apply: only write intents to add for new files

[Raymond E. Pasco]

On 25/06/30 11:53AM, Junio C Hamano wrote:
> "Raymond E. Pasco" <ray@ameretat.dev> writes:
> 
> > In the "update only the worktree" mode, the index should not be touched
> > except to record intents to add when --intent-to-add is on. Because
> > having --intent-to-add on sets update_index, to indicate that we are
> > touching the index, we can't rely only on that flag to decide whether to
> > write an index entry.
> 
> Does that let us inspect state->ita_only alone and conclude that
> state->update_index is set, though?  IOW ...
> 
> >  	if (patch->conflicted_threeway)
> >  		return add_conflicted_stages_file(state, patch);
> > -	else if (state->update_index)
> > +	else if (state->check_index || (state->ita_only && patch->is_new > 0))
> 
> ... I would have expected the new code to check not just ita_only but
> check ita_only only when update_index is in effect.
> 
> 
> >  		return add_index_file(state, path, mode, buf, size);
> >  	return 0;
> >  }

We're behind a more specific test on state->apply to enter this codepath
at all (the only way in is the call to write_out_results around line
4859). So we're already committed to applying the patch, which is one
half of update_index, but the other half is check_index || ita_only and
we must behave differently depending on which of those is in effect and
causing us to touch the index.

[PATCH 4/5] t4140: test apply --intent-to-add interactions

[Raymond E. Pasco]

Test that applying a new file creation patch to an existing index works,
and that applying a patch with both modifications and new file creations
works.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 t/t4140-apply-ita.sh | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/t/t4140-apply-ita.sh b/t/t4140-apply-ita.sh
index c614eaf04c..c664209c41 100755
--- a/t/t4140-apply-ita.sh
+++ b/t/t4140-apply-ita.sh
@@ -7,6 +7,10 @@ test_description='git apply of i-t-a file'
 test_expect_success setup '
 	test_write_lines 1 2 3 4 5 >blueprint &&
 
+  cat blueprint >committed-file &&
+  git add committed-file &&
+  git commit -m "commit" &&
+
 	cat blueprint >test-file &&
 	git add -N test-file &&
 	git diff >creation-patch &&
@@ -14,7 +18,14 @@ test_expect_success setup '
 
 	rm -f test-file &&
 	git diff >deletion-patch &&
-	grep "deleted file mode 100644" deletion-patch
+	grep "deleted file mode 100644" deletion-patch &&
+
+	git rm -f test-file &&
+	test_write_lines 6 >>committed-file &&
+	cat blueprint >test-file &&
+	git add -N test-file &&
+	git diff >complex-patch &&
+	git restore committed-file
 '
 
 test_expect_success 'apply creation patch to ita path (--cached)' '
@@ -53,4 +64,22 @@ test_expect_success 'apply deletion patch to ita path (--index)' '
 	git ls-files --stage --error-unmatch test-file
 '
 
+test_expect_success 'apply creation patch to existing index with -N' '
+  git rm -f test-file &&
+  cat blueprint >index-file &&
+  git add index-file &&
+  git apply -N creation-patch &&
+
+  git ls-files --stage --error-unmatch index-file &&
+  git ls-files --stage --error-unmatch test-file
+'
+
+test_expect_success 'apply complex patch with -N' '
+  git rm -f test-file index-file &&
+  git apply -N complex-patch &&
+
+  git ls-files --stage --error-unmatch test-file &&
+  git diff | grep "a/committed-file"
+'
+
 test_done
-- 
2.50.0.195.g74e6fc65d0

[PATCH 5/5] apply docs: clarify wording for --intent-to-add

[Raymond E. Pasco]

Avoid using a double negative, and keep in mind that --index and
--cached are distinct modes of operation.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 Documentation/git-apply.adoc | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-apply.adoc b/Documentation/git-apply.adoc
index 952518b8af..a41069c0ab 100644
--- a/Documentation/git-apply.adoc
+++ b/Documentation/git-apply.adoc
@@ -75,13 +75,13 @@ OPTIONS
 	tree. If `--check` is in effect, merely check that it would
 	apply cleanly to the index entry.
 
+-N::
 --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 is ignored if
+	`--index` or `--cached` are used. Note that `--index` could
+	be implied by other options such as `--3way`.
 
 -3::
 --3way::
-- 
2.50.0.195.g74e6fc65d0

Re: [PATCH 0/5] fix apply --intent-to-add

[Lidong Yan]

Raymond E. Pasco <ray@ameretat.dev> writes:
> The --intent-to-add (-N) flag to apply has not worked properly since its
> introduction in Git 2.19; in particular, it creates an empty index
> except for any new files in the patch, making it largely useless except
> on blank repositories.

I find the problem is that when we use 'git apply —intent-to-add’, git doesn’t
read index file then update the intent-to-add cache entry, git forgot to read
the index file and only update one intent-to-add cache entry in index file. 

> This patch series fixes it to work as expected and updates the tests
> and documentation for this flag.

I think [PATCH 2/5] actually solve the problem. [PATCH 3/5] makes 'git apply'
consistent with 'git add', which means
  git add —intent-to-add exist-in-index-file.c
and
  git apply —intent-to-add patch-contains-exist-in-index-file

will ignore intent-to-add and do nothing.

Re: [PATCH 0/5] fix apply --intent-to-add

[Raymond E. Pasco]

On 25/06/29 11:10AM, Lidong Yan wrote:
> I think [PATCH 2/5] actually solve the problem. [PATCH 3/5] makes 'git apply'
> consistent with 'git add', which means
>   git add —intent-to-add exist-in-index-file.c
> and
>   git apply —intent-to-add patch-contains-exist-in-index-file
> 
> will ignore intent-to-add and do nothing.

This behavior is correct and expected (even if you don't register an
intent to add) because the file is there in the worktree, so you can't
create a new file in the worktree with that name.

But the real reason for that test in create_file is that we're in a mode
that shouldn't be touching the index at all, so we shouldn't touch the
index (except in the special case where intents to add live in the
index). If there's a patch that updates existing file A, and creates new
file B, then without that test apply will touch the index for both A and
B, which is wrong. It must only touch it for B.

[PATCH v2 0/4] fix apply --intent-to-add

[Raymond E. Pasco]

I've rerolled this taking comments into consideration; I've removed the
first patch, because I'm convinced not erroring out is fine here, and
expanded on the log messages somewhat. I've also fixed an issue where I
was outsmarted by a text editor when writing the test patch.

Raymond E. Pasco (4):
  apply: read in the index in --intent-to-add mode
  apply: only write intents to add for new files
  t4140: test apply --intent-to-add interactions
  apply docs: clarify wording for --intent-to-add

 Documentation/git-apply.adoc |  9 +++++----
 apply.c                      |  4 ++--
 t/t4140-apply-ita.sh         | 31 ++++++++++++++++++++++++++++++-
 3 files changed, 37 insertions(+), 7 deletions(-)

Range-diff against v1:
1:  b1c9ea7cac < -:  ---------- apply: error on --intent-to-add outside gitdir
2:  3a422c8124 ! 1:  71ec291fba apply: read in the index in --intent-to-add mode
    @@ Commit message
         cases, because the index was never read in (in apply, this is done
         in read_apply_cache()) before writing to it.
     
    -    If we merely gate read_apply_cache() behind update_index, then it will
    -    not be read when state->apply is false, even if it must be checked.
    -    Therefore, we instead read the index if it will be either checked or
    -    updated, because reading the index is a prerequisite to either.
    +    This causes the operation to clobber the old, correct index with a
    +    new empty-tree index before writing intent-to-add entries to this
    +    empty index; the final result is that the index now records every
    +    existing file in the repository as deleted, which is incorrect.
    +
    +    This error can be corrected by first reading the index. The
    +    update_index flag is correctly set if ita_only is true, because
    +    this mode updates the index. However, if we merely gate the call
    +    to read_apply_cache() behind update_index, then it will not be read
    +    when state->apply is false, even if it must be checked. Therefore,
    +    we instead read the index if it will be either checked or updated,
    +    because reading the index is a prerequisite to either.
     
         Reported-by: Ryan Hodges <rhodges@cisco.com>
         Original-patch-by: Johannes Altmanninger <aclopte@gmail.com>
3:  df28144f82 ! 2:  5f49ff9035 apply: only write intents to add for new files
    @@ Commit message
         touching the index, we can't rely only on that flag to decide whether to
         write an index entry.
     
    -    Instead, we must test whether we are in a mode which updates the
    -    index, or else are in worktree-only mode with --intent-to-add on and
    -    the current file being an addition. We do not need to check
    -    state->apply, because we only enter write_out_results() if state->apply
    -    is already set.
    +    Because we have already entered write_out_results() and are performing
    +    writes, we know that state->apply is true. If state->check_index is
    +    additionally true, we are in a mode which updates the index and should
    +    always write, whereas if we are merely in ita_only mode we must only
    +    write if the patch is a new file creation patch.
     
         Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
     
4:  0bbae13b63 ! 3:  f8a6d8032f t4140: test apply --intent-to-add interactions
    @@ t/t4140-apply-ita.sh: test_description='git apply of i-t-a file'
      test_expect_success setup '
      	test_write_lines 1 2 3 4 5 >blueprint &&
      
    -+  cat blueprint >committed-file &&
    -+  git add committed-file &&
    -+  git commit -m "commit" &&
    ++	cat blueprint >committed-file &&
    ++	git add committed-file &&
    ++	git commit -m "commit" &&
     +
      	cat blueprint >test-file &&
      	git add -N test-file &&
    @@ t/t4140-apply-ita.sh: test_expect_success 'apply deletion patch to ita path (--i
      '
      
     +test_expect_success 'apply creation patch to existing index with -N' '
    -+  git rm -f test-file &&
    -+  cat blueprint >index-file &&
    -+  git add index-file &&
    -+  git apply -N creation-patch &&
    ++	git rm -f test-file &&
    ++	cat blueprint >index-file &&
    ++	git add index-file &&
    ++	git apply -N creation-patch &&
     +
    -+  git ls-files --stage --error-unmatch index-file &&
    -+  git ls-files --stage --error-unmatch test-file
    ++	git ls-files --stage --error-unmatch index-file &&
    ++	git ls-files --stage --error-unmatch test-file
     +'
     +
     +test_expect_success 'apply complex patch with -N' '
    -+  git rm -f test-file index-file &&
    -+  git apply -N complex-patch &&
    ++	git rm -f test-file index-file &&
    ++	git apply -N complex-patch &&
     +
    -+  git ls-files --stage --error-unmatch test-file &&
    -+  git diff | grep "a/committed-file"
    ++	git ls-files --stage --error-unmatch test-file &&
    ++	git diff | grep "a/committed-file"
     +'
     +
      test_done
5:  970c739ca9 ! 4:  ad42992d03 apply docs: clarify wording for --intent-to-add
    @@ Documentation/git-apply.adoc: OPTIONS
     -	Note that `--index` could be implied by other options such
     -	as `--cached` or `--3way`.
     +	option in linkgit:git-add[1]). This option is ignored if
    -+	`--index` or `--cached` are used. Note that `--index` could
    -+	be implied by other options such as `--3way`.
    ++	`--index` or `--cached` are used, and has no effect outside a Git
    ++	repository. Note that `--index` could be implied by other options
    ++	such as `--3way`.
      
      -3::
      --3way::
-- 
2.50.0.201.gfeb04032fb

[PATCH v2 1/4] apply: read in the index in --intent-to-add mode

[Raymond E. Pasco]

There are three main modes of operation for apply: applying only to the
worktree, applying to the worktree and index (--index), and applying
only to the index (--cached).

The --intent-to-add flag modifies the first of these modes, applying
only to the worktree, in a way which touches the index, because
intents to add are special index entries. However, it has not ever
worked correctly in any but the most trivial (empty repository)
cases, because the index was never read in (in apply, this is done
in read_apply_cache()) before writing to it.

This causes the operation to clobber the old, correct index with a
new empty-tree index before writing intent-to-add entries to this
empty index; the final result is that the index now records every
existing file in the repository as deleted, which is incorrect.

This error can be corrected by first reading the index. The
update_index flag is correctly set if ita_only is true, because
this mode updates the index. However, if we merely gate the call
to read_apply_cache() behind update_index, then it will not be read
when state->apply is false, even if it must be checked. Therefore,
we instead read the index if it will be either checked or updated,
because reading the index is a prerequisite to either.

Reported-by: Ryan Hodges <rhodges@cisco.com>
Original-patch-by: Johannes Altmanninger <aclopte@gmail.com>
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 apply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index 8bbe6ed224..c8d4517c0a 100644
--- a/apply.c
+++ b/apply.c
@@ -4833,7 +4833,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->update_index) && read_apply_cache(state) < 0) {
 		error(_("unable to read index file"));
 		res = -128;
 		goto end;
-- 
2.50.0.201.gfeb04032fb

[PATCH v2 2/4] apply: only write intents to add for new files

[Raymond E. Pasco]

In the "update only the worktree" mode, the index should not be touched
except to record intents to add when --intent-to-add is on. Because
having --intent-to-add on sets update_index, to indicate that we are
touching the index, we can't rely only on that flag to decide whether to
write an index entry.

Because we have already entered write_out_results() and are performing
writes, we know that state->apply is true. If state->check_index is
additionally true, we are in a mode which updates the index and should
always write, whereas if we are merely in ita_only mode we must only
write if the patch is a new file creation patch.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 apply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index c8d4517c0a..8637ad4c9f 100644
--- a/apply.c
+++ b/apply.c
@@ -4565,7 +4565,7 @@ static int create_file(struct apply_state *state, struct patch *patch)
 
 	if (patch->conflicted_threeway)
 		return add_conflicted_stages_file(state, patch);
-	else if (state->update_index)
+	else if (state->check_index || (state->ita_only && patch->is_new > 0))
 		return add_index_file(state, path, mode, buf, size);
 	return 0;
 }
-- 
2.50.0.201.gfeb04032fb

[PATCH v2 3/4] t4140: test apply --intent-to-add interactions

[Raymond E. Pasco]

Test that applying a new file creation patch to an existing index works,
and that applying a patch with both modifications and new file creations
works.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 t/t4140-apply-ita.sh | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/t/t4140-apply-ita.sh b/t/t4140-apply-ita.sh
index c614eaf04c..0b11a8aef4 100755
--- a/t/t4140-apply-ita.sh
+++ b/t/t4140-apply-ita.sh
@@ -7,6 +7,10 @@ test_description='git apply of i-t-a file'
 test_expect_success setup '
 	test_write_lines 1 2 3 4 5 >blueprint &&
 
+	cat blueprint >committed-file &&
+	git add committed-file &&
+	git commit -m "commit" &&
+
 	cat blueprint >test-file &&
 	git add -N test-file &&
 	git diff >creation-patch &&
@@ -14,7 +18,14 @@ test_expect_success setup '
 
 	rm -f test-file &&
 	git diff >deletion-patch &&
-	grep "deleted file mode 100644" deletion-patch
+	grep "deleted file mode 100644" deletion-patch &&
+
+	git rm -f test-file &&
+	test_write_lines 6 >>committed-file &&
+	cat blueprint >test-file &&
+	git add -N test-file &&
+	git diff >complex-patch &&
+	git restore committed-file
 '
 
 test_expect_success 'apply creation patch to ita path (--cached)' '
@@ -53,4 +64,22 @@ test_expect_success 'apply deletion patch to ita path (--index)' '
 	git ls-files --stage --error-unmatch test-file
 '
 
+test_expect_success 'apply creation patch to existing index with -N' '
+	git rm -f test-file &&
+	cat blueprint >index-file &&
+	git add index-file &&
+	git apply -N creation-patch &&
+
+	git ls-files --stage --error-unmatch index-file &&
+	git ls-files --stage --error-unmatch test-file
+'
+
+test_expect_success 'apply complex patch with -N' '
+	git rm -f test-file index-file &&
+	git apply -N complex-patch &&
+
+	git ls-files --stage --error-unmatch test-file &&
+	git diff | grep "a/committed-file"
+'
+
 test_done
-- 
2.50.0.201.gfeb04032fb

[PATCH v2 4/4] apply docs: clarify wording for --intent-to-add

[Raymond E. Pasco]

Avoid using a double negative, and keep in mind that --index and
--cached are distinct modes of operation.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 Documentation/git-apply.adoc | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-apply.adoc b/Documentation/git-apply.adoc
index 952518b8af..6c71ee69da 100644
--- a/Documentation/git-apply.adoc
+++ b/Documentation/git-apply.adoc
@@ -75,13 +75,14 @@ OPTIONS
 	tree. If `--check` is in effect, merely check that it would
 	apply cleanly to the index entry.
 
+-N::
 --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 is ignored if
+	`--index` or `--cached` are used, and has no effect outside a Git
+	repository. Note that `--index` could be implied by other options
+	such as `--3way`.
 
 -3::
 --3way::
-- 
2.50.0.201.gfeb04032fb

[PATCH v3 0/4] fix apply --intent-to-add

[Raymond E. Pasco]

I've rerolled this to considerably expand the commit messages, because
it took me a while to learn apply.c and a while to relearn it as well,
so the messages should be as specific as possible to aid the next
person.

A note about this series: the code changes (first two patches) apply
fine to cff5dc09ed, which introduced the feature in a broken state, and
fix it there. However, that is from 2018, and t4140 did not exist yet at
that time (I wrote it in 2020) and the docs have moved around some in
the interim as well. If it were a year-old, and not a nearly
seven-year-old, issue, I would prioritize making it all work there, but
I'm not sure it's worth adding an entirely new test. Unsure what the
correct thing to do here is.

Raymond E. Pasco (4):
  apply: read in the index in --intent-to-add mode
  apply: only write intents to add for new files
  t4140: test apply --intent-to-add interactions
  apply docs: clarify wording for --intent-to-add

 Documentation/git-apply.adoc |  9 +++++----
 apply.c                      |  4 ++--
 t/t4140-apply-ita.sh         | 31 ++++++++++++++++++++++++++++++-
 3 files changed, 37 insertions(+), 7 deletions(-)

Range-diff against v2:
1:  71ec291fba ! 1:  f3120189b3 apply: read in the index in --intent-to-add mode
    @@ Commit message
         only to the index (--cached).
     
         The --intent-to-add flag modifies the first of these modes, applying
    -    only to the worktree, in a way which touches the index, because
    -    intents to add are special index entries. However, it has not ever
    +    only to the worktree, in a way which touches the index, because intents
    +    to add are special index entries. However, since its introduction
    +    in cff5dc09ed (apply: add --intent-to-add, 2018-05-26), it has not
         worked correctly in any but the most trivial (empty repository)
    -    cases, because the index was never read in (in apply, this is done
    -    in read_apply_cache()) before writing to it.
    +    cases, because the index is never read in (in apply, this is done in
    +    read_apply_cache()) before writing to it.
     
         This causes the operation to clobber the old, correct index with a
         new empty-tree index before writing intent-to-add entries to this
    @@ Commit message
         existing file in the repository as deleted, which is incorrect.
     
         This error can be corrected by first reading the index. The
    -    update_index flag is correctly set if ita_only is true, because
    -    this mode updates the index. However, if we merely gate the call
    -    to read_apply_cache() behind update_index, then it will not be read
    -    when state->apply is false, even if it must be checked. Therefore,
    -    we instead read the index if it will be either checked or updated,
    -    because reading the index is a prerequisite to either.
    +    update_index flag is correctly set if ita_only is true, because this
    +    flag causes the index to be updated. However, if we merely gate the
    +    call to read_apply_cache() behind update_index, then it will not be
    +    read when state->apply is false, even if it must be checked due to
    +    being in --index or --cached mode. Therefore, we instead read the
    +    index if it will be either checked or updated, because reading the
    +    index is a prerequisite to either.
     
         Reported-by: Ryan Hodges <rhodges@cisco.com>
         Original-patch-by: Johannes Altmanninger <aclopte@gmail.com>
2:  5f49ff9035 ! 2:  907a90d849 apply: only write intents to add for new files
    @@ Metadata
      ## Commit message ##
         apply: only write intents to add for new files
     
    -    In the "update only the worktree" mode, the index should not be touched
    -    except to record intents to add when --intent-to-add is on. Because
    -    having --intent-to-add on sets update_index, to indicate that we are
    -    touching the index, we can't rely only on that flag to decide whether to
    -    write an index entry.
    +    In the "apply only to files" mode (i.e., neither --index nor --cached
    +    mode), the index should not be touched except to record intents to
    +    add when --intent-to-add is on. Because having --intent-to-add on sets
    +    update_index, to indicate that we may touch the index, we can't rely
    +    only on that flag in create_file() (which is called to write both new
    +    files and updated files) to decide whether to write an index entry;
    +    if we did, we would write an index entry for every file being patched
    +    (which would moreover be an intent-to-add entry despite not being a
    +    new file, because we are going to turn on the CE_INTENT_TO_ADD flag
    +    in add_index_entry() if we enter it here and ita_only is true).
     
    -    Because we have already entered write_out_results() and are performing
    -    writes, we know that state->apply is true. If state->check_index is
    -    additionally true, we are in a mode which updates the index and should
    +    To decide whether to touch the index, we need to check the
    +    specific reason the index would be updated, rather than merely
    +    their aggregate in the update_index flag. Because we have already
    +    entered write_out_results() and are performing writes, we know that
    +    state->apply is true. If state->check_index is additionally true, we
    +    are in --index or --cached mode, which updates the index and should
         always write, whereas if we are merely in ita_only mode we must only
         write if the patch is a new file creation patch.
     
3:  f8a6d8032f ! 3:  b7603de201 t4140: test apply --intent-to-add interactions
    @@ Metadata
      ## Commit message ##
         t4140: test apply --intent-to-add interactions
     
    -    Test that applying a new file creation patch to an existing index works,
    -    and that applying a patch with both modifications and new file creations
    -    works.
    +    Test that applying a new file creation patch with --intent-to-add to
    +    an existing index does not modify the index outside adding the correct
    +    intents-to-add, and that applying a patch with both modifications
    +    and new file creations with --intent-to-add correctly only adds
    +    intents-to-add to the index.
     
         Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
     
4:  ad42992d03 = 4:  4c786a77c9 apply docs: clarify wording for --intent-to-add
-- 
2.50.0.229.gc167f4d905

[PATCH v3 1/4] apply: read in the index in --intent-to-add mode

[Raymond E. Pasco]

There are three main modes of operation for apply: applying only to the
worktree, applying to the worktree and index (--index), and applying
only to the index (--cached).

The --intent-to-add flag modifies the first of these modes, applying
only to the worktree, in a way which touches the index, because intents
to add are special index entries. However, since its introduction
in cff5dc09ed (apply: add --intent-to-add, 2018-05-26), it has not
worked correctly in any but the most trivial (empty repository)
cases, because the index is never read in (in apply, this is done in
read_apply_cache()) before writing to it.

This causes the operation to clobber the old, correct index with a
new empty-tree index before writing intent-to-add entries to this
empty index; the final result is that the index now records every
existing file in the repository as deleted, which is incorrect.

This error can be corrected by first reading the index. The
update_index flag is correctly set if ita_only is true, because this
flag causes the index to be updated. However, if we merely gate the
call to read_apply_cache() behind update_index, then it will not be
read when state->apply is false, even if it must be checked due to
being in --index or --cached mode. Therefore, we instead read the
index if it will be either checked or updated, because reading the
index is a prerequisite to either.

Reported-by: Ryan Hodges <rhodges@cisco.com>
Original-patch-by: Johannes Altmanninger <aclopte@gmail.com>
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 apply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index 8bbe6ed224..c8d4517c0a 100644
--- a/apply.c
+++ b/apply.c
@@ -4833,7 +4833,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->update_index) && read_apply_cache(state) < 0) {
 		error(_("unable to read index file"));
 		res = -128;
 		goto end;
-- 
2.50.0.229.gc167f4d905

[PATCH v3 2/4] apply: only write intents to add for new files

[Raymond E. Pasco]

In the "apply only to files" mode (i.e., neither --index nor --cached
mode), the index should not be touched except to record intents to
add when --intent-to-add is on. Because having --intent-to-add on sets
update_index, to indicate that we may touch the index, we can't rely
only on that flag in create_file() (which is called to write both new
files and updated files) to decide whether to write an index entry;
if we did, we would write an index entry for every file being patched
(which would moreover be an intent-to-add entry despite not being a
new file, because we are going to turn on the CE_INTENT_TO_ADD flag
in add_index_entry() if we enter it here and ita_only is true).

To decide whether to touch the index, we need to check the
specific reason the index would be updated, rather than merely
their aggregate in the update_index flag. Because we have already
entered write_out_results() and are performing writes, we know that
state->apply is true. If state->check_index is additionally true, we
are in --index or --cached mode, which updates the index and should
always write, whereas if we are merely in ita_only mode we must only
write if the patch is a new file creation patch.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 apply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index c8d4517c0a..8637ad4c9f 100644
--- a/apply.c
+++ b/apply.c
@@ -4565,7 +4565,7 @@ static int create_file(struct apply_state *state, struct patch *patch)
 
 	if (patch->conflicted_threeway)
 		return add_conflicted_stages_file(state, patch);
-	else if (state->update_index)
+	else if (state->check_index || (state->ita_only && patch->is_new > 0))
 		return add_index_file(state, path, mode, buf, size);
 	return 0;
 }
-- 
2.50.0.229.gc167f4d905

[PATCH v3 3/4] t4140: test apply --intent-to-add interactions

[Raymond E. Pasco]

Test that applying a new file creation patch with --intent-to-add to
an existing index does not modify the index outside adding the correct
intents-to-add, and that applying a patch with both modifications
and new file creations with --intent-to-add correctly only adds
intents-to-add to the index.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 t/t4140-apply-ita.sh | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/t/t4140-apply-ita.sh b/t/t4140-apply-ita.sh
index c614eaf04c..0b11a8aef4 100755
--- a/t/t4140-apply-ita.sh
+++ b/t/t4140-apply-ita.sh
@@ -7,6 +7,10 @@ test_description='git apply of i-t-a file'
 test_expect_success setup '
 	test_write_lines 1 2 3 4 5 >blueprint &&
 
+	cat blueprint >committed-file &&
+	git add committed-file &&
+	git commit -m "commit" &&
+
 	cat blueprint >test-file &&
 	git add -N test-file &&
 	git diff >creation-patch &&
@@ -14,7 +18,14 @@ test_expect_success setup '
 
 	rm -f test-file &&
 	git diff >deletion-patch &&
-	grep "deleted file mode 100644" deletion-patch
+	grep "deleted file mode 100644" deletion-patch &&
+
+	git rm -f test-file &&
+	test_write_lines 6 >>committed-file &&
+	cat blueprint >test-file &&
+	git add -N test-file &&
+	git diff >complex-patch &&
+	git restore committed-file
 '
 
 test_expect_success 'apply creation patch to ita path (--cached)' '
@@ -53,4 +64,22 @@ test_expect_success 'apply deletion patch to ita path (--index)' '
 	git ls-files --stage --error-unmatch test-file
 '
 
+test_expect_success 'apply creation patch to existing index with -N' '
+	git rm -f test-file &&
+	cat blueprint >index-file &&
+	git add index-file &&
+	git apply -N creation-patch &&
+
+	git ls-files --stage --error-unmatch index-file &&
+	git ls-files --stage --error-unmatch test-file
+'
+
+test_expect_success 'apply complex patch with -N' '
+	git rm -f test-file index-file &&
+	git apply -N complex-patch &&
+
+	git ls-files --stage --error-unmatch test-file &&
+	git diff | grep "a/committed-file"
+'
+
 test_done
-- 
2.50.0.229.gc167f4d905

[PATCH v3 4/4] apply docs: clarify wording for --intent-to-add

[Raymond E. Pasco]

Avoid using a double negative, and keep in mind that --index and
--cached are distinct modes of operation.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 Documentation/git-apply.adoc | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-apply.adoc b/Documentation/git-apply.adoc
index 952518b8af..6c71ee69da 100644
--- a/Documentation/git-apply.adoc
+++ b/Documentation/git-apply.adoc
@@ -75,13 +75,14 @@ OPTIONS
 	tree. If `--check` is in effect, merely check that it would
 	apply cleanly to the index entry.
 
+-N::
 --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 is ignored if
+	`--index` or `--cached` are used, and has no effect outside a Git
+	repository. Note that `--index` could be implied by other options
+	such as `--3way`.
 
 -3::
 --3way::
-- 
2.50.0.229.gc167f4d905

Re: [PATCH v3 0/4] fix apply --intent-to-add

[Junio C Hamano]

"Raymond E. Pasco" <ray@ameretat.dev> writes:

> I've rerolled this to considerably expand the commit messages, because
> it took me a while to learn apply.c and a while to relearn it as well,
> so the messages should be as specific as possible to aid the next
> person.

Thanks.  Will queue.