Re: [PATCHv4 5/8] clone: factor out checking for an alternate path
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 12, 2016, 22:25 UTC
- Message-ID
- <xmqqa8ghvd3j.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160811231405.17318-6-sbeller@google.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).
Show 10 quoted lines
> - 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.
Show 11 quoted lines
> +/*
> + * 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?
Show 11 quoted lines
> + ...
> +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.