Re: [PATCH 2/2] apply: handle assertion failure gracefully

["René Scharfe" <l.s.r@web.de>]

Am 27.02.2017 um 23:33 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
> 
>> Am 27.02.2017 um 21:04 schrieb Junio C Hamano:
>>> René Scharfe <l.s.r@web.de> writes:
>>>
>>>>> diff --git a/apply.c b/apply.c
>>>>> index cbf7cc7f2..9219d2737 100644
>>>>> --- a/apply.c
>>>>> +++ b/apply.c
>>>>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
>>>>>  	if (!old_name)
>>>>>  		return 0;
>>>>>
>>>>> -	assert(patch->is_new <= 0);
>>>>
>>>> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that
>>>> line. Its intent was to handle diffs that contain an old name even for
>>>> a file that's created.  Citing from its commit message: "When we
>>>> cannot be sure by parsing the patch that it is not a creation patch,
>>>> we shouldn't complain when if there is no such a file."  Why not stop
>>>> complaining also in case we happen to know for sure that it's a
>>>> creation patch? I.e., why not replace the assert() with:
>>>>
>>>> 	if (patch->is_new == 1)
>>>> 		goto is_new;
>>>>
>>>>>  	previous = previous_patch(state, patch, &status);
>>>
>>> When the caller does know is_new is true, old_name must be made/left
>>> NULL.  That is the invariant this assert is checking to catch an
>>> error in the calling code.
>>
>> There are some places in apply.c that set ->is_new to 1, but none of
>> them set ->old_name to NULL at the same time.
> 
> I thought all of these are flipping ->is_new that used to be -1
> (unknown) to (now we know it is new), and sets only new_name without
> doing anything to old_name, because they know originally both names
> are set to NULL.
> 
>> Having to keep these two members in sync sounds iffy anyway.  Perhaps
>> accessors can help, e.g. a setter which frees old_name when is_new is
>> set to 1, or a getter which returns NULL for old_name if is_new is 1.
> 
> Definitely, the setter would make it harder to make the mistake.

When I added setters, apply started to passed NULL to unlink(2) and
rmdir(2) in some of the new tests, which still failed.

That's because three of the diffs trigger both gitdiff_delete(), which
sets is_delete and old_name, and gitdiff_newfile(), which sets is_new
and new_name.  Create and delete equals move, right?  Or should we
error out at this point already?

The last new diff adds a new file that is copied.  Sounds impossible.
How about something like this, which forbids combinations that make no
sense.  Hope it's not too strict; at least all tests succeed.

---
 apply.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 61 insertions(+), 18 deletions(-)

diff --git a/apply.c b/apply.c
index 21b0bebec5..6cb6860511 100644
--- a/apply.c
+++ b/apply.c
@@ -197,6 +197,14 @@ struct fragment {
 #define BINARY_DELTA_DEFLATED	1
 #define BINARY_LITERAL_DEFLATED 2
 
+enum patch_type {
+	CHANGE,
+	CREATE,
+	DELETE,
+	RENAME,
+	COPY
+};
+
 /*
  * This represents a "patch" to a file, both metainfo changes
  * such as creation/deletion, filemode and content changes represented
@@ -205,6 +213,7 @@ struct fragment {
 struct patch {
 	char *new_name, *old_name, *def_name;
 	unsigned int old_mode, new_mode;
+	enum patch_type type;
 	int is_new, is_delete;	/* -1 = unknown, 0 = false, 1 = true */
 	int rejected;
 	unsigned ws_rule;
@@ -229,6 +238,36 @@ struct patch {
 	struct object_id threeway_stage[3];
 };
 
+static int set_patch_type(struct patch *patch, enum patch_type type)
+{
+	if (patch->type != CHANGE && patch->type != type)
+		return error(_("conflicting patch types"));
+	patch->type = type;
+	switch (type) {
+	case CHANGE:
+		break;
+	case CREATE:
+		patch->is_new = 1;
+		patch->is_delete = 0;
+		free(patch->old_name);
+		patch->old_name = NULL;
+		break;
+	case DELETE:
+		patch->is_new = 0;
+		patch->is_delete = 1;
+		free(patch->new_name);
+		patch->new_name = NULL;
+		break;
+	case RENAME:
+		patch->is_rename = 1;
+		break;
+	case COPY:
+		patch->is_copy = 1;
+		break;
+	}
+	return 0;
+}
+
 static void free_fragment_list(struct fragment *list)
 {
 	while (list) {
@@ -907,13 +946,13 @@ static int parse_traditional_patch(struct apply_state *state,
 		}
 	}
 	if (is_dev_null(first)) {
-		patch->is_new = 1;
-		patch->is_delete = 0;
+		if (set_patch_type(patch, CREATE))
+			return -1;
 		name = find_name_traditional(state, second, NULL, state->p_value);
 		patch->new_name = name;
 	} else if (is_dev_null(second)) {
-		patch->is_new = 0;
-		patch->is_delete = 1;
+		if (set_patch_type(patch, DELETE))
+			return -1;
 		name = find_name_traditional(state, first, NULL, state->p_value);
 		patch->old_name = name;
 	} else {
@@ -922,12 +961,12 @@ static int parse_traditional_patch(struct apply_state *state,
 		name = find_name_traditional(state, second, first_name, state->p_value);
 		free(first_name);
 		if (has_epoch_timestamp(first)) {
-			patch->is_new = 1;
-			patch->is_delete = 0;
+			if (set_patch_type(patch, CREATE))
+				return -1;
 			patch->new_name = name;
 		} else if (has_epoch_timestamp(second)) {
-			patch->is_new = 0;
-			patch->is_delete = 1;
+			if (set_patch_type(patch, DELETE))
+				return -1;
 			patch->old_name = name;
 		} else {
 			patch->old_name = name;
@@ -1031,7 +1070,8 @@ static int gitdiff_delete(struct apply_state *state,
 			  const char *line,
 			  struct patch *patch)
 {
-	patch->is_delete = 1;
+	if (set_patch_type(patch, DELETE))
+		return -1;
 	free(patch->old_name);
 	patch->old_name = xstrdup_or_null(patch->def_name);
 	return gitdiff_oldmode(state, line, patch);
@@ -1041,7 +1081,8 @@ static int gitdiff_newfile(struct apply_state *state,
 			   const char *line,
 			   struct patch *patch)
 {
-	patch->is_new = 1;
+	if (set_patch_type(patch, CREATE))
+		return -1;
 	free(patch->new_name);
 	patch->new_name = xstrdup_or_null(patch->def_name);
 	return gitdiff_newmode(state, line, patch);
@@ -1051,7 +1092,8 @@ static int gitdiff_copysrc(struct apply_state *state,
 			   const char *line,
 			   struct patch *patch)
 {
-	patch->is_copy = 1;
+	if (set_patch_type(patch, COPY))
+		return -1;
 	free(patch->old_name);
 	patch->old_name = find_name(state, line, NULL, state->p_value ? state->p_value - 1 : 0, 0);
 	return 0;
@@ -1061,7 +1103,8 @@ static int gitdiff_copydst(struct apply_state *state,
 			   const char *line,
 			   struct patch *patch)
 {
-	patch->is_copy = 1;
+	if (set_patch_type(patch, COPY))
+		return -1;
 	free(patch->new_name);
 	patch->new_name = find_name(state, line, NULL, state->p_value ? state->p_value - 1 : 0, 0);
 	return 0;
@@ -1071,7 +1114,8 @@ static int gitdiff_renamesrc(struct apply_state *state,
 			     const char *line,
 			     struct patch *patch)
 {
-	patch->is_rename = 1;
+	if (set_patch_type(patch, RENAME))
+		return -1;
 	free(patch->old_name);
 	patch->old_name = find_name(state, line, NULL, state->p_value ? state->p_value - 1 : 0, 0);
 	return 0;
@@ -1081,7 +1125,8 @@ static int gitdiff_renamedst(struct apply_state *state,
 			     const char *line,
 			     struct patch *patch)
 {
-	patch->is_rename = 1;
+	if (set_patch_type(patch, RENAME))
+		return -1;
 	free(patch->new_name);
 	patch->new_name = find_name(state, line, NULL, state->p_value ? state->p_value - 1 : 0, 0);
 	return 0;
@@ -3704,10 +3749,8 @@ static int check_preimage(struct apply_state *state,
 	return 0;
 
  is_new:
-	patch->is_new = 1;
-	patch->is_delete = 0;
-	free(patch->old_name);
-	patch->old_name = NULL;
+	if (set_patch_type(patch, CREATE))
+		return -1;
 	return 0;
 }
 
-- 
2.12.0