Johannes Altmanninger <aclopte@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Johannes Altmanninger <aclopte@xxxxxxxxx>
> ---
>
> 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;
