Re: [PATCH 2/6] remote.c: drop "remote" pointer from "struct branch"
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 31, 2015, 20:50 UTC
- Message-ID
- <xmqqvbhgq4ci.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <20150331173555.GB18912@peff.net>
Jeff King <peff@peff.net> writes:
Show 19 quoted lines
> When we create each branch struct, we fill in the
> "remote_name" field from the config, and then fill in the
> actual "remote" field based on that name. However, it turns
> out that nobody really cares about this field. The only two
> sites that access it 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" for
> that; we can just look it up for the duration of the
> operation.
>
> Getting rid of it drops one potential source of confusion:
> is the value the match for "remote_name", or is it the
> remote we would fetch from when on that branch (i.e., does
> it fall back to "origin")?I had to read the above three times before I realized that you were wondering "what is the value of this 'remote' field? is it what remote_get() would give us for 'remote_name' and is NULL if remote_name is not set, or is it never NULL and instead have the remote for 'origin' if remote_name is not set?"
But perhaps it is just me.
We certainly have duplicated information between the two fields, and it first looked somewhat unnatural that you kept the name with which you need to trigger a search for the structure, instead of keeping the structure, one of whose field is its name already. Perhaps there was a valid reason behind this choice, and I am guessing that it is probably because it will not let you differenciate the case where the user explicitly said 'origin' and we used 'origin' as a fallback, if you keep a "remote" field that stores the instance of the remote structure for 'origin' without keeping "remote_name".
But we shouldn't leave it for readers to guess.
Show 78 quoted lines
> When we add pushremote_name, this question would get even
> more confusing, as pushremotes have a more complicated
> lookup procedure. It would be nice for the code to be
> consistent between the remote and pushremote, and this takes
> us one step closer.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/merge.c | 2 +-
> remote.c | 14 ++++++++------
> remote.h | 1 -
> 3 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 3b0f8f9..1840317 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 fcd868d..d5fd605 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1633,15 +1633,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]),
> @@ -1661,11 +1666,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);
> return ret;
> }
>
> diff --git a/remote.h b/remote.h
> index 02d66ce..4bb6672 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;