Re: [PATCH 02/12] remote.c: drop "remote" pointer from "struct branch"
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:04:37
On Fri, May 1, 2015 at 6:45 PM, Jeff King [off-list ref] wrote:
When we create each branch struct, we fill in the
"remote_name" field from the config, and then fill in the
actual "remote" field (with a "struct remote") based on that
name. However, it turns out that nobody really cares about
the latter field. The only two sites that access it at all
are:
1. git-merge, which uses it to notice when the branch does
not have a remote defined. But we can easily replace this
with looking at remote_name instead.
2. remote.c itself, when setting up the @{upstream} merge
config. But we don't need to save the "remote" in the
"struct branch" for that; we can just look it up for
the duration of the operation.
So there is no need to have both fields; they are redundant
with each other (the struct remote contains the name, or you
can look up the struct from the name). It would be nice to
simplify this, especially as we are going to add matching
pushremote config in a future patch (and it would be nice to
keep them consistent).
[...]Nice clear explanation, but...
quoted hunk ↗ jump to hunk
Signed-off-by: Jeff King <redacted> ---diff --git a/remote.c b/remote.c index bec8b31..c85ecef 100644 --- a/remote.c +++ b/remote.c@@ -1632,15 +1632,20 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, static void set_merge(struct branch *ret) { + struct remote *remote; char *ref; unsigned char sha1[20]; int i; + if (!ret->remote_name || !ret->merge_nr) + return; + remote = remote_get(ret->remote_name); + ret->merge = xcalloc(ret->merge_nr, sizeof(*ret->merge)); for (i = 0; i < ret->merge_nr; i++) { ret->merge[i] = xcalloc(1, sizeof(**ret->merge)); ret->merge[i]->src = xstrdup(ret->merge_name[i]); - if (!remote_find_tracking(ret->remote, ret->merge[i]) || + if (!remote_find_tracking(remote, ret->merge[i]) || strcmp(ret->remote_name, ".")) continue; if (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]),@@ -1660,11 +1665,8 @@ struct branch *branch_get(const char *name) ret = current_branch; else ret = make_branch(name, 0); - 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?
return ret; }