Re: [PATCH 3/6] apply: fix adding new files on i-t-a entries
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 28, 2015, 03:01 UTC
- Message-ID
- <xmqqpoxr9szy.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1451181092-26054-4-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 16 quoted lines
> 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?
Show 50 quoted lines
> Reported-by: Patrick Higgins <phiggins@google.com>
> Reported-by: Bjørnar Snoksrud <snoksrud@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> 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