Re: [PATCH v4] git-apply: apply submodule changes

From: Sven Verdoolaege <skimo@kotnet.org>
Date: Aug 14, 2007, 08:39 UTC
Message-ID: <20070814083940.GN999MdfPADPa@greensroom.kotnet.org>
In-Reply-To: <7vd4xqeilh.fsf@assigned-by-dhcp.cox.net>

On Mon, Aug 13, 2007 at 11:27:38PM -0700, Junio C Hamano wrote:

Show 8 quoted lines

>  * write_out_one_result() calls remove_file() and create_file()
>    to match the work tree to the result you prepared with
>    apply_data().
> 
>    - remove_file() is changed not to do any for gitlink.  We
>      _might_ want to try rmdir() if there is an otherwise empty
>      directory there, but currently we cannot do much to the
>      failure on that, so I did not bother with it.

We could at least warn about it, which is what my patch did.

Show 9 quoted lines

>    - create_file() does three things:
>      - create a file in the work tree to match the result;
>      - update the index with the patch result;
>      - invalidate cache-tree entry for the path.
> 
>      For the first task, create_one_file() is usually used to
>      create a blob (either regular file or a symlink).  For
>      gitlinks, we do not affect the work tree for now, just like
>      checkout_entry().

It creates the subdirectory, though, and git-apply should do so too since it expects the subdirectory to be there for subsequent patches (at least in the --index case).

> diff --git a/builtin-apply.c b/builtin-apply.c

Did you remove the documentation on purpose ?

Show 8 quoted lines

> +static int verify_index_match(struct cache_entry *ce, struct stat *st)
> +{
> +	if (!ce_match_stat(ce, st, 1))
> +		return 0;
> +	if (S_ISGITLINK(ntohl(ce->ce_mode))) {
> +		if (S_ISDIR(st->st_mode))
> +			return 0;
> +	}

Not a big deal, but ce_match_stat already checks for that. That's why I was checking for TYPE_CHANGED in its return value.

Show 23 quoted lines

> @@ -2096,16 +2142,22 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
>  				    lstat(old_name, &st))
>  					return -1;
>  			}
> -			if (!cached)
> -				changed = ce_match_stat(ce, &st, 1);
> -			if (changed)
> +			if (!cached && verify_index_match(ce, &st))
>  				return error("%s: does not match index",
>  					     old_name);
>  			if (cached)
>  				st_mode = ntohl(ce->ce_mode);
> +		} else if (stat_ret < 0) {
> +			if (errno == ENOENT && S_ISGITLINK(patch->old_mode))
> +				/*
> +				 * It is Ok not to have the submodule
> +				 * checked out at all.
> +				 */
> +				;
> +			else
> +				return error("%s: %s", old_name,
> +					     strerror(errno));
>  		}

Shouldn't you be consistent with the --index case and require the subdirectory to exist?

skimo