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

[Junio C Hamano <gitster@pobox.com>]

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;