From: Junio C Hamano <gitster@pobox•com>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 3/6] apply: fix adding new files on i-t-a entries
Date: Sun, 27 Dec 2015 19:01:05 -0800 [thread overview]
Message-ID: <xmqqpoxr9szy.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1451181092-26054-4-git-send-email-pclouds@gmail.com> ("Nguyễn Thái Ngọc Duy"'s message of "Sun, 27 Dec 2015 08:51:29 +0700")
Nguyễn Thái Ngọc Duy <pclouds@gmail•com> 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@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
next prev parent reply other threads:[~2015-12-28 3:01 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-27 1:51 [PATCH 0/6] nd/ita-cleanup updates Nguyễn Thái Ngọc Duy
2015-12-27 1:51 ` [PATCH 1/6] blame: remove obsolete comment Nguyễn Thái Ngọc Duy
2015-12-28 17:35 ` Junio C Hamano
2015-12-27 1:51 ` [PATCH 2/6] Add and use convenient macro ce_intent_to_add() Nguyễn Thái Ngọc Duy
2015-12-28 17:53 ` Junio C Hamano
2015-12-27 1:51 ` [PATCH 3/6] apply: fix adding new files on i-t-a entries Nguyễn Thái Ngọc Duy
2015-12-28 3:01 ` Junio C Hamano [this message]
2015-12-29 14:02 ` Duy Nguyen
2015-12-29 17:40 ` Junio C Hamano
2015-12-27 1:51 ` [PATCH 4/6] apply: make sure check_preimage() does not leave empty file on error Nguyễn Thái Ngọc Duy
2015-12-28 18:35 ` Junio C Hamano
2015-12-27 1:51 ` [PATCH 5/6] checkout(-index): do not checkout i-t-a entries Nguyễn Thái Ngọc Duy
2015-12-28 19:56 ` Junio C Hamano
2015-12-27 1:51 ` [PATCH 6/6] grep: make it clear i-t-a entries are ignored Nguyễn Thái Ngọc Duy
2015-12-28 19:59 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqpoxr9szy.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=pclouds@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox