Re: [PATCH v2] apply: make --intent-to-add not stomp index
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 1, 2021, 06:40 UTC
- Message-ID
- <xmqqr1c0cray.fsf@gitster.g>
- In-Reply-To
- <20211030205147.2503327-1-aclopte@gmail.com>
Johannes Altmanninger <aclopte@gmail.com> writes:
Show 32 quoted lines
> 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;