On Mon, Nov 01, 2021 at 12:07:28AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> 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@xxxxxxxxx>
> Test-by: Johannes Altmanninger <aclopte@xxxxxxxxx>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> ---
> 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
>
