On Sat, May 02, 2015 at 11:34:25PM -0400, Eric Sunshine wrote:
> > - 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html