On Thu, Dec 25, 2014 at 11:17 PM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote: > On 12/19/2014 08:39 PM, Stefan Beller wrote: >> Add a command line argument to the git push command to request atomic >> pushes. >> >> [...] >> >> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt >> index 21b3f29..da63bdf 100644 >> --- a/Documentation/git-push.txt >> +++ b/Documentation/git-push.txt >> @@ -9,7 +9,7 @@ git-push - Update remote refs along with associated objects >> SYNOPSIS >> -------- >> [verse] >> -'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>] >> +'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=<git-receive-pack>] >> [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose] >> [-u | --set-upstream] [--signed] >> [--force-with-lease[=<refname>[:<expect>]]] >> @@ -136,6 +136,11 @@ already exists on the remote side. >> logged. See linkgit:git-receive-pack[1] for the details >> on the receiving end. >> >> +--atomic:: >> + Use an atomic transaction on the remote side if available. >> + Either all refs are updated, or on error, no refs are updated. >> + If the server does not support atomic pushes the push will fail. >> + >> [...] > > I'd like to discuss the big picture around this feature. I don't think > that any of these questions are blockers, with the possible exception of > the question of whether "--atomic" should fall back to non-atomic if the > server doesn't support atomic pushes. > > > 1. Should "--atomic" someday become the default? > > You seem to imply that "--atomic" might become the default sometime in > the future. (I realize that this patch series does not propose to change > the default but let's talk about it anyway.) In the real world, the most > common reason for an "--atomic" push to fail would be that somebody else > has pushed to a branch since our last update, resulting in a non-ff > error. Would I find out about such an error before or after I have > transferred my objects to the server? Another idea which I picked up from later patches in Ronnies original series would be to have a hidden refs directory at the server side. Then the push command would first transfer all the objects to this hidden refs space and at the very end the server would just update all the refs atomically. If that fails because somebody else has just pushed stuff to one of the branches you intended to update, the changes are still at the server in the hidden space. So if you just did a pull/merge on that branch and then try to push again, you would not have to transmit the objects again as they are already there at the server. These changes in such a hidden space on the server side could be just normal refs just not(never!) advertised to users. > > If I only find out at the end of the transfer, then it could be a pretty > frustrating experience pushing a lot of references to a server over a > slow connection. After waiting for a long transfer to complete the user > would find out that the push was rejected and everything has to be done > again from scratch. In such cases non-"--atomic" behavior might be > attractive: any references that can be updated should be updated, so > that not *all* of the objects have to be pushed again. > > Even *if* "--atomic" becomes the default, we would certainly want to > support a "--no-atomic" (or "--non-atomic"?) option to get the old > behavior. It might be a good idea to add that option now, so that > forward-looking script writers can start explicitly choosing "--atomic" > vs. "--no-atomic". That's a good point. Though I am not sure where you'd want me to add the --no-atomic flag. I don't think we'll change send-pack as it's plumbing. So current unmaintained scripts should continue to work the way they always did, i.e. having the --no-atomic behavior. The atomic behavior made default would just be part of git push, which is porcelain which is expected to change? But there it makes sense to have a --no-atomic option in place. > > > 2. Is this an option that users will want to specify via the command line? > > For scripts that want to insist on "atomic" updates, it is no problem to > specify "--atomic" on the command line. > > But supposing that "--atomic" is a good default for some people, it > would be awkward for them to have to specify it on every "git push" > invocation. It therefore might be nice to have a configuration setting > to choose whether "--atomic" is the default. That's true. Would it make sense to include that in this series or delay it for another series? I don't want to make the series so large again so it becomes reviewer-unfriendly. > > Also (see above) it might be useful to set "--atomic" only for > particular servers (for example, only for those to which you have a fast > connection). This suggests that the "atomic/non-atomic" configuration > should be settable on a per-remote basis. So on the client side you want to configure on a per-remote basis which default behavior to use and also at the server side if you advertise it at all? > > > 3. What should happen if the server doesn't support atomic pushes? > > It seems to me that there are four reasonable behaviors WRT atomic > pushes. I'll give them tentative names for the purposes of this discussion: > > "force" -- Insist on an atomic push, and fail if the server does not > support atomic pushes > > "true" -- Use atomic push if supported by the server. If not, emit a > warning and fall back to non-atomic. I am not sure about this one. This only makes sense if atomic were the default and the user doesn't really care about atomicity and just wants to get things to the remote. But for this kind of users we want the "auto" option, so the user is not bothered by some warning they don't care about. If the user however had the intention of using atomic behavior (i.e. has specified --atomic explicitly), the user probably wants to have the "force" behavior. > > "auto" -- Use atomic push if supported by the server. If not, silently > fall back to non-atomic. > > "false" -- Push non-atomically regardless of whether the server supports > atomic pushes. > > To make it practical to set "atomic" as a universal default, we will > have to be able to deal with servers that don't (yet) support atomic > pushes. Therefore, we would want to use either "true" or "auto" (as > opposed to "force") as the default. > > Even in such a case, it would be nice for the user to be able to > suppress any warnings for servers that don't support atomic updates. So > if "true" becomes the default, then we might want to support "auto" as well. > > (One could argue that once "atomic" becomes the default, then anybody > who has to deal with an old server should just set "non-atomic" as the > default for that server. But even aside from the inconvenience to the > user, this is not a good alternative. A user who made that change > wouldn't benefit from atomic pushes once the server is upgraded to > support them.) > > The command-line "--atomic" option is proposed to request "force" > behavior. On the one hand that makes sense; the user has explicitly > requested an atomic push, so the command should fail if it is not > possible. But on the other hand, if a particular server doesn't (yet) > support atomic pushes, what recourse does the user have but to run the > push again non-atomically? So it might be more expedient for the > "--atomic" option to be equivalent to "true" instead of "force". I don't > have a strong opinion on this either way. It doesn't take much time to find out if the server supports atomic pushes as you don't have to send the objects, but just wait until the server has advertised its refs and capabilities. Then the user could rerun the command line without having the --atomic flag set or even using --no-atomic. Thanks for your detailed discussion as I wasn't thinking so far ahead yet. > I'd like to discuss the big picture around this feature. I don't think > that any of these questions are blockers, with the possible exception of > the question of whether "--atomic" should fall back to non-atomic if the > server doesn't support atomic pushes. If we make atomic default, then we should have this kind of fallback, if not explicitly suppressed (i.e. the user should experience as little rejection as possible, warnings/hints may be ok? If the user really wants to do it atomically, make it so) For the current state after this patch series when atomic is not default (yet), we probably don't need the fall back as we only talk about the "force" option with this series. > > Michael > > -- > Michael Haggerty > mhagger@xxxxxxxxxxxx > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html