Re: [PATCH 02/12] remote.c: drop "remote" pointer from "struct branch"

From: Jeff King <peff@peff.net>
Date: May 5, 2015, 19:31 UTC
Message-ID: <20150505193105.GE10463@peff.net>
In-Reply-To: <CAPig+cQny16Jei9AtDWVY3ADbPGshFw7CYofD_TnyA+GL58AOg@mail.gmail.com>

On Sat, May 02, 2015 at 11:34:25PM -0400, Eric Sunshine wrote:

Show 15 quoted lines

> > -       if (ret && ret->remote_name) {
> > -               ret->remote = remote_get(ret->remote_name);
> > -               if (ret->merge_nr)
> > -                       set_merge(ret);
> > -       }
> > +       if (ret)
> > +              set_merge(ret);
> 
> When reading the actual patch, I was surprised to see unmentioned
> changes to the reg->merge_nr check. Although the merge_nr
> simplification seems sensible, it appears to be unrelated to the
> stated purpose of the patch, and made the review more difficult since
> it required keeping track of two distinct (yet textually intertwined)
> changes. I wonder if it would make more sense to apply the merge_nr
> simplification as a separate preparatory patch?

I didn't actually mean to change any behavior with respect to ret->merge_nr here (and I don't think I did). What I did was blindly move everything in the conditional after the remote_get into set_merge, so that it happened in the same order (and the remote_get moves into set_merge, because we no longer have the struct element that it was formerly passed down in).

But actually, ret->merge_nr comes from make_branch, and we could continue to respect it regardless of the remote values (i.e., they are both preconditions to setting up the merge data, but it doesn't matter in which order we check them).

One thing I did notice while looking at this is that it seems like we may leak if you call branch_get multiple times. The make_branch() function may sometimes return a brand-new branch and sometimes return a cached version from the "branches" array. In the latter case, we continue to update the "remote" pointer (which is wasteful but at least does not leak because the remotes themselves are part of a cached list). But then we will repeatedly re-allocate the ret->merge array. We probably should make sure it is NULL before trying to fill it in.

I'll see if I can insert a cleanup patch in this part of the series.

-Peff