Re: [PATCH 3/3] remote: introduce and fill branch-pushremote
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] remote: introduce and fill branch-pushremote
Jeff King wrote: 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. Not sure I understand what the problem is. Let's say we have two branches: master, and side with remote.pushdefault = ram, branch.*.remote = origin, and branch.side.pushremote = peff. Now, when I query master's pushremote, I get ram and when I query side's pushremote, I get peff; all the logic for falling-back from branch.*.pushremote to remote.pushdefault to branch.*.remote is in branch_get(), so I need to do nothing extra on the caller-side. From the caller's perspective, why does it matter if the pushremote of a particular branch is due to branch.*.pushremote or remote.pushdefault? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] remote: introduce and fill branch-pushremote
On Mon, Jan 13, 2014 at 04:52:52PM +0530, Ramkumar Ramachandra wrote: Not sure I understand what the problem is. Let's say we have two branches: master, and side with remote.pushdefault = ram, branch.*.remote = origin, and branch.side.pushremote = peff. Now, when I query master's pushremote, I get ram and when I query side's pushremote, I get peff; all the logic for falling-back from branch.*.pushremote to remote.pushdefault to branch.*.remote is in branch_get(), so I need to do nothing extra on the caller-side. From the caller's perspective, why does it matter if the pushremote of a particular branch is due to branch.*.pushremote or remote.pushdefault? Imagine your HEAD is at side. What should master@{publish} produce? I would argue ram/master. Where does ram come from in your code? It does not matter for actually pushing, because to do a non-default push, you must always specify a remote. But @{publish} will ask the question even if I am on 'side' now, what would happen if I were to default-push on 'master'?. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] remote: introduce and fill branch-pushremote
Jeff King p...@peff.net writes: It does not matter for actually pushing, because to do a non-default push, you must always specify a remote. But @{publish} will ask the question even if I am on 'side' now, what would happen if I were to default-push on 'master'?. In a similar wording to yours, it can be said that B@{upstream} is what would happen if I were to default-pull on 'B'?. A related tangent is what should B@{publish} should yield when there is no triangular configuration variables like remote.pushdefault, branch.B.pushremote and a possible future extension branch.B.push are defined. The definition you gave, i.e. if I were to default-push, gives a good guideline, I think. I.e. git push origin master does tell us to push out 'master', but it does not explicitly say what ref to update. It may be set to update their remotes/satellite/master when we are emulating a fetch in reverse by pushing, via e.g. [remote origin] push = refs/heads/master:refs/remotes/satellite/master and it would be intuitive if we make master@{publish} resolve to refs/remotes/satellite/master in such a case. One thing that makes things a bit fuzzy is what should happen if you have more than one push destinations. For example: [remote home] pushurl = ... push = refs/heads/master:refs/remotes/satellite/master [remote github] pushurl = ... mirror [remote] pushdefault = ??? git push home updates their 'refs/remotes/satellite/master' with my 'master' with the above, while git push github will update their 'refs/heads/master' with 'master'. We can say master@{publish} is 'remotes/satellite/master' if remote.pushdefault (or 'branch.master.pushremote) is set to 'home', it is 'master' if it is 'github', and if git push while sitting on 'master' does not push it anywhere then master@{publish} is an error. There may be a better definition of what if I were to default-push really means, but I don't think of any offhand. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] remote: introduce and fill branch-pushremote
On Mon, Jan 13, 2014 at 12:15:08PM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: It does not matter for actually pushing, because to do a non-default push, you must always specify a remote. But @{publish} will ask the question even if I am on 'side' now, what would happen if I were to default-push on 'master'?. In a similar wording to yours, it can be said that B@{upstream} is what would happen if I were to default-pull on 'B'?. Right. I wondered at first if there was a similar bug in @{upstream}, but as I noted earlier, it is not defined if a per-branch remote is not set. The answer to your question above is nothing, so we do not have to worry about it. :) A related tangent is what should B@{publish} should yield when there is no triangular configuration variables like remote.pushdefault, branch.B.pushremote and a possible future extension branch.B.push are defined. The definition you gave, i.e. if I were to default-push, gives a good guideline, I think. Yes, that is what I tried for with my original patches. (e.g., push.default=upstream should just make @{publish} a synonym for @{upstream}, which is what my patch did). I punted on simple, but it would ideally do the same thing as push. Which is why I do not think my patches are appropriate as-is; they need to somehow share the logic with git push rather than try to reimplement it. I.e. git push origin master does tell us to push out 'master', but it does not explicitly say what ref to update. It may be set to update their remotes/satellite/master when we are emulating a fetch in reverse by pushing, via e.g. [remote origin] push = refs/heads/master:refs/remotes/satellite/master and it would be intuitive if we make master@{publish} resolve to refs/remotes/satellite/master in such a case. Right. And my patches did that (or at least I intended them to :) ) by applying the push refspec (if any), and then applying the fetch refspec on top of that. But again, that seems like policy that should be shared with git push. That being said, I do not think your example is the best one for @{publish}. You have not specified any remote at all. I think the closest push behavior for @{publish} would be something like: git checkout master git push I.e., where would _that_ push go? One thing that makes things a bit fuzzy is what should happen if you have more than one push destinations. For example: [remote home] pushurl = ... push = refs/heads/master:refs/remotes/satellite/master [remote github] pushurl = ... mirror [remote] pushdefault = ??? git push home updates their 'refs/remotes/satellite/master' with my 'master' with the above, while git push github will update their 'refs/heads/master' with 'master'. We can say master@{publish} is 'remotes/satellite/master' if remote.pushdefault (or 'branch.master.pushremote) is set to 'home', it is 'master' if it is 'github', and if git push while sitting on 'master' does not push it anywhere then master@{publish} is an error. There may be a better definition of what if I were to default-push really means, but I don't think of any offhand. Exactly. I do not think the multiple push destinations matter here, because it is always what would I do if I were on the branch. At most one of them can be the default in that case (based on the config as you noted). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] remote: introduce and fill branch-pushremote
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. Helped-by: Jeff King p...@peff.net Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- remote.c | 15 --- remote.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/remote.c b/remote.c index a89efab..286cdce 100644 --- a/remote.c +++ b/remote.c @@ -351,9 +351,10 @@ static int handle_config(const char *key, const char *value, void *cb) explicit_default_remote_name = 1; } } 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; } else if (!strcmp(subkey, .merge)) { if (!value) return config_error_nonbool(key); @@ -1543,7 +1544,9 @@ struct branch *branch_get(const char *name) ret = current_branch; else ret = make_branch(name, 0); - if (ret ret-remote_name) { + if (!ret) + return ret; + if (ret-remote_name) { ret-remote = remote_get(ret-remote_name); if (ret-merge_nr) { int i; @@ -1557,6 +1560,12 @@ struct branch *branch_get(const char *name) } } } + if (ret-pushremote_name) + ret-pushremote = remote_get(ret-pushremote_name); + else if (pushremote_name) + ret-pushremote = remote_get(pushremote_name); + else + ret-pushremote = ret-remote; return ret; } diff --git a/remote.h b/remote.h index 00c6a76..ac5aadc 100644 --- a/remote.h +++ b/remote.h @@ -201,6 +201,9 @@ struct branch { const char *remote_name; struct remote *remote; + const char *pushremote_name; + struct remote *pushremote; + const char **merge_name; struct refspec **merge; int merge_nr; -- 1.8.5.2.313.g5abf4c0.dirty -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html