Re: [PATCH 3/3] remote: introduce and fill branch->pushremote

[Jeff King <peff@peff.net>]

On Sun, Jan 12, 2014 at 10:41:06PM +0530, Ramkumar Ramachandra wrote:

> When a caller uses branch_get() to retrieve a "struct branch", they get
> the per-branch remote name and a pointer to the remote struct. However,
> they have no way of knowing about the per-branch pushremote from this
> interface. So, let's expose that information via fields similar to
> "remote" and "remote_name"; "pushremote" and "pushremote_name".

Makes sense. This is similar to what I posted before, but stops short of
setting branch->pushremote based on "default.pushremote". I think that's
a good thing. Your patch matches branch->remote better, and the logic
for doing that fallback should probably stay outside of the "struct
branch" construct.

All 3 patches look like sane building blocks to me.

One comment on this hunk, though:

>  		} else if (!strcmp(subkey, ".pushremote")) {
> +			if (git_config_string(&branch->pushremote_name, key, value))
> +				return -1;
>  			if (branch == current_branch)
> -				if (git_config_string(&pushremote_name, key, value))
> -					return -1;
> +				pushremote_name = branch->pushremote_name;

In this code (both before and after your patch), pushremote_name does
double-duty for storing both "remote.pushdefault", and the current
branch's "branch.*.pushremote". I introduced an extra variable in my
version of the patch to store "remote.pushdefault" directly, and turned
pushremote_name into an alias (either to the current branch config, or
to the global config).

I did that for two reasons, one minor and one that I think will come up
further in the topic:

  1. After your patch "pushremote_name" sometimes owns its memory (if
     allocated for remote.pushdefault), and sometimes not (if an alias to
     branch.*.pushremote). This isn't a problem in the current code,
     because we never actually free() the string, meaning that if you
     set push.default twice, we leak. But that probably does not matter
     too much, and we have many such minor leaks of global config.

  2. If the current branch has a branch.*.pushremote set, but we want to
     know where a _different_ branch would be pushed, we have no way to
     access remote.pushdefault (it gets overwritten in the hunk above).

     @{upstream} does not have this problem, because it is _only_
     defined if branch.*.remote is set. There is no such thing as
     defaulting to a "remote.default" (or "origin") there, and we never
     need to look at default_remote_name.

     For @{publish}, though, I think we will want that default. The
     common config will be to simply set "remote.pushdefault", rather
     than setting up "branch.*.pushremote" for each branch, and we would
     want @{publish} to handle that properly.

So I think your patch is OK as-is, as the problem in (2) does not show
up until later in the series. But I suspect you will need to do
something to address it (and I think it is fine as a patch that comes
later to do that refactoring).

-Peff