Hi,
thanks for your answer.
On Sun, May 11, 2008 at 5:36 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Imre Deak <imre.deak@xxxxxxxxx> 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.
--
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
