git push --confirm ?

git push --confirm ?

[Owen Taylor]

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

People sometimes push things they don't mean to. Depending on the
workflow and environment, that can be anywhere between a trivial
nuisance to an embarrassing and awkward cleanup.

It would seem handy to me to have a --confirm option to git-push
(and 'core.confirm-push' to turn it on be default), that would
have the following behavior:

 * An initial --dry-run pass is done but with more verbosity -
   for updates of existing references, it would show what commits
   were being added or removed in a one-line format.

 * The user is prompted if they want to proceed
 
 * If the user agrees, then the push is run without --dry-run

I've attached a mockup of this as a porcelain 'git safe-push'.

(Done not using 'git push --porcelain' because I wanted it to work
with existing released Git versions. I was hoping to be able to do 
'git config alias.push safe-push', but no facility for interactive 
only aliases...)

I think this wouldn't be too hard to add to 'git push', though
I haven't tried to code it. Yes, it's not atomic without protocol
changes - I think that's OK:

 - If the push isn't being forced intermediate ref updates will
   be caught as a non-fast-forward in the second pass.

 - If the push is being forced, you might overwrite someone else's
   push anyways even without --confirm.

Thoughts on whether this makes sense as an addition?

- Owen


[-- Attachment #2: git-safe-push --]
[-- Type: application/x-shellscript, Size: 2584 bytes --]

Re: git push --confirm ?

[Jeff King]

On Sat, Sep 12, 2009 at 01:51:37PM -0400, Owen Taylor wrote:

>  * An initial --dry-run pass is done but with more verbosity -
>    for updates of existing references, it would show what commits
>    were being added or removed in a one-line format.
> 
>  * The user is prompted if they want to proceed
>  
>  * If the user agrees, then the push is run without --dry-run
>
> [...]
>
> I think this wouldn't be too hard to add to 'git push', though
> I haven't tried to code it. Yes, it's not atomic without protocol
> changes - I think that's OK:

I have never wanted such a feature, so maybe I am a bad person to
comment, but I don't see much advantage from a UI standpoint over what
we have now. Which is "git push --dry-run", check to see if you like it,
and then re-run without --dry-run. If you just want to see more output
in the first --dry-run, then that is easy to do with an alternate
format.

But what _would_ be useful is doing it atomically. You can certainly do
all three of those steps from within one "git push" invocation, and I
think that is enough without any protocol changes. The protocol already
sends for each ref a line like:

  <old-sha1> <new-sha1> <ref>

and receive-pack will not proceed with the update unless the <old-sha1>
matches what is about to be changed.

>  - If the push isn't being forced intermediate ref updates will
>    be caught as a non-fast-forward in the second pass.
> 
>  - If the push is being forced, you might overwrite someone else's
>    push anyways even without --confirm.

Yeah, "--force" is not very fine-grained. I wonder if rather than a
complete --confirm you would rather have something iterative like:

  $ git push --interactive
  Pushing to server:/path/to/repo.git
    * [new branch]      topic -> topic
  Push this branch [Yn]?
      5ad9dce..cfc497a  topic -> topic
  Push this branch [Yn]?
      5ad9dce...cfc497a topic -> topic (non-fast forward)
  Force this branch [yN]?

where of course the actual output text and y/n defaults are subject to
debate. You could even have a 'v' option at each prompt to visualize the
differences in gitk so you can easily get more information on what you
might be overwriting in a non-fast-forward scenario.

-Peff

Re: git push --confirm ?

[Owen Taylor]

On Sat, 2009-09-12 at 14:43 -0400, Jeff King wrote:
> On Sat, Sep 12, 2009 at 01:51:37PM -0400, Owen Taylor wrote:
> 
> >  * An initial --dry-run pass is done but with more verbosity -
> >    for updates of existing references, it would show what commits
> >    were being added or removed in a one-line format.
> > 
> >  * The user is prompted if they want to proceed
> >  
> >  * If the user agrees, then the push is run without --dry-run
> >
> > [...]
> >
> > I think this wouldn't be too hard to add to 'git push', though
> > I haven't tried to code it. Yes, it's not atomic without protocol
> > changes - I think that's OK:
> 
> I have never wanted such a feature, so maybe I am a bad person to
> comment, but I don't see much advantage from a UI standpoint over what
> we have now. Which is "git push --dry-run", check to see if you like it,
> and then re-run without --dry-run. If you just want to see more output
> in the first --dry-run, then that is easy to do with an alternate
> format.

The main UI advantage is that you can adjust the default with 'git
config' it on and leave it on. The time you screw up is not when you are
worried that you are going to push the wrong thing. It's when you are
you know exactly what 'git push' is going to do and it does something
different.

Secondarily, I don't really find the output of 'git push --dry-run'
that great - it's pretty good for finding out what branches you are
going to push... that you correctly understood the syntax of git push
and the relationship to your branch configuration, but not so good at
seeing what's going to be pushed,

If it shows:

 72b3142..1fa3134 my-topic -> my-topic
 12a31aa..34f2621 master -> master

That doesn't necessarily warn you that along with the bug fix you think
you are pushing you have a big merge into master sitting there that you
haven't finished testing. For updates, showing a commit count and (a
probably limited number of) commit subjects would avoid having to
cut-and-paste the update summary into git log.

As you say, maybe that's something that just needs to be fixed
with a better format for --dry-run. But that doesn't negate the main UI
advantage.

> But what _would_ be useful is doing it atomically. You can certainly do
> all three of those steps from within one "git push" invocation, and I
> think that is enough without any protocol changes. The protocol already
> sends for each ref a line like:
> 
>   <old-sha1> <new-sha1> <ref>
> 
> and receive-pack will not proceed with the update unless the <old-sha1>
> matches what is about to be changed.

Hmm, yeah, I've certainly looked at git-receive-pack(1) before but
hadn't internalized that --force was client side. Certainly doing it
with a single atomic pass is the better way to do it.

(Wouldn't work for rsync and http pushes, right? A simple "Not
supported" perhaps.)

> >  - If the push isn't being forced intermediate ref updates will
> >    be caught as a non-fast-forward in the second pass.
> > 
> >  - If the push is being forced, you might overwrite someone else's
> >    push anyways even without --confirm.
> 
> Yeah, "--force" is not very fine-grained. I wonder if rather than a
> complete --confirm you would rather have something iterative like:
> 
>   $ git push --interactive
>   Pushing to server:/path/to/repo.git
>     * [new branch]      topic -> topic
>   Push this branch [Yn]?
>       5ad9dce..cfc497a  topic -> topic
>   Push this branch [Yn]?

Hmm, of two minds about this. Doing it as a pick-and-choose
--interactive does integrate it conceptually with other parts of Git.
And probably is occasionally useful.

But it makes it considerably less convenient to just config on.
Because any time you want to push more than 2-3 refs at once you'll have
to add --no-interactive.

It also increases the amount of reading - if I see all the branches at
once that are being pushed I can immediately notice that I'm pushing two
branches when I thought I was pushing one, without actually having to
read the branch names.

My conception of the feature is as a safety harness. That some people
will be willing to pay a keystroke or two for that double check that
their mental model matches reality.

      5ad9dce...cfc497a topic -> topic (non-fast forward) 
> Force this branch [yN]?

This one is a disaster waiting to happen. Even with the reversed
defaults you may well have the 'y<return>' habit going. Unless the
non-fast-forward looks completely different (Red and Blinky) you
probably are going to go right past it.

- Owen

Re: git push --confirm ?

[Jeff King]

[cc'd Daniel; I think this proposal for a "confirm push" might interact
with your foreign VCS work a bit. I'm not sure anymore what would be the
right level for inserting this code.]

On Sat, Sep 12, 2009 at 04:11:06PM -0400, Owen Taylor wrote:

> The main UI advantage is that you can adjust the default with 'git
> config' it on and leave it on. The time you screw up is not when you are
> worried that you are going to push the wrong thing. It's when you are
> you know exactly what 'git push' is going to do and it does something
> different.

That makes sense. I would not want to use such a feature, but I see the
use case you are talking about (see, I told you I was bad person to
comment. ;) ).

It should be pretty straightforward to implement for the git protocol.
Pushing goes something like:

  1. Get the list of refs from the remote.

  2. Using the desired refspecs (either configured or from the command
     line), make a list of src/dst pairs of refs to be pushed.

  3. For each ref pair, send the "<old> <new> <name>" triple to the
     remote (or not, if it is already up-to-date, a non-fast-forward,
     etc).

  4. Send the packed objects.

  5. For each ref pair, print the status in a summary table.

So you would just want a "2.5" where you show something similar to the
summary table and get some confirmation (or abort). An iterative "do you
want to push this ref" strategy would be similar; just mark the refs you
do and don't want to push.

The tricky thing will be handling different transports. Some of that
code has been factored out, but I haven't looked at the details. On top
of that, I think Daniel is working in this area for his support
of foreign VCS helpers (and other transports like libcurl are getting
pushed out into their own helpers). So he may have a better idea of how
to go about this sanely.

> haven't finished testing. For updates, showing a commit count and (a
> probably limited number of) commit subjects would avoid having to
> cut-and-paste the update summary into git log.
> 
> As you say, maybe that's something that just needs to be fixed
> with a better format for --dry-run. But that doesn't negate the main UI
> advantage.

Sure. I just think the two concepts are somewhat orthogonal (though you
would probably want to enable them together for your particular
workflow). It sounds like you want something like (and obviously you
could have a config option to avoid typing --log-changes each time):

  $ git push --dry-run --log-changes
  To server:/path/to/repo.git
    5ad9dce..cfc497a  topic -> topic
      abcd123: commit subject 1
      cfc497a: commit subject 2

That can potentially get long, though. I'm not sure if you would want to
abbreviate it in some way, and if so, how.

> Hmm, yeah, I've certainly looked at git-receive-pack(1) before but
> hadn't internalized that --force was client side. Certainly doing it
> with a single atomic pass is the better way to do it.

There is actually a server-side analogue, which is the
"receive.denyNonFastForwards" config option, and it defaults to "off".
The "--force" option is a client side way of helping you be polite. The
receive config option is about actual policy.

> (Wouldn't work for rsync and http pushes, right? A simple "Not
> supported" perhaps.)

Rsync, at least, is already non-atomic because there is no way to do
locking. So you wouldn't make anything worse by supporting this feature
(though you do widen the gap for the race condition by waiting for user
input). I'm not sure anyone really cares about rsync these days, though.

I believe http-push actually does some kind of DAV locking. So there's
no reason you this couldn't work for http push.

> Hmm, of two minds about this. Doing it as a pick-and-choose
> --interactive does integrate it conceptually with other parts of Git.
> And probably is occasionally useful.
> 
> But it makes it considerably less convenient to just config on.
> Because any time you want to push more than 2-3 refs at once you'll have
> to add --no-interactive.
> 
> It also increases the amount of reading - if I see all the branches at
> once that are being pushed I can immediately notice that I'm pushing two
> branches when I thought I was pushing one, without actually having to
> read the branch names.

I think it really depends on your workflow, and how many refs you are
typically pushing. So yeah, I can see that the iterative asking is not
really a replacement for what you are asking for.

-Peff

Re: git push --confirm ?

[Daniel Barkalow]

On Sat, 12 Sep 2009, Jeff King wrote:

> [cc'd Daniel; I think this proposal for a "confirm push" might interact
> with your foreign VCS work a bit. I'm not sure anymore what would be the
> right level for inserting this code.]
> 
> On Sat, Sep 12, 2009 at 04:11:06PM -0400, Owen Taylor wrote:
> 
> > The main UI advantage is that you can adjust the default with 'git
> > config' it on and leave it on. The time you screw up is not when you are
> > worried that you are going to push the wrong thing. It's when you are
> > you know exactly what 'git push' is going to do and it does something
> > different.
> 
> That makes sense. I would not want to use such a feature, but I see the
> use case you are talking about (see, I told you I was bad person to
> comment. ;) ).
> 
> It should be pretty straightforward to implement for the git protocol.
> Pushing goes something like:
> 
>   1. Get the list of refs from the remote.
> 
>   2. Using the desired refspecs (either configured or from the command
>      line), make a list of src/dst pairs of refs to be pushed.
> 
>   3. For each ref pair, send the "<old> <new> <name>" triple to the
>      remote (or not, if it is already up-to-date, a non-fast-forward,
>      etc).
> 
>   4. Send the packed objects.
> 
>   5. For each ref pair, print the status in a summary table.
> 
> So you would just want a "2.5" where you show something similar to the
> summary table and get some confirmation (or abort). An iterative "do you
> want to push this ref" strategy would be similar; just mark the refs you
> do and don't want to push.
> 
> The tricky thing will be handling different transports. Some of that
> code has been factored out, but I haven't looked at the details. On top
> of that, I think Daniel is working in this area for his support
> of foreign VCS helpers (and other transports like libcurl are getting
> pushed out into their own helpers). So he may have a better idea of how
> to go about this sanely.

The status used to be that each method of pushing implemented 
approximately the rules you give, but implemented it separately. Now 
there's a common implementation of those rules, with (1) being a method 
call, (3&4) being a method, and the rest being in transport_push(). 
However, rsync and curl have not yet been converted to the new style. When 
there is support for push with helpers, it will only use the new style 
(because it would be pointlessly annoying to implement the git rules for 
refspecs in the helpers).

So it should be easy to put something into transport_push to do a step 2.5 
confirmation, and a bit more work (which ought to get done anyway) to make 
it apply to rsync and http URLs.

(Furthermore, currently, http-push is a separate program from the fetch 
code, which is moving from the main git executable to git-remote-curl; the 
http push code should probably actually move to git-remote-curl, so that 
there is a single external program taking care of all operations on such 
URLs and there is less complexity in how the curl-using code is 
structured.)

	-Daniel
*This .sig left intentionally blank*

Re: git push --confirm ?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But what _would_ be useful is doing it atomically. You can certainly do
> all three of those steps from within one "git push" invocation, and I
> think that is enough without any protocol changes. The protocol already
> sends for each ref a line like:
>
>   <old-sha1> <new-sha1> <ref>
>
> and receive-pack will not proceed with the update unless the <old-sha1>
> matches what is about to be changed.

Be careful that using that information and doing things in one session
won't give you atomicity in the sense that it may still fail after you
said "yes that is what I want to push, really" to the confirmation
question.

It does save you an extra connection, compared to separate invocations
without and then with --dry-run, so it still is a plus.

I do not think this is an unreasonable option to have.  Just please don't
justify this change based on atomicity argument, but justify it as a mere
convenience feature.

Re: git push --confirm ?

[Jeff King]

On Sat, Sep 12, 2009 at 05:41:23PM -0700, Junio C Hamano wrote:

> > But what _would_ be useful is doing it atomically. You can certainly do
> > all three of those steps from within one "git push" invocation, and I
> > think that is enough without any protocol changes. The protocol already
> > sends for each ref a line like:
> >
> >   <old-sha1> <new-sha1> <ref>
> >
> > and receive-pack will not proceed with the update unless the <old-sha1>
> > matches what is about to be changed.
> 
> Be careful that using that information and doing things in one session
> won't give you atomicity in the sense that it may still fail after you
> said "yes that is what I want to push, really" to the confirmation
> question.

Of course, but that issue exists already. It is just that the window
between receiving the refs and then asking them to be updated is much
smaller when there is no human input in the loop (and since we haven't
actually _shown_ the list to the user, it appears atomic to them).

I think this type of atomicity is fine for this application. The point
of this is to err on the side of caution. So it is OK to say "Push
this?" and then after the user has confirmed say "Oops, somebody pushed
something else while we were waiting for your input. Try again." The
important thing is to not say "Push this?", have the user confirm that
what they are pushing over is OK, and then end up pushing over something
different (which is what can happen with separate push invocations).

The only way to get true atomicity across the confirmation and push
would be to take a lock at the beginning of the push session. Which is
too coarse-grained in the first place (it disallows simultaneous update
of unrelated refs), but would also require protocol updates.

> It does save you an extra connection, compared to separate invocations
> without and then with --dry-run, so it still is a plus.
> 
> I do not think this is an unreasonable option to have.  Just please don't
> justify this change based on atomicity argument, but justify it as a mere
> convenience feature.

I don't agree. Making sure we use the _same_ <old-sha1> in the
confirmation output we show to the user and in the ref update we send to
the remote is critical for this to be safe.

-Peff

Re: git push --confirm ?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> I do not think this is an unreasonable option to have.  Just please don't
>> justify this change based on atomicity argument, but justify it as a mere
>> convenience feature.
>
> I don't agree. Making sure we use the _same_ <old-sha1> in the
> confirmation output we show to the user and in the ref update we send to
> the remote is critical for this to be safe.

OK.

You may be giving stale info to the user if somebody else is pushing from
sideways anyway, and the difference between a separate --dry-run and real
push when that happens is where and how the human waits.

With a separate --dry-run, the wait happens while the output is examined
offline.  The user may run "git log --oneline old...new" himself before
deciding to run the real push.

With --confirm, the wait happens while the --confirm waits for the human,
and perhaps the command does "git log --oneline old...new" as convenience.
While all this is happening, the TCP connection to the remote end is still
kept open.  We do not lock anything, but if somebody else pushed from
sideways, at the end of this session we would notice that, and the push
will be aborted.

This somewhat makes me worry about DoS point of view, but it does make it
somewhat safer.

I think the largest practical safety would come from the fact that this
would make it convenient (i.e. a single command "push --confirm") than
having to run two separate ones with manual inspection in between.  A
safety feature that is cumbersome to use won't add much to safety, as that
is unlikely to be used in the first place.

Re: git push --confirm ?

[Jeff King]

On Sun, Sep 13, 2009 at 03:37:32AM -0700, Junio C Hamano wrote:

> With --confirm, the wait happens while the --confirm waits for the human,
> and perhaps the command does "git log --oneline old...new" as convenience.
> While all this is happening, the TCP connection to the remote end is still
> kept open.  We do not lock anything, but if somebody else pushed from
> sideways, at the end of this session we would notice that, and the push
> will be aborted.
> 
> This somewhat makes me worry about DoS point of view, but it does make it
> somewhat safer.

I don't see how it makes a DoS any worse. A malicious attacker can
always open the TCP connection and let it sit; we are changing only the
client code, after all.

It does increase the possibility of _accidentally_ wasting a TCP
connection. I don't know if that is a real-world problem or not. I would
think heavily-utilized sites might put a time-out on the connection to
avoid such a DoS in the first place.

However, such a timeout is perhaps reason for us to be concerned with
implementing this feature with a single session. Will users looking at
the commits for confirmation delay enough to hit configured timeouts,
dropping their connection and forcing them to start again?

One other way to implement this would be with two TCP connections:

  1. git push --dry-run, recording <old-sha1> for each ref to be pushed.
     Afterwards, drop the TCP connection.

  2. Get confirmation from the user.

  3. Do the push again, confirming that the <old-sha1> values sent by
     the server match what we showed the user for confirmation. If not,
     abort the push.

Besides being a lot more annoying to implement, there is one big
downside: in many cases the single TCP connection is a _feature_. If you
are pushing via ssh and providing a password manually, it is a
significant usability regression to have to input it twice.

Also, given that ssh is going to be by far the biggest transport for
pushing via the git protocol, I suspect any timeouts are set for
_before_ the authentication phase (i.e., SSH times you out if you don't
actually log in). So in that sense it may not be worth worrying about
how long we take during the push itself.

> I think the largest practical safety would come from the fact that this
> would make it convenient (i.e. a single command "push --confirm") than
> having to run two separate ones with manual inspection in between.  A
> safety feature that is cumbersome to use won't add much to safety, as that
> is unlikely to be used in the first place.

Sure. But that is about packaging it up as a single session for the
user. If there is no concern about atomicity, you could do that with a
simple wrapper script.

-Peff

Re: git push --confirm ?

[Uri Okrent]

Jeff King wrote:
[snip]
> Besides being a lot more annoying to implement, there is one big
> downside: in many cases the single TCP connection is a _feature_. If you
> are pushing via ssh and providing a password manually, it is a
> significant usability regression to have to input it twice.
> 
> Also, given that ssh is going to be by far the biggest transport for
> pushing via the git protocol, I suspect any timeouts are set for
> _before_ the authentication phase (i.e., SSH times you out if you don't
> actually log in). So in that sense it may not be worth worrying about
> how long we take during the push itself.

That doesn't seem like a huge hurdle to overcome. Most ssh clients support some
sort of ServerAliveInterval parameter for just this reason. Sending a keep alive
packet every 60 seconds or so while waiting for user confirmation doesn't seem
all that egregious.
-- 
    Uri

Please consider the environment before printing this message.
http://www.panda.org/how_you_can_help/