Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:
> I think this is the fundamental problem:
>
> ..
> if (patch->is_new < 0 && !oldlines) {
> patch->is_new = 1;
> ..
>
> because that logic simply isn't right. (is_new < 0 && !oldlines) does
> *not* mean that it must be new.
>
> We can say it the other way around, of course:
>
> if (patch->is_new < 0 && oldlines)
> patch->is_new = 0;
>
> and that's a valid rule, but I think we already would never set "is_new"
> to -1 if we had old lines, so that would probably be a pointless thing to
> do.
Yeah, in fact we have this:
if (patch->is_new < 0 &&
(oldlines || (patch->fragments && patch->fragments->next)))
patch->is_new = 0;
a several lines above the piece you quoted in parse_single_patch().
> So: remove the check for (is_new < 0 && !oldlines) because it doesn't
> actually add any information, and leave "is_new" as unknown until later
> when we actually *see* that file or not. Hmm?
That leads to the similar logic to remove "If we do not know if it is
delete or not, not adding any lines (or any replacement lines) means
delete" logic as well, which in turn means the whole if (!unidiff_zero...)
block would go.
Which actually is a good thing, I think. As Imre mentioned, I do not
think
if (!unidiff_zero || context)
makes sense. I cannot guess what the intention of that was.
commit 4be609625e48e908f2b76d35bfeb61a8ba3a83a0
Author: Junio C Hamano <junkio@xxxxxxx>
Date: Sun Sep 17 01:04:24 2006 -0700
apply --unidiff-zero: loosen sanity checks for --unidiff=0 patches
In "git-apply", we have a few sanity checks and heuristics that
expects that the patch fed to us is a unified diff with at least
one line of context.
...description of good tests)...
These sanity checks are good safety measures, but breaks down
when people feed a diff generated with --unified=0. This was
recently noticed first by Matthew Wilcox and Gerrit Pape.
This adds a new flag, --unified-zero, to allow bypassing these
checks. If you are in control of the patch generation process,
you should not use --unified=0 patch and fix it up with this
flag; rather you should try work with a patch with context. But
if all you have to work with is a patch without context, this
flag may come handy as the last resort.
Signed-off-by: Junio C Hamano <junkio@xxxxxxx>
So unless unidiff-zero is given, we would want extra checks inside that
block. Also if we have context in the patch anywhere, that means the
patch cannot be coming from "diff -u0", so we go into that block.
Even though other checks that are guarded with "if (!unidiff_zero)"
elsewhere in the code do make sense, however, this block may not do
anything useful, as you pointed out.
With the change to remove the whole block, all tests still passes, and a
limited test with this:
--- empty 2008-05-13 16:56:57.000000000 -0700
+++ empty.1 2008-05-13 16:57:07.000000000 -0700
@@ -0,0 +1 @@
+foo
to update an originally empty file "empty" also seems to work.
However, with this change, it no longer allows you to accept such a patch
and treat it as a creation of "empty". Instead we barf with "error:
empty: No such file or directory", if you do not have an empty "empty"
file in the work tree when you run "git apply" on the above patch.
When "diff" was run with flags that could produce context (that is what we
get from !unidiff_zero), if we do not see any lines that begin with '-',
the only case that it is not a creation patch is if you compared the
postimage with a preimage of size 0. Because we do not have usable
/dev/null cue, we cannot tell that case from a creation patch.
I am having a strong suspicion that doing this change is robbing Peter to
pay Imre. We simply cannot have it both ways, methinks.
---
builtin-apply.c | 17 +----------------
1 files changed, 1 insertions(+), 16 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index 1103625..395f16b 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -418,7 +418,7 @@ static int guess_p_value(const char *nameline)
}
/*
- * Get the name etc info from the --/+++ lines of a traditional patch header
+ * Get the name etc info from the ---/+++ lines of a traditional patch header
*
* FIXME! The end-of-filename heuristics are kind of screwy. For existing
* files, we can happily check the index for a match, but for creating a
@@ -1143,21 +1143,6 @@ static int parse_single_patch(char *line, unsigned long size, struct patch *patc
if (patch->is_delete < 0 &&
(newlines || (patch->fragments && patch->fragments->next)))
patch->is_delete = 0;
- if (!unidiff_zero || context) {
- /* If the user says the patch is not generated with
- * --unified=0, or if we have seen context lines,
- * then not having oldlines means the patch is creation,
- * and not having newlines means the patch is deletion.
- */
- if (patch->is_new < 0 && !oldlines) {
- patch->is_new = 1;
- patch->old_name = NULL;
- }
- if (patch->is_delete < 0 && !newlines) {
- patch->is_delete = 1;
- patch->new_name = NULL;
- }
- }
if (0 < patch->is_new && oldlines)
die("new file %s depends on old contents", patch->new_name);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
