| Message ID | xmqqfssgcq1b.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Re* [PATCH v2] apply: make --intent-to-add not stomp index | expand |
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 >
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; 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 '