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

From: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
Date: Apr 13, 2009, 21:03 UTC
Message-ID: <20090413230351.7cbb01f5@gmail.com>
In-Reply-To: <7v4owsfktw.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> wrote:

Show 26 quoted lines

> 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