Re: [PATCHv4 5/8] clone: factor out checking for an alternate path

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

Stefan Beller <sbeller@google.com> writes:

> +	struct strbuf sb = STRBUF_INIT;
> +	char *ref_git = compute_alternate_path(item->string, &sb);

Who owns the memory for ref_git?

> -	if (!access(mkpath("%s/shallow", ref_git), F_OK))
> -		die(_("reference repository '%s' is shallow"), item->string);
> +	if (!ref_git)
> +		die("%s", sb.buf);

Presumably the second argument to compute_alternate_path() is a
strbuf to receive the error message?  It is unfortunate that the
variable used for this purpose is a bland "sb", but perhaps that
cannot be helped as you would reuse that strbuf for a different
purpose (i.e. not to store the error message, but to formulate a
pathname).

> -	if (!access(mkpath("%s/info/grafts", ref_git), F_OK))
> -		die(_("reference repository '%s' is grafted"), item->string);
> +	strbuf_addf(&sb, "%s/objects", ref_git);
> +	add_to_alternates_file(sb.buf);
>  
> -	strbuf_addf(&alternate, "%s/objects", ref_git);
> -	add_to_alternates_file(alternate.buf);
> -	strbuf_release(&alternate);
> -	free(ref_git);
> +	strbuf_release(&sb);

I am wondering about the loss of free() here in the first comment.

> +/*
> + * Compute the exact path an alternate is at and returns it. In case of
> + * error NULL is returned and the human readable error is added to `err`
> + * `path` may be relative and should point to $GITDIR.
> + * `err` must not be null.
> + */
> +char *compute_alternate_path(const char *path, struct strbuf *err)
> +{
> +	char *ref_git = NULL;
> +	const char *repo, *ref_git_s;
> +	struct strbuf err_buf = STRBUF_INIT;

Why do you need "err_buf", instead of directly writing the error to
"err", especially if "err" is not optional?

> + ...
> +out:
> +	if (err_buf.len) {
> +		strbuf_addbuf(err, &err_buf);
> +		free(ref_git);
> +		ref_git = NULL;
> +	}
> +
> +	strbuf_release(&err_buf);
> +	return ref_git;
> +}

So ref_git is a piece of memory on heap, and the caller is
responsible for not leaking it.