Re: [PATCH] builtin-apply: check for empty files when detecting creation patch

[Junio C Hamano <gitster@pobox.com>]

Imre Deak <imre.deak@gmail.com> 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?

> @@ -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.