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

[Johannes Altmanninger <aclopte@gmail.com>]

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
>