From: Karthik Nayak <karthik.188@gmail.com>
Date: Wed, 16 Sep 2015 06:04:08 GMT
Subject: Re: [PATCH v4 5/8] branch: drop non-commit error reporting
Message-ID: <CAOLa=ZQ+uAy6CfWqRgV_Wf8iQoyHdtYU=HEn748Vb=FRc36f_g@mail.gmail.com>
In-Reply-To: <xmqqvbbcvl0v.fsf@gitster.mtv.corp.google.com>

On Tue, Sep 15, 2015 at 1:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Remove the error reporting variable to make the code easier to port
>> over to using ref-filter APIs. This variable is not required as in
>> ref-filter we already check for possible errors and report them.
>
> Hmmmm.  What do you exactly mean "possible errors" here?
>

The checking if it points to a commit.

> Unlike generic refs that can point at anything, refs/heads/* refs
> must point at a commit [*1*], and that is why the error message says
> 'does not point at a commit'.
>
> Does ref-filter API have corresponding check to treat the local and
> remote branch hierarchies differently from tags and others?
>

This check in the existing code is only done when one of these holds:
ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER

There is a similar check in ref-filter when we filter and obtain refs.

    if (filter->merge_commit || filter->with_commit || filter->verbose) {
        commit = lookup_commit_reference_gently(oid->hash, 1);
        if (!commit)
            return 0;
        /* We perform the filtering for the '--contains' option */
        if (filter->with_commit &&
            !commit_contains(filter, commit))
            return 0;
    }

> [Footnote]
>
> *1* Strictly speaking, use of lookup_commit_reference_gently() in
>     the existing code allows a committish to be there and does not
>     limit the tip objects to be commits, but I think it is a bug.
>     At least, even with deref_tag(), we reject non committish found
>     at the tip of branch refs and explain with the error message
>     this patch removes.

Ah, didn't go through this.

-- 
Regards,
Karthik Nayak