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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 28, 2015, 18:35 UTC
Message-ID: <xmqqbn9al8v9.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1451181092-26054-5-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 23 quoted lines

> 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.