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 4/6] apply: make sure check_preimage() does not leave empty file on error
Date: Mon, 28 Dec 2015 10:35:06 -0800 [thread overview]
Message-ID: <xmqqbn9al8v9.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1451181092-26054-5-git-send-email-pclouds@gmail.com> ("Nguyễn Thái Ngọc Duy"'s message of "Sun, 27 Dec 2015 08:51:30 +0700")
Nguyễn Thái Ngọc Duy <pclouds@gmail•com> writes:
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index bb5ef2b..96c8755 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -95,5 +95,21 @@ test_expect_success 'apply adds new file on i-t-a entry' '
> )
> '
>
> +test_expect_success 'apply:check_preimage() not creating empty file' '
> + git init check-preimage &&
> + (
> + cd check-preimage &&
> + echo oldcontent >newfile &&
> + git add newfile &&
> + echo newcontent >newfile &&
> + git diff >patch &&
> + rm .git/index &&
> + git add -N newfile &&
> + rm newfile &&
> + test_must_fail git apply -3 patch &&
> + ! test -f newfile
> + )
> +'
Unlike "apply --index" (and "apply" without any option to work on
working tree), "apply -3" is allowed to write to the working tree
even when it fails, if the failure comes from a merge conflict and
that is by design. In this case, the patch expects that there is an
existing file with 'oldcontent' so it should try to do a three-way
merge to go from an 'unspecified' content (i.e. the current state in
the index) in a way similar to going from 'oldcontent' to
'newcontent', and because we _have_ to make sure that the path that
holds the 'unspecified' content is clean between the index and the
working tree--but an earlier "add -N" said that "the index knows
about the path, I am not telling what its contents are yet", by
definition the index and the working tree cannot match so the
application _must_ fail, before even attempting to do a three-way
merge, hence it should not touch the working tree.
So expecting a failure from "apply -3" sounds like the right thing
to do, and obviously it should not leave any "newfile". The test
also must check that the index has not been modified since the last
"git add -N" (most notably, we want to see 'newfile' still has the
I-T-A bit on).
More interesting cases to test would be:
- Instead of "rm newfile", have ">newfile" to empty the contents
before applying the patch with "apply -3". We should expect the
same "apply -3 fails", but we should see an empty 'newfile' in
the working tree. The index must be the same as the last "git
add -N".
- Instead of "rm newfile", have "echo oldcontent >newfile". What
should "apply -3" do? The patch would be applicable with "apply"
without any argument (hence without involving the index), but
"-3" requires 'check_index', and again any entry with I-T-A bit
on by definition does not match between the index and the working
tree, so the application must fail, I think.
next prev parent reply other threads:[~2015-12-28 18:35 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
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 [this message]
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=xmqqbn9al8v9.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