Re: [PATCHv4 5/8] clone: factor out checking for an alternate path
- From
Stefan Beller <sbeller@google.com>
- Date
- Aug 15, 2016, 19:03 UTC
- Message-ID
- <CAGZ79kYVh7N-H7iO4E7HydeOzdUfxGTc3sUo8RhCzR33u9gRHw@mail.gmail.com>
- In-Reply-To
- <xmqqa8ghvd3j.fsf@gitster.mtv.corp.google.com>
On Fri, Aug 12, 2016 at 3:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> 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?
The caller of compute_alternate_path(..), which makes add_one_reference faulty as of this patch.
Show 12 quoted lines
>
>> - 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).Ok. I had an intermediate version with 2 strbufs but for some reason I decided one is better. We'll have 2 again. (err and sb; sb will have a smaller scope only in the else part.)
Show 13 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.fixed in a reroll.
Show 19 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?
>
>> + ...
>> +out:
>> + if (err_buf.len) {If we were directly writing to err, we would have checked err.len here. Then you open up a subtle way of saying "dry run" by giving a non empty error buffer.
I contemplated doing that actually instead of splitting up into 2 functions, but I considered that bad taste as it would require documentation.
Show 11 quoted lines
>> + 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.
Correct.