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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 13, 2009, 18:51 UTC
Message-ID: <7v4owsfktw.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1239478260-7420-1-git-send-email-michal.kiedrowicz@gmail.com>

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

Show 17 quoted lines

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

Show 16 quoted lines

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

Show 6 quoted lines

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