Re: fetch_refs_via_pack() discards status?

From: André Goddard Rosa <andre.goddard@gmail.com>
Date: Dec 6, 2007, 00:38 UTC
Message-ID: <b8bf37780712051638x5a2ea5d4i61631f31af65cb54@mail.gmail.com>
In-Reply-To: <7vwsrsonqm.fsf@gitster.siamese.dyndns.org>

On Dec 5, 2007 7:06 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

> Daniel Barkalow <barkalow@iabervon.org> writes:
>
> > On Tue, 4 Dec 2007, Junio C Hamano wrote:
> >
> >> The code calls fetch_pack() to get the list of refs it fetched, and
> >> discards refs and always returns 0 to signal success.
> >>
> >> But builtin-fetch-pack.c::fetch_pack() has error cases.  The function
> >> returns NULL if error is detected (shallow-support side seems to choose
> >> to die but I suspect that is easily fixable to error out as well).
> >>
> >> Shouldn't fetch_refs_via_pack() propagate that error to the caller?
> >
> > I think that's right. I think I got as far as having the error status from
> > fetch_pack() actually returned correctly, and then failed to look at it.
> > I'd personally avoid testing a pointer to freed memory, but that's
> > obviously not actually wrong.
> >
> >       -Daniel
>
> Hmph, is that an Ack that the patchlet is actually a bugfix?
>

Hi, Mr. Junio!

     My 2 cents: I think he means that we should not test a freed pointer:

       free_refs(refs);
       free(dest);                     <===
-       return 0;
+       return (refs ? 0 : -1);     <===

Best regards,

-- 
[]s,
André Goddard