Re: [PATCH] builtin-apply: keep information about files to be deleted

Junio C Hamano [off-list ref] wrote:

This PATH_TO_BE_DELETED logic should be Ok for the normal case, but it
seems a bit fragile.  In a sequence of patches, if you have even one
patch that makes the path disappear, you initialize it as
PATH_TO_BE_DELETED, and special case the "creation should not clobber
existing path" rule to allow it to be present in the tree.

That may make this sequence work, I presume, with your change:

	patch #1	renames frotz.c to hello.c
      patch #2	renames hello.c to frotz.c

because of patch #2, hello.c is marked as PATH_TO_BE_DELETED
initially and then when patch #1 is handled, frotz.c is allowed to
replace it.

But if you have further patches that do the following (the "file
table" mechanism was added to handle concatenated patches that affect
the same path more than once), I thing PATH_TO_BE_DELETED logic would
break down:

      patch #3	renames alpha.c to hello.c
	patch #4	renames hello.c to alpha.c

When patch #3 is handled, the PATH_TO_BE_DELETED mark is long gone
from hello.c, and we will see the same failure you addressed in your
patch, won't we?

As far as I understand the code, diffs are applied independently
(for every file apply_patch() is called) and for every apply_patch()
call fn_table is cleared. So situation you described in only possible
in a *single* diff and I don't think it is possible to happen. 

Performing two criss-cross renames results in following diff:

	mv file1 tmp
	mv file2 file1
	mv tmp file2

	mv file1 tmp
	mv file3 file1
	mv tmp file3

	git diff -M -B

diff --git a/file3 b/file1
similarity index 100%
rename from file3
rename to file1
diff --git a/file1 b/file2
similarity index 100%
rename from file1
rename to file2
diff --git a/file2 b/file3
similarity index 100%
rename from file2
rename to file3

However, sanity checking still may be performed and error printed on
situations which cannot be resolved.

The prepare_fn_table() may be a good place to diagnose such a
situation and warn or error out if the user feeds such an input we
cannot handle sanely.

I'll add some checks to this function as you suggest.

Style;

	if (to_be_deleted(tpatch))
        	tpatch = NULL;

Other than that, I think it is a sensible approach.

Thanks for feedback.

-- 
Michał Kiedrowicz