Subject: Re: [PATCH 4/6] apply: make sure check_preimage() does not leave empty file on error

From: Junio C Hamano <[email-masked]>
Date: Mon, 28 Dec 2015 10:35:06 -0800

Nguyễn Thái Ngọc Duy  <[email-masked]> 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.