From e41bf352e3280e6990605a18ebbbd40c6f1c0d6d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 1 May 2015 18:44:41 -0400 Subject: remote.c: drop default_remote_name variable When we read the remote config from disk, we update a default_remote_name variable if we see branch.*.remote config for the current branch. This isn't wrong, or even all that complicated, but it is a bit simpler (because it reduces our overall state) to just lazily compute the default when we need it. The ulterior motive here is that the push config uses a similar structure, and _is_ much more complicated as a result. That will be simplified in a future patch, and it's more readable if the logic for remotes and push-remotes matches. Note that we also used default_remote_name as a signal that the remote config has been loaded; after this patch, we now use an explicit flag. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) (limited to 'remote.c') diff --git a/remote.c b/remote.c index 68901b0070..bec8b311b0 100644 --- a/remote.c +++ b/remote.c @@ -49,10 +49,8 @@ static int branches_alloc; static int branches_nr; static struct branch *current_branch; -static const char *default_remote_name; static const char *branch_pushremote_name; static const char *pushremote_name; -static int explicit_default_remote_name; static struct rewrites rewrites; static struct rewrites rewrites_push; @@ -367,12 +365,7 @@ static int handle_config(const char *key, const char *value, void *cb) return 0; branch = make_branch(name, subkey - name); if (!strcmp(subkey, ".remote")) { - if (git_config_string(&branch->remote_name, key, value)) - return -1; - if (branch == current_branch) { - default_remote_name = branch->remote_name; - explicit_default_remote_name = 1; - } + return git_config_string(&branch->remote_name, key, value); } else if (!strcmp(subkey, ".pushremote")) { if (branch == current_branch) if (git_config_string(&branch_pushremote_name, key, value)) @@ -501,12 +494,15 @@ static void alias_all_urls(void) static void read_config(void) { + static int loaded; unsigned char sha1[20]; const char *head_ref; int flag; - if (default_remote_name) /* did this already */ + + if (loaded) return; - default_remote_name = "origin"; + loaded = 1; + current_branch = NULL; head_ref = resolve_ref_unsafe("HEAD", 0, sha1, &flag); if (head_ref && (flag & REF_ISSYMREF) && @@ -708,8 +704,11 @@ static struct remote *remote_get_1(const char *name, const char *pushremote_name name = pushremote_name; name_given = 1; } else { - name = default_remote_name; - name_given = explicit_default_remote_name; + if (current_branch && current_branch->remote_name) { + name = current_branch->remote_name; + name_given = 1; + } else + name = "origin"; } } -- cgit v1.3-5-g9baa From ee2499fe38d93fcbf4cbecdf83b32a495b0b12b9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 21 May 2015 00:45:09 -0400 Subject: remote.c: refactor setup of branch->merge list When we call branch_get() to lookup or create a "struct branch", we make sure the "merge" field is filled in so that callers can access it. But the conditions under which we do so are a little confusing, and can lead to two funny situations: 1. If there's no branch.*.remote config, we cannot provide branch->merge (because it is really just an application of branch.*.merge to our remote's refspecs). But branch->merge_nr may be non-zero, leading callers to be believe they can access branch->merge (e.g., in branch_merge_matches and elsewhere). It doesn't look like this can cause a segfault in practice, as most code paths dealing with merge config will bail early if there is no remote defined. But it's a bit of a dangerous construct. We can fix this by setting merge_nr to "0" explicitly when we realize that we have no merge config. Note that merge_nr also counts the "merge_name" fields (which we _do_ have; that's how merge_nr got incremented), so we will "lose" access to them, in the sense that we forget how many we had. But no callers actually care; we use merge_name only while iteratively reading the config, and then convert it to the final "merge" form the first time somebody calls branch_get(). 2. We set up the "merge" field every time branch_get is called, even if it has already been done. This leaks memory. It's not a big deal in practice, since most code paths will access only one branch, or perhaps each branch only one time. But if you want to be pathological, you can leak arbitrary memory with: yes @{upstream} | head -1000 | git rev-list --stdin We can fix this by skipping setup when branch->merge is already non-NULL. In addition to those two fixes, this patch pushes the "do we need to setup merge?" logic down into set_merge, where it is a bit easier to follow. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) (limited to 'remote.c') diff --git a/remote.c b/remote.c index bec8b311b0..ac17e66c09 100644 --- a/remote.c +++ b/remote.c @@ -1636,6 +1636,19 @@ static void set_merge(struct branch *ret) unsigned char sha1[20]; int i; + if (!ret) + return; /* no branch */ + if (ret->merge) + return; /* already run */ + if (!ret->remote_name || !ret->merge_nr) { + /* + * no merge config; let's make sure we don't confuse callers + * with a non-zero merge_nr but a NULL merge + */ + ret->merge_nr = 0; + return; + } + 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)); @@ -1660,11 +1673,9 @@ struct branch *branch_get(const char *name) ret = current_branch; else ret = make_branch(name, 0); - if (ret && ret->remote_name) { + if (ret && ret->remote_name) ret->remote = remote_get(ret->remote_name); - if (ret->merge_nr) - set_merge(ret); - } + set_merge(ret); return ret; } -- cgit v1.3-5-g9baa From 9e3751d4437b43e72497178774c74be1ceac28b9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 21 May 2015 00:45:13 -0400 Subject: remote.c: drop "remote" pointer from "struct branch" 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). So which one do we keep and which one do we get rid of? If we had a lot of callers accessing the struct, it would be more efficient to keep it (since you have to do a lookup to go from the name to the struct, but not vice versa). But we don't have a lot of callers; we have exactly one, so efficiency doesn't matter. We can decide this based on simplicity and readability. And the meaning of the struct value is somewhat unclear. Is it always the remote matching remote_name? If remote_name is NULL (i.e., no per-branch config), does the struct fall back to the "origin" remote, or is it also NULL? These questions will get even more tricky with pushremotes, whose fallback behavior is more complicated. So let's just store the name, which pretty clearly represents the branch.*.remote config. Any lookup or fallback behavior can then be implemented in helper functions. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/technical/api-remote.txt | 4 ---- builtin/merge.c | 2 +- remote.c | 7 ++++--- remote.h | 1 - 4 files changed, 5 insertions(+), 9 deletions(-) (limited to 'remote.c') diff --git a/Documentation/technical/api-remote.txt b/Documentation/technical/api-remote.txt index 5d245aa9d1..2cfdd224a8 100644 --- a/Documentation/technical/api-remote.txt +++ b/Documentation/technical/api-remote.txt @@ -97,10 +97,6 @@ It contains: The name of the remote listed in the configuration. -`remote`:: - - The struct remote for that remote. - `merge_name`:: An array of the "merge" lines in the configuration. diff --git a/builtin/merge.c b/builtin/merge.c index 3b0f8f96d4..1840317118 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -955,7 +955,7 @@ static int setup_with_upstream(const char ***argv) if (!branch) die(_("No current branch.")); - if (!branch->remote) + if (!branch->remote_name) die(_("No remote for the current branch.")); if (!branch->merge_nr) die(_("No default upstream defined for the current branch.")); diff --git a/remote.c b/remote.c index ac17e66c09..c298a43a1c 100644 --- a/remote.c +++ b/remote.c @@ -1632,6 +1632,7 @@ 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; @@ -1649,11 +1650,13 @@ static void set_merge(struct branch *ret) 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]), @@ -1673,8 +1676,6 @@ 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); set_merge(ret); return ret; } diff --git a/remote.h b/remote.h index 02d66ceff5..4bb6672735 100644 --- a/remote.h +++ b/remote.h @@ -203,7 +203,6 @@ struct branch { const char *refname; const char *remote_name; - struct remote *remote; const char **merge_name; struct refspec **merge; -- cgit v1.3-5-g9baa From f052154db332e48ea35b1a0d783361a40a361250 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 21 May 2015 00:45:16 -0400 Subject: remote.c: hoist branch.*.remote lookup out of remote_get_1 We'll want to use this logic as a fallback when looking up the pushremote, so let's pull it out into its own function. We don't technically need to make this available outside of remote.c, but doing so will provide a consistent API with pushremote_for_branch, which we will add later. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote.c | 21 ++++++++++++++------- remote.h | 1 + 2 files changed, 15 insertions(+), 7 deletions(-) (limited to 'remote.c') diff --git a/remote.c b/remote.c index c298a43a1c..0d2976b36b 100644 --- a/remote.c +++ b/remote.c @@ -692,6 +692,18 @@ static int valid_remote_nick(const char *name) return !strchr(name, '/'); /* no slash */ } +const char *remote_for_branch(struct branch *branch, int *explicit) +{ + if (branch && branch->remote_name) { + if (explicit) + *explicit = 1; + return branch->remote_name; + } + if (explicit) + *explicit = 0; + return "origin"; +} + static struct remote *remote_get_1(const char *name, const char *pushremote_name) { struct remote *ret; @@ -703,13 +715,8 @@ static struct remote *remote_get_1(const char *name, const char *pushremote_name if (pushremote_name) { name = pushremote_name; name_given = 1; - } else { - if (current_branch && current_branch->remote_name) { - name = current_branch->remote_name; - name_given = 1; - } else - name = "origin"; - } + } else + name = remote_for_branch(current_branch, &name_given); } ret = make_remote(name, 0); diff --git a/remote.h b/remote.h index 4bb6672735..2a7e7a698b 100644 --- a/remote.h +++ b/remote.h @@ -211,6 +211,7 @@ struct branch { }; struct branch *branch_get(const char *name); +const char *remote_for_branch(struct branch *branch, int *explicit); int branch_has_merge_config(struct branch *branch); int branch_merge_matches(struct branch *, int n, const char *); -- cgit v1.3-5-g9baa From da66b2743cf7244e52c4b9d91646b782cd4f7eeb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 21 May 2015 00:45:20 -0400 Subject: remote.c: provide per-branch pushremote name When remote.c loads its config, it records the branch.*.pushremote for the current branch along with the global remote.pushDefault value, and then binds them into a single value: the default push for the current branch. We then pass this value (which may be NULL) to remote_get_1 when looking up a remote for push. This has a few downsides: 1. It's confusing. The early-binding of the "current value" led to bugs like the one fixed by 98b406f (remote: handle pushremote config in any order, 2014-02-24). And the fact that pushremotes fall back to ordinary remotes is not explicit at all; it happens because remote_get_1 cannot tell the difference between "we are not asking for the push remote" and "there is no push remote configured". 2. It throws away intermediate data. After read_config() finishes, we have no idea what the value of remote.pushDefault was, because the string has been overwritten by the current branch's branch.*.pushremote. 3. It doesn't record other data. We don't note the branch.*.pushremote value for anything but the current branch. Let's make this more like the fetch-remote config. We'll record the pushremote for each branch, and then explicitly compute the correct remote for the current branch at the time of reading. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote.c | 40 ++++++++++++++++++++++------------------ remote.h | 2 ++ 2 files changed, 24 insertions(+), 18 deletions(-) (limited to 'remote.c') diff --git a/remote.c b/remote.c index 0d2976b36b..a91d06396e 100644 --- a/remote.c +++ b/remote.c @@ -49,7 +49,6 @@ static int branches_alloc; static int branches_nr; static struct branch *current_branch; -static const char *branch_pushremote_name; static const char *pushremote_name; static struct rewrites rewrites; @@ -367,9 +366,7 @@ static int handle_config(const char *key, const char *value, void *cb) if (!strcmp(subkey, ".remote")) { return git_config_string(&branch->remote_name, key, value); } else if (!strcmp(subkey, ".pushremote")) { - if (branch == current_branch) - if (git_config_string(&branch_pushremote_name, key, value)) - return -1; + return git_config_string(&branch->pushremote_name, key, value); } else if (!strcmp(subkey, ".merge")) { if (!value) return config_error_nonbool(key); @@ -510,10 +507,6 @@ static void read_config(void) current_branch = make_branch(head_ref, 0); } git_config(handle_config, NULL); - if (branch_pushremote_name) { - free((char *)pushremote_name); - pushremote_name = branch_pushremote_name; - } alias_all_urls(); } @@ -704,20 +697,31 @@ const char *remote_for_branch(struct branch *branch, int *explicit) return "origin"; } -static struct remote *remote_get_1(const char *name, const char *pushremote_name) +const char *pushremote_for_branch(struct branch *branch, int *explicit) +{ + if (branch && branch->pushremote_name) { + if (explicit) + *explicit = 1; + return branch->pushremote_name; + } + if (pushremote_name) { + if (explicit) + *explicit = 1; + return pushremote_name; + } + return remote_for_branch(branch, explicit); +} + +static struct remote *remote_get_1(const char *name, + const char *(*get_default)(struct branch *, int *)) { struct remote *ret; int name_given = 0; if (name) name_given = 1; - else { - if (pushremote_name) { - name = pushremote_name; - name_given = 1; - } else - name = remote_for_branch(current_branch, &name_given); - } + else + name = get_default(current_branch, &name_given); ret = make_remote(name, 0); if (valid_remote_nick(name)) { @@ -738,13 +742,13 @@ static struct remote *remote_get_1(const char *name, const char *pushremote_name struct remote *remote_get(const char *name) { read_config(); - return remote_get_1(name, NULL); + return remote_get_1(name, remote_for_branch); } struct remote *pushremote_get(const char *name) { read_config(); - return remote_get_1(name, pushremote_name); + return remote_get_1(name, pushremote_for_branch); } int remote_is_configured(const char *name) diff --git a/remote.h b/remote.h index 2a7e7a698b..30a11dac10 100644 --- a/remote.h +++ b/remote.h @@ -203,6 +203,7 @@ struct branch { const char *refname; const char *remote_name; + const char *pushremote_name; const char **merge_name; struct refspec **merge; @@ -212,6 +213,7 @@ struct branch { struct branch *branch_get(const char *name); const char *remote_for_branch(struct branch *branch, int *explicit); +const char *pushremote_for_branch(struct branch *branch, int *explicit); int branch_has_merge_config(struct branch *branch); int branch_merge_matches(struct branch *, int n, const char *); -- cgit v1.3-5-g9baa From 8770e6fbb28dffdf9e00d05365120e438d3d236f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 21 May 2015 00:45:23 -0400 Subject: remote.c: hoist read_config into remote_get_1 Before the previous commit, we had to make sure that read_config() was called before entering remote_get_1, because we needed to pass pushremote_name by value. But now that we pass a function, we can let remote_get_1 handle loading the config itself, turning our wrappers into true one-liners. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'remote.c') diff --git a/remote.c b/remote.c index a91d06396e..e6b29b34b6 100644 --- a/remote.c +++ b/remote.c @@ -718,6 +718,8 @@ static struct remote *remote_get_1(const char *name, struct remote *ret; int name_given = 0; + read_config(); + if (name) name_given = 1; else @@ -741,13 +743,11 @@ static struct remote *remote_get_1(const char *name, struct remote *remote_get(const char *name) { - read_config(); return remote_get_1(name, remote_for_branch); } struct remote *pushremote_get(const char *name) { - read_config(); return remote_get_1(name, pushremote_for_branch); } -- cgit v1.3-5-g9baa From a9f9f8cc1f59104257eb1a11a2d048f54dd92ee6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 21 May 2015 00:45:28 -0400 Subject: remote.c: introduce branch_get_upstream helper All of the information needed to find the @{upstream} of a branch is included in the branch struct, but callers have to navigate a series of possible-NULL values to get there. Let's wrap that logic up in an easy-to-read helper. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/branch.c | 8 +++----- builtin/for-each-ref.c | 5 ++--- builtin/log.c | 7 ++----- remote.c | 12 +++++++++--- remote.h | 7 +++++++ 5 files changed, 23 insertions(+), 16 deletions(-) (limited to 'remote.c') diff --git a/builtin/branch.c b/builtin/branch.c index 1d150378e9..bd1fa0b43a 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -123,14 +123,12 @@ static int branch_merged(int kind, const char *name, if (kind == REF_LOCAL_BRANCH) { struct branch *branch = branch_get(name); + const char *upstream = branch_get_upstream(branch); unsigned char sha1[20]; - if (branch && - branch->merge && - branch->merge[0] && - branch->merge[0]->dst && + if (upstream && (reference_name = reference_name_to_free = - resolve_refdup(branch->merge[0]->dst, RESOLVE_REF_READING, + resolve_refdup(upstream, RESOLVE_REF_READING, sha1, NULL)) != NULL) reference_rev = lookup_commit_reference(sha1); } diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 83f9cf9163..dc2a201a45 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -664,10 +664,9 @@ static void populate_value(struct refinfo *ref) continue; branch = branch_get(ref->refname + 11); - if (!branch || !branch->merge || !branch->merge[0] || - !branch->merge[0]->dst) + refname = branch_get_upstream(branch); + if (!refname) continue; - refname = branch->merge[0]->dst; } else if (starts_with(name, "color:")) { char color[COLOR_MAXLEN] = ""; diff --git a/builtin/log.c b/builtin/log.c index dd8f3fcfc4..fb61c08ee5 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1632,16 +1632,13 @@ int cmd_cherry(int argc, const char **argv, const char *prefix) break; default: current_branch = branch_get(NULL); - if (!current_branch || !current_branch->merge - || !current_branch->merge[0] - || !current_branch->merge[0]->dst) { + upstream = branch_get_upstream(current_branch); + if (!upstream) { fprintf(stderr, _("Could not find a tracked" " remote branch, please" " specify manually.\n")); usage_with_options(cherry_usage, options); } - - upstream = current_branch->merge[0]->dst; } init_revisions(&revs, prefix); diff --git a/remote.c b/remote.c index e6b29b34b6..dca3442aba 100644 --- a/remote.c +++ b/remote.c @@ -1705,6 +1705,13 @@ int branch_merge_matches(struct branch *branch, return refname_match(branch->merge[i]->src, refname); } +const char *branch_get_upstream(struct branch *branch) +{ + if (!branch || !branch->merge || !branch->merge[0]) + return NULL; + return branch->merge[0]->dst; +} + static int ignore_symref_update(const char *refname) { unsigned char sha1[20]; @@ -1914,12 +1921,11 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs) int rev_argc; /* Cannot stat unless we are marked to build on top of somebody else. */ - if (!branch || - !branch->merge || !branch->merge[0] || !branch->merge[0]->dst) + base = branch_get_upstream(branch); + if (!base) return 0; /* Cannot stat if what we used to build on no longer exists */ - base = branch->merge[0]->dst; if (read_ref(base, sha1)) return -1; theirs = lookup_commit_reference(sha1); diff --git a/remote.h b/remote.h index 30a11dac10..d96895282e 100644 --- a/remote.h +++ b/remote.h @@ -218,6 +218,13 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit); int branch_has_merge_config(struct branch *branch); int branch_merge_matches(struct branch *, int n, const char *); +/** + * Return the fully-qualified refname of the tracking branch for `branch`. + * I.e., what "branch@{upstream}" would give you. Returns NULL if no + * upstream is defined. + */ +const char *branch_get_upstream(struct branch *branch); + /* Flags to match_refs. */ enum match_refs_flags { MATCH_REFS_NONE = 0, -- cgit v1.3-5-g9baa From 3a429d0af342d85ef6d561e3a60ae8793a34ae78 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 21 May 2015 00:45:32 -0400 Subject: remote.c: report specific errors from branch_get_upstream When the previous commit introduced the branch_get_upstream helper, there was one call-site that could not be converted: the one in sha1_name.c, which gives detailed error messages for each possible failure. Let's teach the helper to optionally report these specific errors. This lets us convert another callsite, and means we can use the helper in other locations that want to give the same error messages. The logic and error messages come straight from sha1_name.c, with the exception that we start each error with a lowercase letter, as is our usual style (note that a few tests need updated as a result). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/branch.c | 2 +- builtin/for-each-ref.c | 2 +- builtin/log.c | 2 +- remote.c | 33 +++++++++++++++++++++++++++++---- remote.h | 6 +++++- sha1_name.c | 25 +++++++------------------ t/t1507-rev-parse-upstream.sh | 8 ++++---- 7 files changed, 48 insertions(+), 30 deletions(-) (limited to 'remote.c') diff --git a/builtin/branch.c b/builtin/branch.c index bd1fa0b43a..b7202b3399 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -123,7 +123,7 @@ static int branch_merged(int kind, const char *name, if (kind == REF_LOCAL_BRANCH) { struct branch *branch = branch_get(name); - const char *upstream = branch_get_upstream(branch); + const char *upstream = branch_get_upstream(branch, NULL); unsigned char sha1[20]; if (upstream && diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index dc2a201a45..18d209bc9a 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -664,7 +664,7 @@ static void populate_value(struct refinfo *ref) continue; branch = branch_get(ref->refname + 11); - refname = branch_get_upstream(branch); + refname = branch_get_upstream(branch, NULL); if (!refname) continue; } else if (starts_with(name, "color:")) { diff --git a/builtin/log.c b/builtin/log.c index fb61c08ee5..6faeb82c8e 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1632,7 +1632,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix) break; default: current_branch = branch_get(NULL); - upstream = branch_get_upstream(current_branch); + upstream = branch_get_upstream(current_branch, NULL); if (!upstream) { fprintf(stderr, _("Could not find a tracked" " remote branch, please" diff --git a/remote.c b/remote.c index dca3442aba..1b7051a187 100644 --- a/remote.c +++ b/remote.c @@ -1705,10 +1705,35 @@ int branch_merge_matches(struct branch *branch, return refname_match(branch->merge[i]->src, refname); } -const char *branch_get_upstream(struct branch *branch) +__attribute((format (printf,2,3))) +static const char *error_buf(struct strbuf *err, const char *fmt, ...) { - if (!branch || !branch->merge || !branch->merge[0]) - return NULL; + if (err) { + va_list ap; + va_start(ap, fmt); + strbuf_vaddf(err, fmt, ap); + va_end(ap); + } + return NULL; +} + +const char *branch_get_upstream(struct branch *branch, struct strbuf *err) +{ + if (!branch) + return error_buf(err, _("HEAD does not point to a branch")); + if (!branch->merge || !branch->merge[0] || !branch->merge[0]->dst) { + if (!ref_exists(branch->refname)) + return error_buf(err, _("no such branch: '%s'"), + branch->name); + if (!branch->merge) + return error_buf(err, + _("no upstream configured for branch '%s'"), + branch->name); + return error_buf(err, + _("upstream branch '%s' not stored as a remote-tracking branch"), + branch->merge[0]->src); + } + return branch->merge[0]->dst; } @@ -1921,7 +1946,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs) int rev_argc; /* Cannot stat unless we are marked to build on top of somebody else. */ - base = branch_get_upstream(branch); + base = branch_get_upstream(branch, NULL); if (!base) return 0; diff --git a/remote.h b/remote.h index d96895282e..03ca0058fe 100644 --- a/remote.h +++ b/remote.h @@ -222,8 +222,12 @@ int branch_merge_matches(struct branch *, int n, const char *); * Return the fully-qualified refname of the tracking branch for `branch`. * I.e., what "branch@{upstream}" would give you. Returns NULL if no * upstream is defined. + * + * If `err` is not NULL and no upstream is defined, a more specific error + * message is recorded there (if the function does not return NULL, then + * `err` is not touched). */ -const char *branch_get_upstream(struct branch *branch); +const char *branch_get_upstream(struct branch *branch, struct strbuf *err); /* Flags to match_refs. */ enum match_refs_flags { diff --git a/sha1_name.c b/sha1_name.c index 6d10f052b5..461157a5bc 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -1059,27 +1059,16 @@ static const char *get_upstream_branch(const char *branch_buf, int len) { char *branch = xstrndup(branch_buf, len); struct branch *upstream = branch_get(*branch ? branch : NULL); + struct strbuf err = STRBUF_INIT; + const char *ret; - /* - * Upstream can be NULL only if branch refers to HEAD and HEAD - * points to something different than a branch. - */ - if (!upstream) - die(_("HEAD does not point to a branch")); - if (!upstream->merge || !upstream->merge[0]->dst) { - if (!ref_exists(upstream->refname)) - die(_("No such branch: '%s'"), branch); - if (!upstream->merge) { - die(_("No upstream configured for branch '%s'"), - upstream->name); - } - die( - _("Upstream branch '%s' not stored as a remote-tracking branch"), - upstream->merge[0]->src); - } free(branch); - return upstream->merge[0]->dst; + ret = branch_get_upstream(upstream, &err); + if (!ret) + die("%s", err.buf); + + return ret; } static int interpret_upstream_mark(const char *name, int namelen, diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh index 1978947c41..46ef1f22dc 100755 --- a/t/t1507-rev-parse-upstream.sh +++ b/t/t1507-rev-parse-upstream.sh @@ -150,7 +150,7 @@ test_expect_success 'branch@{u} works when tracking a local branch' ' test_expect_success 'branch@{u} error message when no upstream' ' cat >expect <<-EOF && - fatal: No upstream configured for branch ${sq}non-tracking${sq} + fatal: no upstream configured for branch ${sq}non-tracking${sq} EOF error_message non-tracking@{u} 2>actual && test_i18ncmp expect actual @@ -158,7 +158,7 @@ test_expect_success 'branch@{u} error message when no upstream' ' test_expect_success '@{u} error message when no upstream' ' cat >expect <<-EOF && - fatal: No upstream configured for branch ${sq}master${sq} + fatal: no upstream configured for branch ${sq}master${sq} EOF test_must_fail git rev-parse --verify @{u} 2>actual && test_i18ncmp expect actual @@ -166,7 +166,7 @@ test_expect_success '@{u} error message when no upstream' ' test_expect_success 'branch@{u} error message with misspelt branch' ' cat >expect <<-EOF && - fatal: No such branch: ${sq}no-such-branch${sq} + fatal: no such branch: ${sq}no-such-branch${sq} EOF error_message no-such-branch@{u} 2>actual && test_i18ncmp expect actual @@ -183,7 +183,7 @@ test_expect_success '@{u} error message when not on a branch' ' test_expect_success 'branch@{u} error message if upstream branch not fetched' ' cat >expect <<-EOF && - fatal: Upstream branch ${sq}refs/heads/side${sq} not stored as a remote-tracking branch + fatal: upstream branch ${sq}refs/heads/side${sq} not stored as a remote-tracking branch EOF error_message bad-upstream@{u} 2>actual && test_i18ncmp expect actual -- cgit v1.3-5-g9baa From 1ca41a19323d455cf0028001677f3adfae0d4cc4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 21 May 2015 20:46:43 -0400 Subject: remote.c: untangle error logic in branch_get_upstream The error-diagnosis logic in branch_get_upstream was copied straight from sha1_name.c in the previous commit. However, because we check all error cases and upfront and then later diagnose them, the logic is a bit tangled. In particular: - if branch->merge[0] is NULL, we may end up dereferencing it for an error message (in practice, it should never be NULL, so this is probably not a triggerable bug). - We may enter the code path because branch->merge[0]->dst is NULL, but we then start our error diagnosis by checking whether our local branch exists. But that is only relevant to diagnosing missing merge config, not a missing tracking ref; our diagnosis may hide the real problem. Instead, let's just use a sequence of "if" blocks to check for each error type, diagnose it, and return NULL. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) (limited to 'remote.c') diff --git a/remote.c b/remote.c index 1b7051a187..d2519c22ce 100644 --- a/remote.c +++ b/remote.c @@ -1721,18 +1721,25 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err) { if (!branch) return error_buf(err, _("HEAD does not point to a branch")); - if (!branch->merge || !branch->merge[0] || !branch->merge[0]->dst) { + + if (!branch->merge || !branch->merge[0]) { + /* + * no merge config; is it because the user didn't define any, + * or because it is not a real branch, and get_branch + * auto-vivified it? + */ if (!ref_exists(branch->refname)) return error_buf(err, _("no such branch: '%s'"), branch->name); - if (!branch->merge) - return error_buf(err, - _("no upstream configured for branch '%s'"), - branch->name); + return error_buf(err, + _("no upstream configured for branch '%s'"), + branch->name); + } + + if (!branch->merge[0]->dst) return error_buf(err, _("upstream branch '%s' not stored as a remote-tracking branch"), branch->merge[0]->src); - } return branch->merge[0]->dst; } -- cgit v1.3-5-g9baa From 979cb245e29b35812d2d71a04069ba7a4580981f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 21 May 2015 20:49:11 -0400 Subject: remote.c: return upstream name from stat_tracking_info After calling stat_tracking_info, callers often want to print the name of the upstream branch (in addition to the tracking count). To do this, they have to access branch->merge->dst[0] themselves. This is not wrong, as the return value from stat_tracking_info tells us whether we have an upstream branch or not. But it is a bit leaky, as we make an assumption about how it calculated the upstream name. Instead, let's add an out-parameter that lets the caller know the upstream name we found. As a bonus, we can get rid of the unusual tri-state return from the function. We no longer need to use it to differentiate between "no tracking config" and "tracking ref does not exist" (since you can check the upstream_name for that), so we can just use the usual 0/-1 convention for success/error. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/branch.c | 16 +++++----------- builtin/for-each-ref.c | 4 ++-- remote.c | 35 +++++++++++++++++------------------ remote.h | 3 ++- wt-status.c | 18 ++++++------------ 5 files changed, 32 insertions(+), 44 deletions(-) (limited to 'remote.c') diff --git a/builtin/branch.c b/builtin/branch.c index b7202b3399..e4d184d69a 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -425,25 +425,19 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name, int ours, theirs; char *ref = NULL; struct branch *branch = branch_get(branch_name); + const char *upstream; struct strbuf fancy = STRBUF_INIT; int upstream_is_gone = 0; int added_decoration = 1; - switch (stat_tracking_info(branch, &ours, &theirs)) { - case 0: - /* no base */ - return; - case -1: - /* with "gone" base */ + if (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) { + if (!upstream) + return; upstream_is_gone = 1; - break; - default: - /* with base */ - break; } if (show_upstream_ref) { - ref = shorten_unambiguous_ref(branch->merge[0]->dst, 0); + ref = shorten_unambiguous_ref(upstream, 0); if (want_color(branch_use_color)) strbuf_addf(&fancy, "%s%s%s", branch_get_color(BRANCH_COLOR_UPSTREAM), diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 18d209bc9a..92bd2b2665 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -716,7 +716,7 @@ static void populate_value(struct refinfo *ref) char buf[40]; if (stat_tracking_info(branch, &num_ours, - &num_theirs) != 1) + &num_theirs, NULL)) continue; if (!num_ours && !num_theirs) @@ -738,7 +738,7 @@ static void populate_value(struct refinfo *ref) assert(branch); if (stat_tracking_info(branch, &num_ours, - &num_theirs) != 1) + &num_theirs, NULL)) continue; if (!num_ours && !num_theirs) diff --git a/remote.c b/remote.c index d2519c22ce..c8845746b6 100644 --- a/remote.c +++ b/remote.c @@ -1938,12 +1938,15 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1) /* * Compare a branch with its upstream, and save their differences (number - * of commits) in *num_ours and *num_theirs. + * of commits) in *num_ours and *num_theirs. The name of the upstream branch + * (or NULL if no upstream is defined) is returned via *upstream_name, if it + * is not itself NULL. * - * Return 0 if branch has no upstream (no base), -1 if upstream is missing - * (with "gone" base), otherwise 1 (with base). + * Returns -1 if num_ours and num_theirs could not be filled in (e.g., no + * upstream defined, or ref does not exist), 0 otherwise. */ -int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs) +int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs, + const char **upstream_name) { unsigned char sha1[20]; struct commit *ours, *theirs; @@ -1954,8 +1957,10 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs) /* Cannot stat unless we are marked to build on top of somebody else. */ base = branch_get_upstream(branch, NULL); + if (upstream_name) + *upstream_name = base; if (!base) - return 0; + return -1; /* Cannot stat if what we used to build on no longer exists */ if (read_ref(base, sha1)) @@ -1973,7 +1978,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs) /* are we the same? */ if (theirs == ours) { *num_theirs = *num_ours = 0; - return 1; + return 0; } /* Run "rev-list --left-right ours...theirs" internally... */ @@ -2009,7 +2014,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs) /* clear object flags smudged by the above traversal */ clear_commit_marks(ours, ALL_REV_FLAGS); clear_commit_marks(theirs, ALL_REV_FLAGS); - return 1; + return 0; } /* @@ -2018,23 +2023,17 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs) int format_tracking_info(struct branch *branch, struct strbuf *sb) { int ours, theirs; + const char *full_base; char *base; int upstream_is_gone = 0; - switch (stat_tracking_info(branch, &ours, &theirs)) { - case 0: - /* no base */ - return 0; - case -1: - /* with "gone" base */ + if (stat_tracking_info(branch, &ours, &theirs, &full_base) < 0) { + if (!full_base) + return 0; upstream_is_gone = 1; - break; - default: - /* with base */ - break; } - base = shorten_unambiguous_ref(branch->merge[0]->dst, 0); + base = shorten_unambiguous_ref(full_base, 0); if (upstream_is_gone) { strbuf_addf(sb, _("Your branch is based on '%s', but the upstream is gone.\n"), diff --git a/remote.h b/remote.h index 03ca0058fe..357a90963d 100644 --- a/remote.h +++ b/remote.h @@ -239,7 +239,8 @@ enum match_refs_flags { }; /* Reporting of tracking info */ -int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs); +int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs, + const char **upstream_name); int format_tracking_info(struct branch *branch, struct strbuf *sb); struct ref *get_local_heads(void); diff --git a/wt-status.c b/wt-status.c index 853419f05f..4313b9d845 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1532,21 +1532,15 @@ static void wt_shortstatus_print_tracking(struct wt_status *s) color_fprintf(s->fp, branch_color_local, "%s", branch_name); - switch (stat_tracking_info(branch, &num_ours, &num_theirs)) { - case 0: - /* no base */ - fputc(s->null_termination ? '\0' : '\n', s->fp); - return; - case -1: - /* with "gone" base */ + if (stat_tracking_info(branch, &num_ours, &num_theirs, &base) < 0) { + if (!base) { + fputc(s->null_termination ? '\0' : '\n', s->fp); + return; + } + upstream_is_gone = 1; - break; - default: - /* with base */ - break; } - base = branch->merge[0]->dst; base = shorten_unambiguous_ref(base, 0); color_fprintf(s->fp, header_color, "..."); color_fprintf(s->fp, branch_color_remote, "%s", base); -- cgit v1.3-5-g9baa From e291c75a95d60862cbe12897b5cffb01ba4cedd5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 21 May 2015 00:45:36 -0400 Subject: remote.c: add branch_get_push In a triangular workflow, the place you pull from and the place you push to may be different. As we have branch_get_upstream for the former, this patch adds branch_get_push for the latter (and as the former implements @{upstream}, so will this implement @{push} in a future patch). Note that the memory-handling for the return value bears some explanation. Some code paths require allocating a new string, and some let us return an existing string. We should provide a consistent interface to the caller, so it knows whether to free the result or not. We could do so by xstrdup-ing any existing strings, and having the caller always free. But that makes us inconsistent with branch_get_upstream, so we would prefer to simply take ownership of the resulting string. We do so by storing it inside the "struct branch", just as we do with the upstream refname (in that case we compute it when the branch is created, but there's no reason not to just fill it in lazily in this case). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ remote.h | 10 ++++++++ 2 files changed, 95 insertions(+) (limited to 'remote.c') diff --git a/remote.c b/remote.c index c8845746b6..a467d4ff07 100644 --- a/remote.c +++ b/remote.c @@ -1744,6 +1744,91 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err) return branch->merge[0]->dst; } +static const char *tracking_for_push_dest(struct remote *remote, + const char *refname, + struct strbuf *err) +{ + char *ret; + + ret = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, refname); + if (!ret) + return error_buf(err, + _("push destination '%s' on remote '%s' has no local tracking branch"), + refname, remote->name); + return ret; +} + +static const char *branch_get_push_1(struct branch *branch, struct strbuf *err) +{ + struct remote *remote; + + if (!branch) + return error_buf(err, _("HEAD does not point to a branch")); + + remote = remote_get(pushremote_for_branch(branch, NULL)); + if (!remote) + return error_buf(err, + _("branch '%s' has no remote for pushing"), + branch->name); + + if (remote->push_refspec_nr) { + char *dst; + const char *ret; + + dst = apply_refspecs(remote->push, remote->push_refspec_nr, + branch->refname); + if (!dst) + return error_buf(err, + _("push refspecs for '%s' do not include '%s'"), + remote->name, branch->name); + + ret = tracking_for_push_dest(remote, dst, err); + free(dst); + return ret; + } + + if (remote->mirror) + return tracking_for_push_dest(remote, branch->refname, err); + + switch (push_default) { + case PUSH_DEFAULT_NOTHING: + return error_buf(err, _("push has no destination (push.default is 'nothing')")); + + case PUSH_DEFAULT_MATCHING: + case PUSH_DEFAULT_CURRENT: + return tracking_for_push_dest(remote, branch->refname, err); + + case PUSH_DEFAULT_UPSTREAM: + return branch_get_upstream(branch, err); + + case PUSH_DEFAULT_UNSPECIFIED: + case PUSH_DEFAULT_SIMPLE: + { + const char *up, *cur; + + up = branch_get_upstream(branch, err); + if (!up) + return NULL; + cur = tracking_for_push_dest(remote, branch->refname, err); + if (!cur) + return NULL; + if (strcmp(cur, up)) + return error_buf(err, + _("cannot resolve 'simple' push to a single destination")); + return cur; + } + } + + die("BUG: unhandled push situation"); +} + +const char *branch_get_push(struct branch *branch, struct strbuf *err) +{ + if (!branch->push_tracking_ref) + branch->push_tracking_ref = branch_get_push_1(branch, err); + return branch->push_tracking_ref; +} + static int ignore_symref_update(const char *refname) { unsigned char sha1[20]; diff --git a/remote.h b/remote.h index 357a90963d..312b7ca131 100644 --- a/remote.h +++ b/remote.h @@ -209,6 +209,8 @@ struct branch { struct refspec **merge; int merge_nr; int merge_alloc; + + const char *push_tracking_ref; }; struct branch *branch_get(const char *name); @@ -229,6 +231,14 @@ int branch_merge_matches(struct branch *, int n, const char *); */ const char *branch_get_upstream(struct branch *branch, struct strbuf *err); +/** + * Return the tracking branch that corresponds to the ref we would push to + * given a bare `git push` while `branch` is checked out. + * + * The return value and `err` conventions match those of `branch_get_upstream`. + */ +const char *branch_get_push(struct branch *branch, struct strbuf *err); + /* Flags to match_refs. */ enum match_refs_flags { MATCH_REFS_NONE = 0, -- cgit v1.3-5-g9baa