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

René Scharfe [off-list ref] writes:

quoted

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;

quoted

 	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.

Errors in the patches fed as its input are caught by "if we do not
know if the patch is to add a new path yet, then declare it is, but
if we do know the patch is _NOT_ adding a new path, barf if that
path is not there" and other checks in this function, and changing
the assert to "if already new, then make it a no-op" defeats the
whole point of having an assert (and just removing it is even worse).

Thanks.