Re: [PATCH 6/7] push.c: add an --atomic argument

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 29, 2014, 20:33 UTC
Message-ID: <xmqq7fxayz4u.fsf@gitster.dls.corp.google.com>
In-Reply-To: <549D0B78.402@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 14 quoted lines

> 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?

That question is pretty much rhetorical, as certain rejections you cannot fundamentally implement without having the data at hand.

> 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.

We'd like to have a long overdue protocol update for fetch and push soonish anyawy (perhaps in the first half of 2015) and part of that should include unified logic for common ancestor negotiation between fetch and push [*1*]. We should be able to ease that with an optimization similar to quickfetch done on the fetch side once that happens.

Show 5 quoted lines

> 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".

Perhaps. But on the other hand, pushing multiple refs at the same time is a sign that they are related and need to go together. The reason why one but not others fails would be an indication that there is somebody else pushing into the same repository and the pusher and the other party are stepping on each other's toes, which should be resolved primarily by inter-developer communication, not with "--no-atomic" workaround.

Show 14 quoted lines

> 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.
>
> 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.

I think you are hinting to have remote.atomicPush = {yes,no} that is weaker than remote.$nick.atomicPush = {yes,no} or something like that. I agree that would be a good direction to go.

> 3. What should happen if the server doesn't support atomic pushes?

If you asked for atomic push explicitly and if the server cannot support it, push should fail.

If the only reason we are doing atomic is because in some future we flipped the default (i.e. no remote.*.atomicPush or --atomic option from the command line), then it might be OK to continue with a warning.

I however think that the automatic demotion is too much complexity for such a simple option. "Please be atomic if you can but I'd take non-atomic one if you do not want to give me atomic one" that is responded by "I'd do non-atomic then, as you are perfectly happy without" is not very useful---such a pusher should just say "I accept non-atomic", which is what "--no-atomic" is for.

[Footnotes]

*1* Two other big ones are syntax change to have an explicit
    extension packets instead of hiding the capability after NUL,
    and resolving the "who speaks first" issue.