When we can only guess if we have a creation patch, we do
this by treating the patch as such if there weren't any old
lines. Zero length files can be patched without old lines
though, so do an extra check for file size.
Signed-off-by: Imre Deak <redacted>
---
builtin-apply.c | 34 +++++++++++++++++++++++++++++++++-
1 files changed, 33 insertions(+), 1 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index caa3f2a..80d0779 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1096,6 +1096,33 @@ static int parse_fragment(char *line, unsigned long size,
return offset;
}
+static size_t existing_file_size(const char *file)
+{
+ size_t st_size = -1;
+
+ if (file == NULL)
+ return -1;
+
+ if (cached) {
+ struct cache_entry *ce;
+ int pos;
+
+ pos = cache_name_pos(file, strlen(file));
+ if (pos < 0)
+ return -1;
+ ce = active_cache[pos];
+ st_size = ntohl(ce->ce_size);
+ } else {
+ struct stat st;
+
+ if (lstat(file, &st) < 0)
+ return -1;
+ st_size = st.st_size;
+ }
+
+ return st_size;
+}
+
static int parse_single_patch(char *line, unsigned long size, struct patch *patch)
{
unsigned long offset = 0;@@ -1143,13 +1170,18 @@ 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;
+ /* FIXME: How can be there context if it's a creation / deletion? */
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.
+ *
+ * It's also possible that a zero length file is added
+ * to.
*/
- if (patch->is_new < 0 && !oldlines) {
+ if (patch->is_new < 0 && !oldlines &&
+ existing_file_size(patch->old_name) != 0) {
patch->is_new = 1;
patch->old_name = NULL;
}--
1.5.4.2.dirty
Imre Deak [off-list ref] writes:
When we can only guess if we have a creation patch, we do
this by treating the patch as such if there weren't any old
lines. Zero length files can be patched without old lines
though, so do an extra check for file size.
You described what your patch does, but you did not explain why it is a
good addition. One way to do so is to illustrate in what occasion what
the existing code does is insufficient.
+static size_t existing_file_size(const char *file)
+{
+ size_t st_size = -1;
+
+ if (file == NULL)
+ return -1;
+ if (cached) {
+ struct cache_entry *ce;
+ int pos;
+
+ pos = cache_name_pos(file, strlen(file));
+ if (pos < 0)
+ return -1;
+ ce = active_cache[pos];
+ st_size = ntohl(ce->ce_size);
ntohl()? I thought ce->ce_* are host-native byte order these days...
+ } else {
+ struct stat st;
+
+ if (lstat(file, &st) < 0)
+ return -1;
Doesn't this break the use case where "git-apply --stat" is used as an
improved diffstat outside a git repository?
quoted hunk
@@ -1143,13 +1170,18 @@ 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;
+ /* FIXME: How can be there context if it's a creation / deletion? */
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.
+ *
+ * It's also possible that a zero length file is added
+ * to.
*/
- if (patch->is_new < 0 && !oldlines) {
+ if (patch->is_new < 0 && !oldlines &&
+ existing_file_size(patch->old_name) != 0) {
patch->is_new = 1;
patch->old_name = NULL;
}
The user did not say the patch was produced without context, or we do have
context. The latter cannot be a creation patch so the new logic is not
appropriate. But let's forget that problem for now and look at the case
where the patch did _not_ have any context, i.e. only added and deleted
lines.
If the patch did not have context, and the user did not ask for -u0 patch
when it was produced, it could be a creation patch, but if there are
deleted lines it cannot be. That is the original logic.
After your patch, the original logic is allowed to decide that the patch
is a creation _only if_ you happen to already have a file that is _to be
created_ in the work tree with some existing contents, or the file does
not exist. I do not see a sane logic behind that. If you were making
sure that the work tree does _not_ have the file, then I would understand,
even though I think it is wrong for "apply --stat" case. If you see a
file in the work tree, and if you assume the patch would apply to the
work tree, then the patch cannot be creation!
In general, it is not right to look at the work tree to decide how to
interpret what the patch means to begin with, but maybe you are trying to
use work tree status as a hint to disambiguate a corner case that the
information in a patch we are reading is insufficient, in which case it
might be Ok. But I cannot tell what that corner case is.
I am lost. Please explain what you are trying to fix first before
explaining how you attempted to fix it.
Hi,
thanks for your answer.
On Sun, May 11, 2008 at 5:36 AM, Junio C Hamano [off-list ref] wrote:
Imre Deak [off-list ref] writes:
> When we can only guess if we have a creation patch, we do
> this by treating the patch as such if there weren't any old
> lines. Zero length files can be patched without old lines
> though, so do an extra check for file size.
You described what your patch does, but you did not explain why it is a
good addition. One way to do so is to illustrate in what occasion what
the existing code does is insufficient.
The patch makes it possible to apply foreign patches (not created with
git diff) to zero length files already existing in the index. The problem:
$ git init
Initialized empty Git repository in .git/
$ rm -rf a
$ touch a
$ git add a
$ git commit -madd
Created initial commit 818f2b7: add
0 files changed, 0 insertions(+), 0 deletions(-)
create mode 100644 a
$ echo 123 > a.new
$ diff -u a a.new > patch
$ git apply patch
error: a: already exists in working directory
The error happens because git guesses that `patch` is a creation patch
and since `a` already exists in the index it will bail out. With the
modification
foreign patches won't be guessed to be a creation patch if the file to be
patch already exists in the index or the working directory.
> +static size_t existing_file_size(const char *file)
> +{
> + size_t st_size = -1;
> +
> + if (file == NULL)
> + return -1;
> + if (cached) {
> + struct cache_entry *ce;
> + int pos;
> +
> + pos = cache_name_pos(file, strlen(file));
> + if (pos < 0)
> + return -1;
> + ce = active_cache[pos];
> + st_size = ntohl(ce->ce_size);
ntohl()? I thought ce->ce_* are host-native byte order these days...
Sorry, I actually created the patch against an older version. It should be
without ntohl().
> + } else {
> + struct stat st;
> +
> + if (lstat(file, &st) < 0)
> + return -1;
Doesn't this break the use case where "git-apply --stat" is used as an
improved diffstat outside a git repository?
In that case we'll return error (file doesn't exist) which will leave
the possibility
open to guess the patch to be a creation patch. With the change when
doing --stat the patch might or might not be changed to a creation patch
based on the current working directory content. Though I think this doesn't
result in any difference in the --stat output one could argue that --stat's
operation shouldn't depend in any way on the working directory content
except when --check is specified in addition. So probably we should special
case --stat here...
> @@ -1143,13 +1170,18 @@ 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;
> + /* FIXME: How can be there context if it's a creation / deletion? */
> 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.
> + *
> + * It's also possible that a zero length file is added
> + * to.
> */
> - if (patch->is_new < 0 && !oldlines) {
> + if (patch->is_new < 0 && !oldlines &&
> + existing_file_size(patch->old_name) != 0) {
> patch->is_new = 1;
> patch->old_name = NULL;
> }
The user did not say the patch was produced without context, or we do have
context. The latter cannot be a creation patch so the new logic is not
appropriate. But let's forget that problem for now and look at the case
where the patch did _not_ have any context, i.e. only added and deleted
lines.
If the patch did not have context, and the user did not ask for -u0 patch
when it was produced, it could be a creation patch, but if there are
deleted lines it cannot be. That is the original logic.
So as far as I understand creation or deletion patches cannot have any context
lines. If that's correct shouldn't the if clause
if (!unidiff_zero || context)
read instead
if (!unidiff_zero)
?
After your patch, the original logic is allowed to decide that the patch
is a creation _only if_ you happen to already have a file that is _to be
created_ in the work tree with some existing contents, or the file does
not exist. I do not see a sane logic behind that. If you were making
sure that the work tree does _not_ have the file, then I would understand,
even though I think it is wrong for "apply --stat" case. If you see a
file in the work tree, and if you assume the patch would apply to the
work tree, then the patch cannot be creation!
You are right, that condition is wrong. The right one then should be:
existing_file_size(patch->old_name) < 0
In general, it is not right to look at the work tree to decide how to
interpret what the patch means to begin with, but maybe you are trying to
use work tree status as a hint to disambiguate a corner case that the
information in a patch we are reading is insufficient, in which case it
might be Ok. But I cannot tell what that corner case is.
Hm, I agree with that if it's only a --stat, but if it's --check or
--apply I think
there is no other way to guess if we really have a creation patch or only an
add-to-a-zero-length-file patch.
--Imre
I am lost. Please explain what you are trying to fix first before
explaining how you attempted to fix it.