git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* RE: [PATCH v3] apply: --intent-to-add should imply --index
@ 2025-05-01  9:03 Jason Cho
  2025-05-01 12:48 ` Kristoffer Haugsbakk
  0 siblings, 1 reply; 16+ messages in thread
From: Jason Cho @ 2025-05-01  9:03 UTC (permalink / raw)
  To: git@vger.kernel.org
  Cc: aclopte@gmail.com, gitster@pobox.com, rhodges@cisco.com,
	rphodges@gmail.com

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.



^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: Re* [PATCH v2] apply: make --intent-to-add not stomp index
@ 2021-11-06 11:24 Johannes Altmanninger
  2021-11-06 11:42 ` [PATCH v3] apply: --intent-to-add should imply --index Johannes Altmanninger
  0 siblings, 1 reply; 16+ messages in thread
From: Johannes Altmanninger @ 2021-11-06 11:24 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: rhodges, git, rphodges

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
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2025-05-13 17:53 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-01  9:03 [PATCH v3] apply: --intent-to-add should imply --index Jason Cho
2025-05-01 12:48 ` Kristoffer Haugsbakk
2025-05-01 16:31   ` Junio C Hamano
2025-05-02  4:11     ` Ryan Hodges
2025-05-03  3:51       ` Raymond E. Pasco
2025-05-03  8:22         ` Raymond E. Pasco
2025-05-11  0:36         ` [PATCH 0/5] apply: fix apply --intent-to-add Raymond E. Pasco
2025-05-11  0:36           ` [PATCH 1/5] apply: error on --intent-to-add outside gitdir Raymond E. Pasco
2025-05-11  0:36           ` [PATCH 2/5] apply: read in the index in --intent-to-add mode Raymond E. Pasco
2025-05-12  2:13             ` Raymond E. Pasco
2025-05-13 17:52               ` Jason Cho
2025-05-11  0:36           ` [PATCH 3/5] apply: only write intents to add for new files Raymond E. Pasco
2025-05-11  0:36           ` [PATCH 4/5] t4140: test apply --intent-to-add interactions Raymond E. Pasco
2025-05-11  0:36           ` [PATCH 5/5] apply docs: clarify wording for --intent-to-add Raymond E. Pasco
  -- strict thread matches above, loose matches on Subject: below --
2021-11-06 11:24 Re* [PATCH v2] apply: make --intent-to-add not stomp index Johannes Altmanninger
2021-11-06 11:42 ` [PATCH v3] apply: --intent-to-add should imply --index Johannes Altmanninger
2021-11-06 11:47   ` Johannes Altmanninger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).