Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> Suppose that you came up with some contents to register at path F in
> your working tree, told Git about your intention with "add -N F", and
> then tried to apply a patch that wants to _create_ F:
>
> Without this patch, we would say "F already exists so a patch to
> create is incompatible with our current state".
>
> With this patch, i-t-a entries are ignored and we do not say that, but
> instead we'll hopefully trigger "does it exist in the working tree"
> check, unless you are running under "--cached".
>
> Which means that this change will not lead to data loss in the
> "untracked" file F in the working tree that was merely added to the
> index with i-t-a bit.
>
> (commit message mostly from Junio)
Hmm, I do not quite recall. The above looks under-explained anyway;
we should stress the fact that it is a designed behaviour of this
change to allow only "apply --cached" and continue rejecting other
forms of "apply", but the above makes it sound like it is merely a
coincidence.
It might make sense, from purely mechanistic point of view, to allow
"git apply --cached" to create in the above scenario, but it does
not make any sense to allow "git apply" or "git apply --index", both
of which modifies the working tree (and I do not think the patch
allows the former; I think the latter would fail correctly, but it
may leave the index in a weird state--I didn't check).
"git add -N F" is like saying "I am telling you that F _exists_; I
am just not telling you what its exact contents are yet". It's like
reserving a table in a restaurant. The diner might not have arrived
yet, but that does not mean you can give the table to somebody else.
You need to wait for the reservation to be canceled (which you would
do by "git rm --cached F") before you give the table to somebody
else (i.e. creation by the patch).
So from that point of view, I am not convinced "git apply --cached"
should be allowed in such a case, though.
"I thought I told you to that I'll add to this path, but you chose
not to notice that the patch I tried to apply would create the path
with totally different contents and now I am getting from 'git diff'
nonsensical comparison" would be a plausible complaint if we took
this patch. What is the practical benefit of automatically and
silently canceling the reservation made by an earlier "add -N"?
What workflow benefits from this change, and is this the best
solution to help that workflow?
> Reported-by: Patrick Higgins <phiggins@xxxxxxxxxx>
> Reported-by: Bjørnar Snoksrud <snoksrud@xxxxxxxxx>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---
> builtin/apply.c | 9 +++++----
> t/t2203-add-intent.sh | 13 +++++++++++++
> 2 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 0769b09..315fce8 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -3550,10 +3550,11 @@ static int check_to_create(const char *new_name, int ok_if_exists)
> {
> struct stat nst;
>
> - if (check_index &&
> - cache_name_pos(new_name, strlen(new_name)) >= 0 &&
> - !ok_if_exists)
> - return EXISTS_IN_INDEX;
> + if (check_index && !ok_if_exists) {
> + int pos = cache_name_pos(new_name, strlen(new_name));
> + if (pos >= 0 && !ce_intent_to_add(active_cache[pos]))
> + return EXISTS_IN_INDEX;
> + }
> if (cached)
> return 0;
>
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 2a4a749..bb5ef2b 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -82,5 +82,18 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
> test_cmp expect actual
> '
>
> +test_expect_success 'apply adds new file on i-t-a entry' '
> + git init apply &&
> + (
> + cd apply &&
> + echo newcontent >newfile &&
> + git add newfile &&
> + git diff --cached >patch &&
> + rm .git/index &&
> + git add -N newfile &&
> + git apply --cached patch
> + )
> +'
> +
> test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html