| Message ID | 20211030205147.2503327-1-aclopte@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] apply: make --intent-to-add not stomp index | expand |
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;
On Sun, Oct 31, 2021 at 11:40:05PM -0700, Junio C Hamano wrote: > 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. I must confess, I didn't even consider alternative solutions. > > 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. To be clear, check_index *disables* the unsafe_paths check, but it enables a stronger check: verify_index_match(), which makes sure that the touched paths exist in the index.
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; 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 '
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(-)