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

[Junio C Hamano <gitster@pobox.com>]

Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:

> diff --git a/builtin-apply.c b/builtin-apply.c
> index 1926cd8..6f6bf85 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -2271,6 +2271,16 @@ static struct patch *in_fn_table(const char *name)
>  	return NULL;
>  }
>  
> +static int to_be_deleted(struct patch *patch)
> +{
> +	return patch == (struct patch *) -2;
> +}
> +
> +static int was_deleted(struct patch *patch)
> +{
> +	return patch == (struct patch *) -1;
> +}

Please use more descriptive symbolic constants, and add a comment.
Perhaps:

    /*
     * item->util in the filename table records the status of the path.
     * Usually it points at a patch (whose result records the contents
     * of it after applying it), but it could be PATH_WAS_DELETED for a
     * path that a previously applied patch has already removed.
     */
    #define PATH_TO_BE_DELETED ((struct patch *) -2)
    #define PATH_WAS_DELETED ((struct patch *) -1)

> @@ -2295,6 +2305,24 @@ static void add_to_fn_table(struct patch *patch)
> ...
> +static void prepare_fn_table(struct patch *patch)
> +{
> +	/*
> +	 * store information about incoming file deletion
> +	 */
> +	while (patch) {
> +		if ((patch->new_name == NULL) || (patch->is_rename)) {
> +			struct string_list_item *item =
> +				string_list_insert(patch->old_name, &fn_table);
> +			item->util = (struct patch *) -2;
> +		}
> +		patch = patch->next;
> +	}
> +}

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?

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.

> @@ -2410,6 +2438,8 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
>  			return error("%s: %s", old_name, strerror(errno));
>  	}
>  
> +	if(to_be_deleted(tpatch)) tpatch = NULL;
> +

Style;

	if (to_be_deleted(tpatch))
        	tpatch = NULL;

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