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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 15, 2016, 20:36 UTC
Message-ID: <xmqq1t1phir4.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kYVh7N-H7iO4E7HydeOzdUfxGTc3sUo8RhCzR33u9gRHw@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 27 quoted lines

> On Fri, Aug 12, 2016 at 3:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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.
>
>>
>>> -     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".

Show 13 quoted lines

>> 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.