On Mon, Aug 15, 2016 at 1:36 PM, Junio C Hamano <gits...@pobox.com> wrote:
>>> 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.

Me neither. I think I elided the interesting part.

> 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);


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

(2) is very readable, will reroll with that.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to