Re: [PATCH] builtin-apply: keep information...

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:36

Michał Kiedrowicz [off-list ref] writes:

quoted hunk

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)

quoted hunk

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

quoted hunk

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

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

From: Michał Kiedrowicz <hidden>
Date: 2016-06-15 22:46:36

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help