Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files

[Lars Schneider <larsxschneider@gmail.com>]

> On 06 Oct 2017, at 06:54, Jeff King <peff@peff.net> wrote:
> 
> On Fri, Oct 06, 2017 at 08:01:48AM +0900, Junio C Hamano wrote:
> 
>>> But
>>> I think we'd want to protect the read_blob_entry() call at the top of
>>> the case with a check for dco->state == CE_RETRY.
>> 
>> Yeah, I think that makes more sense.
>> 
>> A patch may look like this on top of these two patches, but I'd
>> prefer to see Lars's eyeballing and possibly wrapping it up in an
>> applicable patch after taking the authorship.
> 

This looks all good to me. Thank you!
A few minor style suggestions below.


> ...
> 
> The "structured" way, of course, would be to put everything under
> write_out_file into a helper function and just call it from both places
> rather than relying on a spaghetti of gotos and switch-breaks.
> 
> I'm OK with whatever structure we end up with, as long as it fixes the
> leak (and ideally the pessimization).
> 
> Anyway, here's the real patch in case anybody wants to apply it and play
> with it further.
> 
> -- >8 --
> diff --git a/entry.c b/entry.c
> index 1c7e3c11d5..d28b42d82d 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -261,6 +261,7 @@ static int write_entry(struct cache_entry *ce,
> 	size_t newsize = 0;
> 	struct stat st;
> 	const struct submodule *sub;
> +	struct delayed_checkout *dco = state->delayed_checkout;
> 
> 	if (ce_mode_s_ifmt == S_IFREG) {
> 		struct stream_filter *filter = get_stream_filter(ce->name,
> @@ -273,55 +274,61 @@ static int write_entry(struct cache_entry *ce,
> 	}
> 
> 	switch (ce_mode_s_ifmt) {
> -	case S_IFREG:
> 	case S_IFLNK:
> 		new = read_blob_entry(ce, &size);
> 		if (!new)
> 			return error("unable to read sha1 file of %s (%s)",
> 				path, oid_to_hex(&ce->oid));
> 
> -		if (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {
> -			ret = symlink(new, path);
> -			free(new);
> -			if (ret)
> -				return error_errno("unable to create symlink %s",
> -						   path);

Nit: This could go into one line now.


> -			break;
> -		}
> +		/* fallback to handling it like a regular file if we must */
> +		if (!has_symlinks || to_tempfile)
> +			goto write_out_file;
> 
> +		ret = symlink(new, path);
> +		free(new);
> +		if (ret)
> +			return error_errno("unable to create symlink %s",
> +					   path);
> +		break;
> +
> +	case S_IFREG:
> 		/*
> 		 * Convert from git internal format to working tree format
> 		 */
> -		if (ce_mode_s_ifmt == S_IFREG) {
> -			struct delayed_checkout *dco = state->delayed_checkout;
> -			if (dco && dco->state != CE_NO_DELAY) {
> -				/* Do not send the blob in case of a retry. */
> -				if (dco->state == CE_RETRY) {

Maybe we could add here something like:
            /* The filer process got the blob already in case of a retry. Unnecessary to send it, again! */

> -					new = NULL;
> -					size = 0;
> -				}
> -				ret = async_convert_to_working_tree(
> -					ce->name, new, size, &buf, dco);

Nit: This could go into one line now.


> -				if (ret && string_list_has_string(&dco->paths, ce->name)) {
> -					free(new);
> -					goto finish;
> -				}
> -			} else
> -				ret = convert_to_working_tree(
> -					ce->name, new, size, &buf);

Nit: This could go into one line now.


> 
> -			if (ret) {
> +		if (dco && dco->state == CE_RETRY) {
> +			new = NULL;
> +			size = 0;
> +		} else {
> +			new = read_blob_entry(ce, &size);
> +			if (!new)
> +				return error ("unable to read sha1 file of %s (%s)",
> +					      path, oid_to_hex(&ce->oid));
> +		}
> +
> +		if (dco && dco->state != CE_NO_DELAY) {
> +			ret = async_convert_to_working_tree(
> +							    ce->name, new, size, &buf, dco);
> +			if (ret && string_list_has_string(&dco->paths, ce->name)) {
> 				free(new);
> -				new = strbuf_detach(&buf, &newsize);
> -				size = newsize;
> +				goto finish;
> 			}
> -			/*
> -			 * No "else" here as errors from convert are OK at this
> -			 * point. If the error would have been fatal (e.g.
> -			 * filter is required), then we would have died already.
> -			 */
> +		} else
> +			ret = convert_to_working_tree(
> +						      ce->name, new, size, &buf);
> +
> +		if (ret) {
> +			free(new);
> +			new = strbuf_detach(&buf, &newsize);
> +			size = newsize;
> 		}
> +		/*
> +		 * No "else" here as errors from convert are OK at this
> +		 * point. If the error would have been fatal (e.g.
> +		 * filter is required), then we would have died already.
> +		 */
> 
> +write_out_file:
> 		fd = open_output_fd(path, ce, to_tempfile);
> 		if (fd < 0) {
> 			free(new);

...

>		break;
>	case S_IFGITLINK:

Maybe add a newline above "S_IFGITLINK" and "default" for readability. 
Above "case S_IFREG:" we have a newline, too. 


- Lars