Re: [PATCH 4/8] apply: make sure check_preimage() does not leave empty file on error
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 25, 2015, 17:16 UTC
- Message-ID
- <xmqqd1yb1e7a.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1440205700-19749-5-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 15 quoted lines
> The test case probably describes the test scenario the best. We have a > patch to modify some file but the base file is gone. Because > check_preimage() finds an index entry with the same old_name, it tries > to restore the on-disk base file with cached content with > checkout_target() and move on. > > If this old_name is i-t-a, before this patch "apply -3" will call > checkout_target() which will write an empty file (i-t-a entries always > have "empty blob" SHA-1), then it'll fail at verify_index_match() > (i-t-a entries are always "modified") and the operation is aborted. > > This empty file should not be created. Compare to the case where > old_name does not exist in index, "apply -3" fails with a different > error message "...: does not exist" but it does not touch > worktree. This patch makes sure the same happens to i-t-a entries.
This part (unlike 3/8) is very well explained, and makes sense to me.
> load_current() shares the same code pattern (look up an index entry, > if on-disk version is not found, restore using the cached > version).
The "fall-back to the cached one" is very much deliberate to allow us to work with an empty working tree as long as the index is correct, so this perhaps makes sense. If the user said "add -N F", and F is involved in the patch application, we do not want to consider F exists in the index and we would instead want to pretend that we do not have F ourselves. It does not even make sense to do "apply -3" for such a path, and rejecting with "does not exist" is a good thing to do.
Good.
> Fix it too (even though it's not exercised in any test case)
Hmm, as this is adding new test for the other case, perhaps this case is covered by another one, too?
Show 53 quoted lines
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> builtin/apply.c | 4 ++--
> t/t2203-add-intent.sh | 16 ++++++++++++++++
> 2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 76b58a1..b185c97 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -3348,7 +3348,7 @@ static int load_current(struct image *image, struct patch *patch)
> die("BUG: patch to %s is not a creation", patch->old_name);
>
> pos = cache_name_pos(name, strlen(name));
> - if (pos < 0)
> + if (pos < 0 || ce_intent_to_add(active_cache[pos]))
> return error(_("%s: does not exist in index"), name);
> ce = active_cache[pos];
> if (lstat(name, &st)) {
> @@ -3501,7 +3501,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
>
> if (check_index && !previous) {
> int pos = cache_name_pos(old_name, strlen(old_name));
> - if (pos < 0) {
> + if (pos < 0 || ce_intent_to_add(active_cache[pos])) {
> if (patch->is_new < 0)
> goto is_new;
> return error(_("%s: does not exist in index"), old_name);
> 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
> + )
> +'
> +
> test_done