Stefan Beller <sbeller@xxxxxxxxxx> writes:
> On Fri, Aug 12, 2016 at 3:25 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Stefan Beller <sbeller@xxxxxxxxxx> 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.
>
>>
>>> - 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.)
My "unfortunate" was meant to say "it is unfortunate that this is
the best, adding one extra variable is not worth the cost".
>> 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.
Sorry, but I do not understand this response at all. I am still
talking about keeping one function "compute_alternate_path()". The
suggestion was just to drop "struct strbuf err_buf" local variable,
and instead add the error messages the patch adds to err_buf with
code like:
> + if (!ref_git_s) {
> + strbuf_addf(&err_buf, _("path '%s' does not exist"), path);
> + goto out;
to directly do
strbuf_addf(err, _("path '%s' does not exist"), path);
instead. That way you do not have to move the bytes around from one
buffer to the other, like this:
>>> + strbuf_addbuf(err, &err_buf);
>>> + free(ref_git);
>>> + ref_git = NULL;
>>> + }
If you allow err->len to be non-zero upon entry, you'd need a way to
remember if you noticed an error, so that you can free and clear
ref_git after "out:" label, and doing so with a separate variable
would make the code more readable, I would think, either by (1)
recording the original err->len upon entry, and comparing it with
err->len at "out:" label, or (2) using a new bool "seen_error = 0"
and setting it to true when you diagnose an error. The latter would
make the code a bit more verbose but I suspect the result would be
easier to read than the former.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html